Fix heap corruption crash from concurrent array mutation during diff (#1578)

Summary:
Fixes https://github.com/Instagram/IGListKit/issues/1578

Callers of `IGListDiff`/`IGListDiffPaths` may pass `NSMutableArray` instances backed by collections that are mutated on other threads. Because the diffing algorithm uses `__unsafe_unretained` pointers internally for performance, concurrent mutation can cause use-after-free heap corruption — typically manifesting as:

```
malloc: Incorrect checksum for freed object: probably modified after being freed.
```

inside `std::deque::push_back` during the entry `oldIndexes` stack growth.

This change adds `[oldArray copy]` and `[newArray copy]` at the top of `IGListDiffing()`. For immutable `NSArray` inputs this is a no-op retain with zero overhead. For `NSMutableArray` inputs it creates an immutable snapshot, narrowing the race window from the entire O(n+m) diff to just the `-copy` call.

This is a best-effort mitigation — callers are still responsible for not mutating the source array concurrently since `-[NSMutableArray copy]` itself is not atomic.

Differential Revision: D101205956

fbshipit-source-id: 514ebbef1903c796c10fee23f4efb4dd9b3073bd

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
 -----
 

Source/IGListDiffKit/IGListDiff.mm

@@ -97,6 +97,13 @@ static id IGListDiffing(BOOL returnIndexPaths,
                         NSArray<id<IGListDiffable>> *oldArray,
                         NSArray<id<IGListDiffable>> *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;
 

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 <Foundation/Foundation.h>
+#import <XCTest/XCTest.h>
+
+#import <IGListDiffKit/IGListDiff.h>
+
+#import "IGTestObject.h"
+
+static NSArray<IGTestObject *> *generateArray(NSInteger count) {
+    NSMutableArray<IGTestObject *> *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<IGTestObject *> *old = [NSMutableArray arrayWithCapacity:size];
+        NSMutableArray<IGTestObject *> *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<NSNumber *, IGTestObject *> *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<IGTestObject *> *messageModels = generateArray(size);
+        NSArray<IGTestObject *> *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<IGTestObject *> *mutableOld = [NSMutableArray arrayWithArray:generateArray(size)];
+        NSMutableArray<IGTestObject *> *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