fix: Record cached dyld images async

Sources/SentryCrash/Recording/SentryCrashBinaryImageCache.c

@@ -1,8 +1,10 @@
 #include "SentryCrashBinaryImageCache.h"
 #include "SentryCrashDynamicLinker.h"
+#include <dispatch/dispatch.h>
 #include <mach-o/dyld.h>
 #include <mach-o/dyld_images.h>
 #include <pthread.h>
+#include <stdatomic.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -60,94 +62,121 @@ sentry_resetFuncForAddRemoveImage(void)
 #    define _will_add_image()
 #endif // defined(SENTRY_TEST) || defined(SENTRY_TEST_CI) || defined(DEBUG)
 
-typedef struct SentryCrashBinaryImageNode {
+#define MAX_DYLD_IMAGES 4096
+
+typedef struct {
+    _Atomic(uint32_t) ready; // 0 = not published, 1 = published
     SentryCrashBinaryImage image;
-    bool available;
-    struct SentryCrashBinaryImageNode *next;
-} SentryCrashBinaryImageNode;
+} PublishedBinaryImage;
 
-static SentryCrashBinaryImageNode rootNode = { 0 };
-static SentryCrashBinaryImageNode *tailNode = NULL;
-static pthread_mutex_t binaryImagesMutex = PTHREAD_MUTEX_INITIALIZER;
+static PublishedBinaryImage g_images[MAX_DYLD_IMAGES];
+static _Atomic(uint32_t) g_next_index = 0;
+static _Atomic(uint32_t) g_started = 0;
 
-static sentrycrashbic_cacheChangeCallback imageAddedCallback = NULL;
-static sentrycrashbic_cacheChangeCallback imageRemovedCallback = NULL;
+static _Atomic(sentrycrashbic_cacheChangeCallback) g_addedCallback = NULL;
+static _Atomic(sentrycrashbic_cacheChangeCallback) g_removedCallback = NULL;
 
 static void
-binaryImageAdded(const struct mach_header *header, intptr_t slide)
+add_dyld_image(const struct mach_header *mh)
 {
-    pthread_mutex_lock(&binaryImagesMutex);
-    if (tailNode == NULL) {
-        pthread_mutex_unlock(&binaryImagesMutex);
+    // Don't add images if the cache is not started
+    if (!atomic_load_explicit(&g_started, memory_order_acquire)) {
         return;
     }
-    pthread_mutex_unlock(&binaryImagesMutex);
+
+    // Check dladdr first, before reserving a slot in the array.
+    // If we increment g_next_index before this check and dladdr fails,
+    // we'd create a "hole" with ready=0 that stops iteration.
     Dl_info info;
-    if (!dladdr(header, &info) || info.dli_fname == NULL) {
+    if (!dladdr(mh, &info) || info.dli_fname == NULL) {
         return;
     }
 
-    SentryCrashBinaryImage binaryImage = { 0 };
-    if (!sentrycrashdl_getBinaryImageForHeader(
-            (const void *)header, info.dli_fname, &binaryImage, false)) {
+    // Test hook: called just before adding the image
+    _will_add_image();
+
+    uint32_t idx = atomic_fetch_add_explicit(&g_next_index, 1, memory_order_relaxed);
+
+    if (idx >= MAX_DYLD_IMAGES) {
         return;
     }
 
-    SentryCrashBinaryImageNode *newNode = malloc(sizeof(SentryCrashBinaryImageNode));
-    newNode->available = true;
-    newNode->image = binaryImage;
-    newNode->next = NULL;
-    _will_add_image();
-    pthread_mutex_lock(&binaryImagesMutex);
-    // Recheck tailNode as it could be null when
-    // stopped from another thread.
-    if (tailNode != NULL) {
-        tailNode->next = newNode;
-        tailNode = tailNode->next;
-    } else {
-        free(newNode);
-        newNode = NULL;
-    }
-    pthread_mutex_unlock(&binaryImagesMutex);
-    if (newNode && imageAddedCallback) {
-        imageAddedCallback(&newNode->image);
+    PublishedBinaryImage *entry = &g_images[idx];
+    sentrycrashdl_getBinaryImageForHeader(mh, info.dli_fname, &entry->image, false);
+
+    // Read callback BEFORE publishing to avoid race with registerAddedCallback.
+    // If callback is NULL here, the registering thread will see ready=1 and call it.
+    // If callback is non-NULL here, we call it and the registering thread will either
+    // not have started iterating yet, or will skip this image since it wasn't ready
+    // when it read g_next_index.
+    sentrycrashbic_cacheChangeCallback callback
+        = atomic_load_explicit(&g_addedCallback, memory_order_acquire);
+
+    // ---- Publish ----
+    atomic_store_explicit(&entry->ready, 1, memory_order_release);
+
+    if (callback != NULL) {
+        callback(&entry->image);
     }
 }
 
 static void
-binaryImageRemoved(const struct mach_header *header, intptr_t slide)
+dyld_add_image_cb(const struct mach_header *mh, intptr_t slide)
 {
-    SentryCrashBinaryImageNode *nextNode = &rootNode;
+    add_dyld_image(mh);
+}
 
-    while (nextNode != NULL) {
-        if (nextNode->image.address == (uint64_t)header) {
-            nextNode->available = false;
-            if (imageRemovedCallback) {
-                imageRemovedCallback(&nextNode->image);
+static void
+dyld_remove_image_cb(const struct mach_header *mh, intptr_t slide)
+{
+    sentrycrashbic_cacheChangeCallback callback
+        = atomic_load_explicit(&g_removedCallback, memory_order_acquire);
+
+    // Find the image in our cache by matching the header address
+    uint32_t count = atomic_load_explicit(&g_next_index, memory_order_acquire);
+    if (count > MAX_DYLD_IMAGES)
+        count = MAX_DYLD_IMAGES;
+
+    for (uint32_t i = 0; i < count; i++) {
+        PublishedBinaryImage *src = &g_images[i];
+        if (!atomic_load_explicit(&src->ready, memory_order_acquire)) {
+            continue;
+        }
+        if (src->image.address == (uintptr_t)mh) {
+            atomic_store_explicit(&src->ready, 0, memory_order_release);
+            if (callback) {
+                callback(&src->image);
+                return;
             }
-            break;
         }
-        nextNode = nextNode->next;
     }
 }
 
+static void
+dyld_tracker_start(void)
+{
+    sentry_dyld_register_func_for_add_image(dyld_add_image_cb);
+    sentry_dyld_register_func_for_remove_image(dyld_remove_image_cb);
+}
+
 void
 sentrycrashbic_iterateOverImages(sentrycrashbic_imageIteratorCallback callback, void *context)
 {
-    /**
-     We can't use locks here because this is meant to be used during crashes,
-     where we can't use async unsafe functions. In order to avoid potential problems,
-     we choose an approach that doesn't remove nodes from the list.
-    */
-    SentryCrashBinaryImageNode *nextNode = &rootNode;
-
-    // If tailNode is null it means the cache was stopped, therefore we end the iteration.
-    // This will minimize any race condition effect without the need for locks.
-    while (nextNode != NULL && tailNode != NULL) {
-        if (nextNode->available) {
-            callback(&nextNode->image, 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)
+        count = MAX_DYLD_IMAGES;
+
+    for (uint32_t i = 0; i < count; i++) {
+        PublishedBinaryImage *src = &g_images[i];
+
+        if (atomic_load_explicit(&src->ready, memory_order_acquire)) {
+            callback(&src->image, context);
         }
-        nextNode = nextNode->next;
     }
 }
 
@@ -159,7 +188,7 @@ sentrycrashbic_iterateOverImages(sentrycrashbic_imageIteratorCallback callback,
  * @return true if dyld is not found in the loaded images and should be added to the cache,
  *         false if dyld is already present in the loaded images.
  */
-bool
+static bool
 sentrycrashbic_shouldAddDyld(void)
 {
     // dyld is different from libdyld.dylib; the latter contains the public API
@@ -170,93 +199,106 @@ sentrycrashbic_shouldAddDyld(void)
 
 // Since Apple no longer includes dyld in the images listed `_dyld_image_count` and related
 // functions We manually include it to our cache.
-SentryCrashBinaryImageNode *
-sentrycrashbic_getDyldNode(void)
+// Note: This bypasses add_dyld_image() because dladdr() returns NULL for dyld, so we need
+// to use sentrycrashdl_getBinaryImageForHeader() directly with a hardcoded filename.
+static void
+sentrycrashbic_addDyldNode(void)
 {
     const struct mach_header *header = sentryDyldHeader;
 
-    SentryCrashBinaryImage binaryImage = { 0 };
-    if (!sentrycrashdl_getBinaryImageForHeader((const void *)header, "dyld", &binaryImage, false)) {
-        return NULL;
+    uint32_t idx = atomic_fetch_add_explicit(&g_next_index, 1, memory_order_relaxed);
+    if (idx >= MAX_DYLD_IMAGES) {
+        return;
     }
 
-    SentryCrashBinaryImageNode *newNode = malloc(sizeof(SentryCrashBinaryImageNode));
-    newNode->available = true;
-    newNode->image = binaryImage;
-    newNode->next = NULL;
+    PublishedBinaryImage *entry = &g_images[idx];
+    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;
+    }
 
-    return newNode;
+    atomic_store_explicit(&entry->ready, 1, memory_order_release);
 }
 
-void
-sentrycrashbic_startCache(void)
+static void
+sentrycrashbic_startCacheImpl(void)
 {
-    pthread_mutex_lock(&binaryImagesMutex);
-    if (tailNode != NULL) {
-        // Already initialized
-        pthread_mutex_unlock(&binaryImagesMutex);
+    // Check if already started
+    uint32_t expected = 0;
+    if (!atomic_compare_exchange_strong_explicit(
+            &g_started, &expected, 1, memory_order_acq_rel, memory_order_relaxed)) {
         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();
-        SentryCrashBinaryImageNode *dyldNode = sentrycrashbic_getDyldNode();
-        tailNode = dyldNode;
-        rootNode.next = dyldNode;
-    } else {
-        tailNode = &rootNode;
-        rootNode.next = NULL;
+        sentrycrashbic_addDyldNode();
     }
-    pthread_mutex_unlock(&binaryImagesMutex);
 
-    // During a call to _dyld_register_func_for_add_image() the callback func is called for every
-    // existing image
-    sentry_dyld_register_func_for_add_image(&binaryImageAdded);
-    sentry_dyld_register_func_for_remove_image(&binaryImageRemoved);
+    dyld_tracker_start();
 }
 
 void
-sentrycrashbic_stopCache(void)
+sentrycrashbic_startCache(void)
 {
-    pthread_mutex_lock(&binaryImagesMutex);
-    if (tailNode == NULL) {
-        pthread_mutex_unlock(&binaryImagesMutex);
-        return;
-    }
-
-    SentryCrashBinaryImageNode *node = rootNode.next;
-    rootNode.next = NULL;
-    tailNode = NULL;
-
-    while (node != NULL) {
-        SentryCrashBinaryImageNode *nextNode = node->next;
-        free(node);
-        node = nextNode;
-    }
-
-    pthread_mutex_unlock(&binaryImagesMutex);
+    // During a call to _dyld_register_func_for_add_image() the callback func is called for every
+    // existing image
+    // This must be done on a background thread to not block app launch due to the extensive use of
+    // locks in the image added callback. The main culprit is the calls to `dladdr`. The downside of
+    // doing this async is if there is a crash very shortly after app launch we might not have
+    // recorded all the load addresses of images yet. We think this is an acceptible tradeoff to not
+    // block app launch, since it's always possible to crash early in app launch before Sentry can
+    // capture the crash.
+#if defined(SENTRY_TEST) || defined(SENTRY_TEST_CI)
+    sentrycrashbic_startCacheImpl();
+#else
+    dispatch_async(
+        dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{ sentrycrashbic_startCacheImpl(); });
+#endif
 }
 
-static void
-initialReportToCallback(SentryCrashBinaryImage *image, void *context)
+void
+sentrycrashbic_stopCache(void)
 {
-    sentrycrashbic_cacheChangeCallback callback = (sentrycrashbic_cacheChangeCallback)context;
-    callback(image);
+    // 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);
 }
 
 void
 sentrycrashbic_registerAddedCallback(sentrycrashbic_cacheChangeCallback callback)
 {
-    imageAddedCallback = callback;
-    if (callback) {
-        pthread_mutex_lock(&binaryImagesMutex);
-        sentrycrashbic_iterateOverImages(&initialReportToCallback, callback);
-        pthread_mutex_unlock(&binaryImagesMutex);
+    atomic_store_explicit(&g_addedCallback, callback, memory_order_release);
+
+    if (callback != NULL && atomic_load_explicit(&g_started, memory_order_acquire)) {
+        // Call for all existing images already in the cache
+        uint32_t count = atomic_load_explicit(&g_next_index, memory_order_acquire);
+        if (count > MAX_DYLD_IMAGES)
+            count = MAX_DYLD_IMAGES;
+
+        for (uint32_t i = 0; i < count; i++) {
+            PublishedBinaryImage *src = &g_images[i];
+            if (!atomic_load_explicit(&src->ready, memory_order_acquire)) {
+                break;
+            }
+            callback(&src->image);
+        }
     }
 }
 
 void
 sentrycrashbic_registerRemovedCallback(sentrycrashbic_cacheChangeCallback callback)
 {
-    imageRemovedCallback = callback;
+    atomic_store_explicit(&g_removedCallback, callback, memory_order_release);
 }