Prevent insertion of nil into foundation collections (#1698)

Now trying to insert into NSArray, NSDictionary, NSSet, and NSAttributedString will throw an NSException.

Fixes #1607

* Prevent insertion of nil into foundation collections

* Address CR feedback, throw for sets as well

* Missed one

Frameworks/Foundation/NSCFArray.mm

@@ -1,6 +1,6 @@
 //******************************************************************************
 //
-// Copyright (c) 2016 Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft. All rights reserved.
 //
 // This code is licensed under the MIT License (MIT).
 //
@@ -106,6 +106,7 @@ - (void)removeLastObject {
 
 - (void)replaceObjectAtIndex:(NSUInteger)index withObject:(NSObject*)obj {
     BRIDGED_THROW_IF_IMMUTABLE(_CFArrayIsMutable, CFArrayRef);
+    NS_COLLECTION_THROW_IF_NULL_REASON(obj, [NSString stringWithFormat:@"*** %@ object cannot be nil", NSStringFromSelector(_cmd)]);
     //  Fastpath
     CFRange range;
     range.location = index;
@@ -115,6 +116,13 @@ - (void)replaceObjectAtIndex:(NSUInteger)index withObject:(NSObject*)obj {
 
 - (void)insertObject:(NSObject*)objAddr atIndex:(NSUInteger)index {
     BRIDGED_THROW_IF_IMMUTABLE(_CFArrayIsMutable, CFArrayRef);
+    NS_COLLECTION_THROW_IF_NULL_REASON(objAddr, [NSString stringWithFormat:@"*** %@ object cannot be nil", NSStringFromSelector(_cmd)]);
+    if (objAddr == nil) {
+        @throw [NSException exceptionWithName:NSInvalidArgumentException
+                                       reason:[NSString stringWithFormat:@"*** %@ object cannot be nil", NSStringFromSelector(_cmd)]
+                                     userInfo:nil];
+    }
+
     CFArrayInsertValueAtIndex(static_cast<CFMutableArrayRef>(self), index, reinterpret_cast<const void*>(objAddr));
 }
 
@@ -125,6 +133,7 @@ - (void)removeAllObjects {
 
 - (void)addObject:(NSObject*)objAddr {
     BRIDGED_THROW_IF_IMMUTABLE(_CFArrayIsMutable, CFArrayRef);
+    NS_COLLECTION_THROW_IF_NULL_REASON(objAddr, [NSString stringWithFormat:@"*** %@ object cannot be nil", NSStringFromSelector(_cmd)]);
     CFArrayAppendValue((CFMutableArrayRef)self, (const void*)objAddr);
 }
 

Frameworks/Foundation/NSCFAttributedString.mm

@@ -1,6 +1,6 @@
 //******************************************************************************
 //
-// Copyright (c) 2016 Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft. All rights reserved.
 //
 // This code is licensed under the MIT License (MIT).
 //
@@ -19,6 +19,7 @@
 #import <Foundation/NSMutableAttributedString.h>
 #import "NSCFAttributedString.h"
 #import "CFFoundationInternal.h"
+#import "NSCFCollectionSupport.h"
 
 #import <algorithm>
 
@@ -35,8 +36,10 @@ - (_Nullable instancetype)init {
 }
 
 - (_Nullable instancetype)initWithAttributedString:(NSAttributedString*)string {
-    return reinterpret_cast<NSAttributedStringPrototype*>(static_cast<NSAttributedString*>(
-        CFAttributedStringCreateWithSubstring(kCFAllocatorDefault, static_cast<CFAttributedStringRef>(string), CFRange{ 0, [string length] })));
+    return reinterpret_cast<NSAttributedStringPrototype*>(
+        static_cast<NSAttributedString*>(CFAttributedStringCreateWithSubstring(kCFAllocatorDefault,
+                                                                               static_cast<CFAttributedStringRef>(string),
+                                                                               CFRange{ 0, [string length] })));
 }
 
 - (_Nullable instancetype)initWithString:(NSString*)string attributes:(NSDictionary*)attributes {
@@ -58,11 +61,13 @@ - (_Nullable instancetype)init {
 }
 
 - (_Nullable instancetype)initWithAttributedString:(NSAttributedString*)string {
-    return reinterpret_cast<NSMutableAttributedStringPrototype*>(CFAttributedStringCreateMutableCopy(kCFAllocatorDefault, 0, static_cast<CFAttributedStringRef>(string)));
+    return reinterpret_cast<NSMutableAttributedStringPrototype*>(
+        CFAttributedStringCreateMutableCopy(kCFAllocatorDefault, 0, static_cast<CFAttributedStringRef>(string)));
 }
 
 - (_Nullable instancetype)initWithString:(NSString*)string attributes:(NSDictionary*)attributes {
-    NSMutableAttributedString* attributedString = static_cast<NSMutableAttributedString*>(CFAttributedStringCreateMutable(kCFAllocatorDefault, 0));
+    NSMutableAttributedString* attributedString =
+        static_cast<NSMutableAttributedString*>(CFAttributedStringCreateMutable(kCFAllocatorDefault, 0));
     CFAttributedStringReplaceString(static_cast<CFMutableAttributedStringRef>(attributedString),
                                     CFRange{ 0, 0 },
                                     static_cast<CFStringRef>(string));
@@ -108,7 +113,7 @@ - (NSMutableString*)mutableString {
 - (void)addAttribute:(NSString*)name value:(id)value range:(NSRange)range {
     BRIDGED_THROW_IF_IMMUTABLE(_CFAttributedStringIsMutable, CFAttributedStringRef);
     THROW_NS_IF_FALSE(E_BOUNDS, ((range.location + range.length) <= [self length]));
-
+    NS_COLLECTION_THROW_IF_NULL_REASON(value, [NSString stringWithFormat:@"*** %@ nil value", NSStringFromSelector(_cmd)]);
     CFAttributedStringSetAttribute(reinterpret_cast<CFMutableAttributedStringRef>(self),
                                    *reinterpret_cast<CFRange*>(&range),
                                    (__bridge CFStringRef)name,

Frameworks/Foundation/NSCFCollectionSupport.h

@@ -1,6 +1,6 @@
 //******************************************************************************
 //
-// Copyright (c) 2016 Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft. All rights reserved.
 //
 // This code is licensed under the MIT License (MIT).
 //
@@ -25,4 +25,17 @@ const void* _NSCFCallbackRetain(CFAllocatorRef allocator, const void* value);
 void _NSCFCallbackRelease(CFAllocatorRef allocator, const void* value);
 CFStringRef _NSCFCallbackCopyDescription(const void* value);
 Boolean _NSCFCallbackEquals(const void* value1, const void* value2);
-CFHashCode _NSCFCallbackHash(const void* value);
+CFHashCode _NSCFCallbackHash(const void* value);
+
+#ifdef __OBJC__
+#include <Foundation/NSException.h>
+
+// Helper macro for NSCF collections to use CF counterparts that don't throw when trying to insert nil
+#define NS_COLLECTION_THROW_IF_NULL_REASON(VALUE, ...)                                                           \
+    do {                                                                                                         \
+        if (VALUE == nil) {                                                                                      \
+            @throw [NSException exceptionWithName:NSInvalidArgumentException reason:(__VA_ARGS__) userInfo:nil]; \
+        }                                                                                                        \
+    } while (false)
+
+#endif // __OBJC__

Frameworks/Foundation/NSCFDictionary.mm

@@ -1,6 +1,6 @@
 //******************************************************************************
 //
-// Copyright (c) 2015 Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft. All rights reserved.
 //
 // This code is licensed under the MIT License (MIT).
 //
@@ -112,6 +112,9 @@ - (NSEnumerator*)keyEnumerator {
 
 - (void)setObject:(id)object forKey:(id)key {
     BRIDGED_THROW_IF_IMMUTABLE(_CFDictionaryIsMutable, CFDictionaryRef);
+    NS_COLLECTION_THROW_IF_NULL_REASON(object,
+                                       [NSString
+                                           stringWithFormat:@"*** %@ object cannot be nil (key: %@)", NSStringFromSelector(_cmd), key]);
     CFDictionarySetValue((CFMutableDictionaryRef)self, (const void*)key, (void*)object);
 }
 

Frameworks/Foundation/NSCFSet.mm

@@ -1,6 +1,6 @@
 //******************************************************************************
 //
-// Copyright (c) 2016 Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft. All rights reserved.
 //
 // This code is licensed under the MIT License (MIT).
 //
@@ -24,12 +24,7 @@
 #include <vector>
 
 static CFSetCallBacks _NSCFSetCallBacks = {
-    0,
-    _NSCFCallbackRetain,
-    _NSCFCallbackRelease,
-    _NSCFCallbackCopyDescription,
-    _NSCFCallbackEquals,
-    _NSCFCallbackHash,
+    0, _NSCFCallbackRetain, _NSCFCallbackRelease, _NSCFCallbackCopyDescription, _NSCFCallbackEquals, _NSCFCallbackHash,
 };
 
 @interface NSCFSet : NSMutableSet
@@ -45,7 +40,8 @@ - (_Nullable instancetype)init {
 }
 
 - (_Nullable instancetype)initWithObjects:(id _Nonnull const*)objs count:(NSUInteger)count {
-    return reinterpret_cast<NSSetPrototype*>(static_cast<NSSet*>((CFSetCreate(kCFAllocatorDefault, (const void**)(objs), count, &_NSCFSetCallBacks))));
+    return reinterpret_cast<NSSetPrototype*>(
+        static_cast<NSSet*>((CFSetCreate(kCFAllocatorDefault, (const void**)(objs), count, &_NSCFSetCallBacks))));
 }
 
 @end
@@ -111,6 +107,7 @@ - (id)member:(id)object {
 
 - (void)addObject:(id)object {
     BRIDGED_THROW_IF_IMMUTABLE(_CFSetIsMutable, CFSetRef);
+    NS_COLLECTION_THROW_IF_NULL_REASON(object, [NSString stringWithFormat:@"*** %@ object cannot be nil", NSStringFromSelector(_cmd)]);
     CFSetAddValue(static_cast<CFMutableSetRef>(self), object);
 }
 

Frameworks/Foundation/NSMutableDictionary.mm

@@ -115,7 +115,11 @@ - (void)setObject:(id)object forKey:(id)key {
  @Status Interoperable
 */
 - (void)setObject:(id)object forKeyedSubscript:(id)key {
-    [self setObject:object forKey:key];
+    if (object == nil) {
+        [self removeObjectForKey:key];
+    } else {
+        [self setObject:object forKey:key];
+    }
 }
 
 /**

tests/unittests/Foundation/NSArrayTests.mm

@@ -1,6 +1,6 @@
 //******************************************************************************
 //
-// Copyright (c) 2015 Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft. All rights reserved.
 //
 // This code is licensed under the MIT License (MIT).
 //
@@ -425,3 +425,13 @@ static unsigned long int objectIndexInArray(NSArray* array, int value, int start
 
     EXPECT_OBJCNE(input, output);
 }
+
+TEST(NSMutableArray, InsertingNilShouldThrow) {
+    NSMutableArray* arr = [NSMutableArray arrayWithObject:@"hello"];
+    EXPECT_ANY_THROW(arr[0] = nil);
+    EXPECT_ANY_THROW([arr insertObject:nil atIndex:1]);
+    EXPECT_ANY_THROW([arr addObject:nil]);
+    EXPECT_ANY_THROW([arr replaceObjectAtIndex:0 withObject:nil]);
+    EXPECT_OBJCEQ(@"hello", arr[0]);
+    EXPECT_EQ(1, [arr count]);
+}

tests/unittests/Foundation/NSAttributedStringTests.mm

@@ -1,6 +1,6 @@
 //******************************************************************************
 //
-// Copyright (c) 2015 Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft. All rights reserved.
 //
 // This code is licensed under the MIT License (MIT).
 //
@@ -788,3 +788,8 @@ - (NSUInteger)length {
 
     EXPECT_OBJCNE(input, output);
 }
+
+TEST(NSMutableAttributedString, InsertingNilShouldThrow) {
+    NSMutableAttributedString* string = [[[NSMutableAttributedString alloc] initWithString:@"hello"] autorelease];
+    EXPECT_ANY_THROW([string addAttribute:@"attribute" value:nil range:NSMakeRange(0, 3)]);
+}

tests/unittests/Foundation/NSDictionaryTests.mm

@@ -1,6 +1,6 @@
 //******************************************************************************
 //
-// Copyright (c) 2015 Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft. All rights reserved.
 //
 // This code is licensed under the MIT License (MIT).
 //
@@ -50,11 +50,11 @@
 
 TEST(NSDictionary, KeysSortedByValueUsingComparator) {
     NSDictionary* testDict = @{ @"A" : @2, @"B" : @4, @"C" : @3, @"D" : @1 };
-    NSArray* actualArray = [testDict keysSortedByValueUsingComparator: ^(id obj1, id obj2) {
+    NSArray* actualArray = [testDict keysSortedByValueUsingComparator:^(id obj1, id obj2) {
         return [obj1 compare:obj2];
     }];
 
-    NSArray* expectedArray = @[@"D", @"A", @"C", @"B" ];
+    NSArray* expectedArray = @[ @"D", @"A", @"C", @"B" ];
     ASSERT_OBJCEQ(expectedArray, actualArray);
 }
 
@@ -70,8 +70,8 @@
 }
 
 TEST(NSDictionary, KeysSortedByValue) {
-    NSDictionary* dictionary = @{@"f" : @6, @"b": @2, @"a" : @1, @"c" : @3, @"e" : @5, @"d" : @4};
-    NSArray* expected = @[@"a", @"b", @"c", @"d", @"e", @"f"];
+    NSDictionary* dictionary = @{ @"f" : @6, @"b" : @2, @"a" : @1, @"c" : @3, @"e" : @5, @"d" : @4 };
+    NSArray* expected = @[ @"a", @"b", @"c", @"d", @"e", @"f" ];
     NSArray* actual = [dictionary keysSortedByValueUsingComparator:^NSComparisonResult(id obj1, id obj2) {
         int a = [obj1 intValue];
         int b = [obj2 intValue];
@@ -86,33 +86,42 @@
 }
 
 TEST(NSDictionary, KeysSortedByValueWithOptions) {
-    NSDictionary* dictionary = @{@"f" : @6, @"b": @2, @"a" : @1, @"c" : @3, @"e" : @5, @"d" : @4};
-    NSArray* expected = @[@"a", @"b", @"c", @"d", @"e", @"f"];
-    NSArray* actual = [dictionary keysSortedByValueWithOptions:0 usingComparator:^NSComparisonResult(id obj1, id obj2) {
-        int a = [obj1 intValue];
-        int b = [obj2 intValue];
-        if (a == b) {
-            return NSOrderedSame;
-        }
-
-        return (a > b) ? NSOrderedDescending : NSOrderedAscending;
-    }];
+    NSDictionary* dictionary = @{ @"f" : @6, @"b" : @2, @"a" : @1, @"c" : @3, @"e" : @5, @"d" : @4 };
+    NSArray* expected = @[ @"a", @"b", @"c", @"d", @"e", @"f" ];
+    NSArray* actual = [dictionary keysSortedByValueWithOptions:0
+                                               usingComparator:^NSComparisonResult(id obj1, id obj2) {
+                                                   int a = [obj1 intValue];
+                                                   int b = [obj2 intValue];
+                                                   if (a == b) {
+                                                       return NSOrderedSame;
+                                                   }
+
+                                                   return (a > b) ? NSOrderedDescending : NSOrderedAscending;
+                                               }];
 
     ASSERT_OBJCEQ(expected, actual);
 }
 
 TEST(NSDictionary, KeysSortedByValueWithOptions_Stable) {
-    NSDictionary* dictionary = @{@"a" : @1, @"b": @1, @"c" : @1, @"d" : @1, @"e" : @1, @"f" : @0};
-    NSArray* expected = @[@"f", @"d", @"b", @"e", @"c", @"a"]; // Note: ordering after "f" is dependent on CFDictionary (this ordering matches the reference platform)
-    NSArray* actual = [dictionary keysSortedByValueWithOptions:NSSortStable usingComparator:^NSComparisonResult(id obj1, id obj2) {
-        int a = [obj1 intValue];
-        int b = [obj2 intValue];
-        if (a == b) {
-            return NSOrderedSame;
-        }
-
-        return (a > b) ? NSOrderedDescending : NSOrderedAscending;
-    }];
+    NSDictionary* dictionary = @{ @"a" : @1, @"b" : @1, @"c" : @1, @"d" : @1, @"e" : @1, @"f" : @0 };
+    NSArray* expected = @[
+        @"f",
+        @"d",
+        @"b",
+        @"e",
+        @"c",
+        @"a"
+    ]; // Note: ordering after "f" is dependent on CFDictionary (this ordering matches the reference platform)
+    NSArray* actual = [dictionary keysSortedByValueWithOptions:NSSortStable
+                                               usingComparator:^NSComparisonResult(id obj1, id obj2) {
+                                                   int a = [obj1 intValue];
+                                                   int b = [obj2 intValue];
+                                                   if (a == b) {
+                                                       return NSOrderedSame;
+                                                   }
+
+                                                   return (a > b) ? NSOrderedDescending : NSOrderedAscending;
+                                               }];
 
     ASSERT_OBJCEQ(expected, actual);
 }
@@ -130,3 +139,23 @@
 
     EXPECT_OBJCNE(input, output);
 }
+
+TEST(NSMutableDictionary, InsertingNilForKeyedSubscriptShouldRemoveValue) {
+    NSMutableDictionary* dict = [NSMutableDictionary dictionaryWithObject:@"world" forKey:@"hello"];
+    [dict setObject:nil forKeyedSubscript:@"hello"];
+    EXPECT_EQ(nil, dict[@"hello"]);
+    EXPECT_EQ(0, [dict count]);
+}
+
+TEST(NSMutableDictionary, InsertingNilWithSubscriptingShouldRemoveValue) {
+    NSMutableDictionary* dict = [NSMutableDictionary dictionaryWithObject:@"world" forKey:@"hello"];
+    dict[@"hello"] = nil;
+    EXPECT_EQ(nil, dict[@"hello"]);
+    EXPECT_EQ(0, [dict count]);
+}
+
+TEST(NSMutableDictionary, SetObjectWithNilShouldThrow) {
+    NSMutableDictionary* dict = [NSMutableDictionary dictionaryWithObject:@"world" forKey:@"hello"];
+    EXPECT_ANY_THROW([dict setObject:nil forKey:@"fail"]);
+    EXPECT_OBJCEQ(@"world", dict[@"hello"]);
+}

tests/unittests/Foundation/NSSetTests.mm

@@ -1,6 +1,6 @@
 //******************************************************************************
 //
-// Copyright (c) 2015 Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft. All rights reserved.
 //
 // This code is licensed under the MIT License (MIT).
 //
@@ -143,3 +143,9 @@
 
     EXPECT_OBJCNE(input, output);
 }
+
+TEST(NSMutableSet, ShouldThrowWhenTryingToInsertNil) {
+    NSMutableSet* set = [NSMutableSet set];
+    EXPECT_ANY_THROW([set addObject:nil]);
+    EXPECT_EQ(0, [set count]);
+}