Fix test

Author: noahsmartin

Sources/SentryCrash/Recording/SentryCrashBinaryImageCache.c

@@ -165,6 +165,10 @@ dyld_tracker_start(void)
 void
 sentrycrashbic_iterateOverImages(sentrycrashbic_imageIteratorCallback callback, void *context)
 {
+    if (!atomic_load_explicit(&g_started, memory_order_acquire)) {
+        return;
+    }
+
     uint32_t count = atomic_load_explicit(&g_next_index, memory_order_acquire);
 
     if (count > MAX_DYLD_IMAGES)
@@ -212,18 +216,14 @@ sentrycrashbic_addDyldNode(void)
     }
 
     PublishedBinaryImage *entry = &g_images[idx];
-    if (!sentrycrashdl_getBinaryImageForHeader((const void *)header, "dyld", &entry->image, false)) {
+    if (!sentrycrashdl_getBinaryImageForHeader(
+            (const void *)header, "dyld", &entry->image, false)) {
         // Decrement because we couldn't add the image
         atomic_fetch_sub_explicit(&g_next_index, 1, memory_order_relaxed);
         return;
     }
 
-    sentrycrashbic_cacheChangeCallback callback
-        = atomic_load_explicit(&g_addedCallback, memory_order_acquire);
     atomic_store_explicit(&entry->ready, 1, memory_order_release);
-    if (callback != NULL) {
-        callback(&entry->image);
-    }
 }
 
 static void
@@ -236,6 +236,11 @@ sentrycrashbic_startCacheImpl(void)
         return;
     }
 
+    // Reset g_next_index here rather than in stopCache to avoid the race where
+    // a concurrent add_dyld_image increments g_next_index after stopCache resets it.
+    // The compare-exchange above guarantees we are the only thread in this function.
+    atomic_store_explicit(&g_next_index, 0, memory_order_release);
+
     if (sentrycrashbic_shouldAddDyld()) {
         sentrycrashdl_initialize();
         sentrycrashbic_addDyldNode();
@@ -266,11 +271,13 @@ sentrycrashbic_startCache(void)
 void
 sentrycrashbic_stopCache(void)
 {
-    // The binary image cache does not allocate memory, the cache takes a fixed
-    // size, it's not malloced. So this is kind of pointless, but there are a bunch
-    // of tests using it. We should clean it up and make startCache use dispatchOnce
+    // Only flip the started flag. We intentionally do NOT reset g_next_index here
+    // because a concurrent add_dyld_image (that already passed the g_started check)
+    // could increment g_next_index after our reset, leaving the cache in an
+    // inconsistent state. Instead, g_next_index is reset in startCacheImpl where
+    // we have exclusive access via the compare-exchange. iterateOverImages checks
+    // g_started before reading g_next_index, so the cache appears empty immediately.
     atomic_store_explicit(&g_started, 0, memory_order_release);
-    atomic_store_explicit(&g_next_index, 0, memory_order_release);
 }
 
 void

Tests/SentryTests/SentryCrash/SentryCrashBinaryImageCacheTests.m

@@ -59,6 +59,17 @@
     [array addObject:[NSValue valueWithPointer:image]];
 }
 
+static void
+addBinaryImageNameToArray(SentryCrashBinaryImage *image, void *context)
+{
+    NSMutableArray *array = (__bridge NSMutableArray *)context;
+    if (image->name) {
+        [array addObject:[NSString stringWithUTF8String:image->name]];
+    } else {
+        [array addObject:@"<null>"];
+    }
+}
+
 dispatch_semaphore_t delaySemaphore = NULL;
 dispatch_semaphore_t delayCalled = NULL;
 static void
@@ -289,10 +300,10 @@ - (void)testSentryBinaryImageCacheIntegration
     // by calling start, SentryBinaryImageCache will register a callback with
     // `SentryCrashBinaryImageCache` that should be called for every image already cached.
     NSMutableArray<NSString *> *paths = [NSMutableArray new];
-    [imageCache.cache enumerateObjectsUsingBlock:^(SentryBinaryImageInfo * _Nonnull obj, NSUInteger __unused idx, BOOL * _Nonnull __unused stop) {
-        [paths addObject:obj.name];
-    }];
-    XCTAssertEqual(5, imageCache.cache.count, @"Cache should start with 5 images but contained %@", paths);
+    [imageCache.cache enumerateObjectsUsingBlock:^(SentryBinaryImageInfo *_Nonnull obj,
+        NSUInteger __unused idx, BOOL *_Nonnull __unused stop) { [paths addObject:obj.name]; }];
+    XCTAssertEqual(
+        5, imageCache.cache.count, @"Cache should start with 5 images but contained %@", paths);
 
     addBinaryImage([mach_headers_test_cache[5] pointerValue], 0);
     XCTAssertEqual(6, imageCache.cache.count);
@@ -310,11 +321,10 @@ - (void)assertBinaryImageCacheLength:(int)expected
 {
     int counter = 0;
     sentrycrashbic_iterateOverImages(countNumberOfImagesInCache, &counter);
-    NSMutableArray<NSString *> *paths = [NSMutableArray new];
-    [SentryDependencyContainer.sharedInstance.binaryImageCache.cache enumerateObjectsUsingBlock:^(SentryBinaryImageInfo * _Nonnull obj, NSUInteger __unused idx, BOOL * _Nonnull __unused stop) {
-        [paths addObject:obj.name];
-    }];
-    XCTAssertEqual(counter, expected, @"Cache should have %d images but contained %@", expected, paths);
+    NSMutableArray<NSString *> *names = [NSMutableArray new];
+    sentrycrashbic_iterateOverImages(addBinaryImageNameToArray, (__bridge void *)(names));
+    XCTAssertEqual(
+        counter, expected, @"Cache should have %d images but contained %@", expected, names);
 }
 
 - (void)assertCachedBinaryImages