fix: Record cached dyld images async

Author: noahsmartin

Sources/SentryCrash/Recording/SentryCrashBinaryImageCache.c

@@ -4,6 +4,7 @@
 #include <mach-o/dyld_images.h>
 #include <pthread.h>
 #include <stdio.h>
+#include <dispatch/dispatch.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
@@ -60,94 +61,64 @@ sentry_resetFuncForAddRemoveImage(void)
 #    define _will_add_image()
 #endif // defined(SENTRY_TEST) || defined(SENTRY_TEST_CI) || defined(DEBUG)
 
-typedef struct SentryCrashBinaryImageNode {
-    SentryCrashBinaryImage image;
-    bool available;
-    struct SentryCrashBinaryImageNode *next;
-} SentryCrashBinaryImageNode;
+#include <stdatomic.h>
 
-static SentryCrashBinaryImageNode rootNode = { 0 };
-static SentryCrashBinaryImageNode *tailNode = NULL;
-static pthread_mutex_t binaryImagesMutex = PTHREAD_MUTEX_INITIALIZER;
+#define MAX_DYLD_IMAGES 4096
 
-static sentrycrashbic_cacheChangeCallback imageAddedCallback = NULL;
-static sentrycrashbic_cacheChangeCallback imageRemovedCallback = NULL;
+typedef struct {
+  _Atomic(uint32_t) ready;        // 0 = not published, 1 = published
+  SentryCrashBinaryImage image;
+} PublishedBinaryImage;
 
-static void
-binaryImageAdded(const struct mach_header *header, intptr_t slide)
-{
-    pthread_mutex_lock(&binaryImagesMutex);
-    if (tailNode == NULL) {
-        pthread_mutex_unlock(&binaryImagesMutex);
+static PublishedBinaryImage g_images[MAX_DYLD_IMAGES];
+static _Atomic(uint32_t) g_next_index = 0;
+static _Atomic(uint32_t) g_overflowed = 0;
+
+static void add_dyld_image(const struct mach_header* mh) {
+    uint32_t idx =
+        atomic_fetch_add_explicit(&g_next_index, 1, memory_order_relaxed);
+
+    if (idx >= MAX_DYLD_IMAGES) {
+        atomic_store_explicit(&g_overflowed, 1, memory_order_relaxed);
         return;
     }
-    pthread_mutex_unlock(&binaryImagesMutex);
+    
     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)) {
-        return;
-    }
+    PublishedBinaryImage* entry = &g_images[idx];
+    sentrycrashdl_getBinaryImageForHeader(mh, info.dli_fname, &entry->image, false);
 
-    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);
-    }
+    // ---- Publish ----
+    atomic_store_explicit(&entry->ready, 1, memory_order_release);
 }
 
-static void
-binaryImageRemoved(const struct mach_header *header, intptr_t slide)
-{
-    SentryCrashBinaryImageNode *nextNode = &rootNode;
-
-    while (nextNode != NULL) {
-        if (nextNode->image.address == (uint64_t)header) {
-            nextNode->available = false;
-            if (imageRemovedCallback) {
-                imageRemovedCallback(&nextNode->image);
-            }
-            break;
-        }
-        nextNode = nextNode->next;
-    }
+static void dyld_add_image_cb(const struct mach_header* mh, intptr_t slide) {
+    add_dyld_image(mh);
+}
+
+void dyld_tracker_start(void) {
+  sentry_dyld_register_func_for_add_image(dyld_add_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);
+    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)) {
+            return; // stop at first unpublished entry
         }
-        nextNode = nextNode->next;
+
+        callback(&src->image, context);
     }
 }
 
@@ -170,93 +141,57 @@ 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)
+void
+sentrycrashbic_addDyldNode(void)
 {
     const struct mach_header *header = sentryDyldHeader;
 
     SentryCrashBinaryImage binaryImage = { 0 };
     if (!sentrycrashdl_getBinaryImageForHeader((const void *)header, "dyld", &binaryImage, false)) {
-        return NULL;
+        return;
     }
-
-    SentryCrashBinaryImageNode *newNode = malloc(sizeof(SentryCrashBinaryImageNode));
-    newNode->available = true;
-    newNode->image = binaryImage;
-    newNode->next = NULL;
-
-    return newNode;
+    add_dyld_image(header);
 }
 
 void
 sentrycrashbic_startCache(void)
 {
-    pthread_mutex_lock(&binaryImagesMutex);
-    if (tailNode != NULL) {
-        // Already initialized
-        pthread_mutex_unlock(&binaryImagesMutex);
-        return;
-    }
-
     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);
+    // 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.
+    //
+    // A future update to this code could record everything but the filename synchronously, and then
+    // add in the image name async. However, that would still block app launch somewhat and it's not
+    // clear that is worth the benefits.
+    dispatch_async(dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{
+        dyld_tracker_start();
+    });
 }
 
 void
 sentrycrashbic_stopCache(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);
-}
-
-static void
-initialReportToCallback(SentryCrashBinaryImage *image, void *context)
-{
-    sentrycrashbic_cacheChangeCallback callback = (sentrycrashbic_cacheChangeCallback)context;
-    callback(image);
+    // TODO: Why do we need to support stopping it?
 }
 
 void
 sentrycrashbic_registerAddedCallback(sentrycrashbic_cacheChangeCallback callback)
 {
-    imageAddedCallback = callback;
-    if (callback) {
-        pthread_mutex_lock(&binaryImagesMutex);
-        sentrycrashbic_iterateOverImages(&initialReportToCallback, callback);
-        pthread_mutex_unlock(&binaryImagesMutex);
-    }
+    // TODO: I think this is only for when we used to have on-device symbolication. Should be able to remove now
 }
 
 void
 sentrycrashbic_registerRemovedCallback(sentrycrashbic_cacheChangeCallback callback)
 {
-    imageRemovedCallback = callback;
+    // TODO: I think this is only for when we used to have on-device symbolication. Should be able to remove now
 }