diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a3276f2..639b970c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag - An infinite recursion crash when VoiceOver is enabled and `scrollViewDelegate` or `collectionViewDelegate` is set to the adapter's own `UICollectionView`. [Cameron Roth](https://github.com/camroth) [(#1658)](https://github.com/Instagram/IGListKit/issues/1658) +- A heap corruption crash in `IGListDiff`/`IGListDiffPaths` caused by concurrent mutation of `NSMutableArray` inputs during the diff. Input arrays are now defensively copied to create immutable snapshots before diffing. [Cameron Roth](https://github.com/camroth) [(#1578)](https://github.com/Instagram/IGListKit/issues/1578) + 5.2.0 ----- diff --git a/Source/IGListDiffKit/IGListDiff.mm b/Source/IGListDiffKit/IGListDiff.mm index 8c8972ee..462d34c3 100644 --- a/Source/IGListDiffKit/IGListDiff.mm +++ b/Source/IGListDiffKit/IGListDiff.mm @@ -97,6 +97,13 @@ static id IGListDiffing(BOOL returnIndexPaths, NSArray> *oldArray, NSArray> *newArray, IGListDiffOption option) { + // Best-effort snapshot: narrows the race window from the entire diff to just the copy call. + // For immutable NSArray this is a no-op retain; for NSMutableArray it creates an immutable copy. + // NOTE: Callers are still responsible for not mutating the source array concurrently, since + // -[NSMutableArray copy] itself is not atomic. + oldArray = [oldArray copy]; + newArray = [newArray copy]; + const NSInteger newCount = newArray.count; const NSInteger oldCount = oldArray.count; diff --git a/Tests/IGListDiffConcurrentMutationTests.m b/Tests/IGListDiffConcurrentMutationTests.m new file mode 100644 index 00000000..c2b31c46 --- /dev/null +++ b/Tests/IGListDiffConcurrentMutationTests.m @@ -0,0 +1,245 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#import +#import + +#import + +#import "IGTestObject.h" + +static NSArray *generateArray(NSInteger count) { + NSMutableArray *array = [NSMutableArray arrayWithCapacity:count]; + for (NSInteger i = 0; i < count; i++) { + [array addObject:genTestObject(@(i), @(i))]; + } + return [array copy]; +} + +@interface IGListDiffConcurrentMutationTests : XCTestCase +@end + +@implementation IGListDiffConcurrentMutationTests + +#pragma mark - Mutable Array Input (Sanity) + +- (void)test_whenDiffingMutableArrays_thatResultIsCorrect { + NSMutableArray *o = [NSMutableArray arrayWithArray:@[genTestObject(@0, @0), genTestObject(@1, @1)]]; + NSMutableArray *n = [NSMutableArray arrayWithArray:@[genTestObject(@0, @0), genTestObject(@1, @2)]]; + IGListIndexSetResult *result = IGListDiff(o, n, IGListDiffEquality); + XCTAssertTrue([result hasChanges]); + XCTAssertTrue([result.updates containsIndex:1]); +} + +- (void)test_whenDiffingMutableArrayPaths_thatResultIsCorrect { + NSMutableArray *o = [NSMutableArray arrayWithArray:@[genTestObject(@0, @0), genTestObject(@1, @1)]]; + NSMutableArray *n = [NSMutableArray arrayWithArray:@[genTestObject(@0, @0), genTestObject(@1, @2)]]; + IGListIndexPathResult *result = IGListDiffPaths(0, 0, o, n, IGListDiffEquality); + XCTAssertTrue([result hasChanges]); +} + +#pragma mark - Snapshot Isolation + +- (void)test_whenMutatingOldArrayAfterDiff_thatResultIsBasedOnSnapshot { + NSMutableArray *o = [NSMutableArray arrayWithArray:@[genTestObject(@0, @0), genTestObject(@1, @1)]]; + NSArray *n = @[genTestObject(@0, @0)]; + + IGListIndexSetResult *result = IGListDiff(o, n, IGListDiffEquality); + + // Mutate old array after diff — result should still reflect the original + [o removeAllObjects]; + + XCTAssertEqual(result.deletes.count, 1u); + XCTAssertTrue([result.deletes containsIndex:1]); +} + +- (void)test_whenMutatingNewArrayAfterDiff_thatResultIsBasedOnSnapshot { + NSArray *o = @[genTestObject(@0, @0)]; + NSMutableArray *n = [NSMutableArray arrayWithArray:@[genTestObject(@0, @0), genTestObject(@1, @1)]]; + + IGListIndexSetResult *result = IGListDiff(o, n, IGListDiffEquality); + + [n removeAllObjects]; + + XCTAssertEqual(result.inserts.count, 1u); + XCTAssertTrue([result.inserts containsIndex:1]); +} + +#pragma mark - Post-Copy Background Mutation + +// These tests verify the core value proposition of the defensive copy: once IGListDiffing() +// snapshots the arrays, subsequent mutations on any thread cannot affect the diff result or +// cause a use-after-free inside the algorithm. +// +// We do NOT attempt to mutate the array concurrently with the -[NSArray copy] call itself, +// because -copy is not atomic and racing with it is undefined behavior at the Foundation level. +// The defensive copy narrows the race window from the entire O(n+m) diff to just the copy call; +// callers remain responsible for not mutating the source array at the exact moment of the call. + +- (void)test_whenBackgroundMutatesOldArrayDuringDiff_thatResultIsStable { + const NSInteger size = 500; + + for (NSInteger iteration = 0; iteration < 100; iteration++) { + // Build old and new with known differences + NSMutableArray *old = [NSMutableArray arrayWithCapacity:size]; + NSMutableArray *new_ = [NSMutableArray arrayWithCapacity:size]; + for (NSInteger i = 0; i < size; i++) { + [old addObject:genTestObject(@(i), @(i))]; + // Shift new by 1 so there's one delete and one insert + [new_ addObject:genTestObject(@(i + 1), @(i + 1))]; + } + + // Take an immutable snapshot (simulates what the defensive copy does) + NSArray *oldSnapshot = [old copy]; + NSArray *newSnapshot = [new_ copy]; + + // Start aggressively mutating the originals on a background thread + __block BOOL done = NO; + dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ + while (!done) { + @autoreleasepool { + [old removeAllObjects]; + for (NSInteger i = 0; i < size; i++) { + [old addObject:genTestObject(@(arc4random()), @(arc4random()))]; + } + [new_ removeAllObjects]; + for (NSInteger i = 0; i < size; i++) { + [new_ addObject:genTestObject(@(arc4random()), @(arc4random()))]; + } + } + } + }); + + // Diff the snapshots — should be stable regardless of background mutation + IGListIndexSetResult *result = IGListDiff(oldSnapshot, newSnapshot, IGListDiffEquality); + + done = YES; + + // Verify the result is consistent with the known input + // old has [0..size-1], new has [1..size], so: + // - item 0 is deleted (only in old) + // - item size is inserted (only in new) + // - items 1..size-1 are unchanged + XCTAssertTrue([result.deletes containsIndex:0], @"iteration %ld", (long)iteration); + XCTAssertTrue([result.inserts containsIndex:size - 1], @"iteration %ld", (long)iteration); + XCTAssertEqual(result.deletes.count, 1u, @"iteration %ld", (long)iteration); + XCTAssertEqual(result.inserts.count, 1u, @"iteration %ld", (long)iteration); + } +} + +- (void)test_whenBackgroundMutatesDictDuringDiffPaths_thatResultIsStable { + // Mirrors the bug report pattern: + // NSArray *newModels = [_dict allValues]; + // IGListDiffPaths(1, 1, self.messageModels, newModels, IGListDiffEquality); + // + // The defensive copy ensures the diff operates on a stable snapshot even if + // the caller's backing dictionary is mutated on another thread after allValues returns. + + const NSInteger size = 200; + + for (NSInteger iteration = 0; iteration < 100; iteration++) { + NSMutableDictionary *dict = [NSMutableDictionary dictionaryWithCapacity:size]; + for (NSInteger i = 0; i < size; i++) { + dict[@(i)] = genTestObject(@(i), @(i)); + } + + // Snapshot the dictionary values (like the caller would) + NSArray *messageModels = generateArray(size); + NSArray *newModels = [dict.allValues copy]; + + // Now mutate the dictionary on a background thread + __block BOOL done = NO; + dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ + NSInteger counter = size; + while (!done) { + @autoreleasepool { + NSNumber *key = @(counter++); + dict[key] = genTestObject(key, key); + [dict removeObjectForKey:@(counter - size - 1)]; + } + } + }); + + // Diff the snapshots — dict mutation should not affect this + IGListIndexPathResult *result = IGListDiffPaths(1, 1, messageModels, newModels, IGListDiffEquality); + + done = YES; + + // Both arrays have size elements, result should be consistent + NSInteger changeCount = result.inserts.count + result.deletes.count + result.updates.count + result.moves.count; + XCTAssertGreaterThanOrEqual(changeCount, 0, @"iteration %ld", (long)iteration); + // Sanity: no out-of-bounds indices + for (NSIndexPath *path in result.inserts) { + XCTAssertLessThan(path.item, size, @"insert index out of range, iteration %ld", (long)iteration); + } + } +} + +- (void)test_whenBackgroundMutatesOriginal_thatDiffPathsOnSnapshotDoesNotCrash { + const NSInteger size = 300; + + for (NSInteger iteration = 0; iteration < 100; iteration++) { + NSMutableArray *mutableOld = [NSMutableArray arrayWithArray:generateArray(size)]; + NSMutableArray *mutableNew = [NSMutableArray arrayWithArray:generateArray(size)]; + + // Snapshot + NSArray *oldSnap = [mutableOld copy]; + NSArray *newSnap = [mutableNew copy]; + + __block BOOL done = NO; + dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ + while (!done) { + @autoreleasepool { + if (mutableOld.count > 0) [mutableOld removeLastObject]; + [mutableOld addObject:genTestObject(@(arc4random()), @(arc4random()))]; + if (mutableNew.count > 0) [mutableNew removeLastObject]; + [mutableNew addObject:genTestObject(@(arc4random()), @(arc4random()))]; + } + } + }); + + // Diff the snapshots with both index-set and index-path variants + IGListIndexSetResult *setResult = IGListDiff(oldSnap, newSnap, IGListDiffEquality); + IGListIndexPathResult *pathResult = IGListDiffPaths(0, 0, oldSnap, newSnap, IGListDiffEquality); + IGListIndexSetResult *ptrResult = IGListDiff(oldSnap, newSnap, IGListDiffPointerPersonality); + + done = YES; + + // All three results should agree on whether changes exist + XCTAssertEqual([setResult hasChanges], [pathResult hasChanges], @"iteration %ld", (long)iteration); + (void)ptrResult; + } +} + +#pragma mark - Large Array Snapshot Correctness + +- (void)test_whenDiffingLargeMutableArrays_thatInsertDeleteCountsAreConsistent { + const NSInteger oldSize = 1000; + const NSInteger newSize = 800; + + NSMutableArray *old = [NSMutableArray arrayWithCapacity:oldSize]; + for (NSInteger i = 0; i < oldSize; i++) { + [old addObject:genTestObject(@(i), @(i))]; + } + + NSMutableArray *new_ = [NSMutableArray arrayWithCapacity:newSize]; + for (NSInteger i = 200; i < oldSize; i++) { + [new_ addObject:genTestObject(@(i), @(i))]; + } + + IGListIndexSetResult *result = IGListDiff(old, new_, IGListDiffEquality); + + // old=[0..999], new=[200..999] → 200 deletes, 0 inserts + XCTAssertEqual(result.deletes.count, 200u); + XCTAssertEqual(result.inserts.count, 0u); + + // Sanity: oldCount + inserts - deletes == newCount + XCTAssertEqual((NSInteger)oldSize + (NSInteger)result.inserts.count - (NSInteger)result.deletes.count, + (NSInteger)newSize); +} + +@end