WIP

Author: noahsmartin

Sources/SentryCrash/Recording/SentryCrashBinaryImageCache.c

@@ -1,10 +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 <stdio.h>
-#include <dispatch/dispatch.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
@@ -66,59 +66,116 @@ sentry_resetFuncForAddRemoveImage(void)
 #define MAX_DYLD_IMAGES 4096
 
 typedef struct {
-  _Atomic(uint32_t) ready;        // 0 = not published, 1 = published
-  SentryCrashBinaryImage image;
+    _Atomic(uint32_t) ready; // 0 = not published, 1 = published
+    SentryCrashBinaryImage image;
 } PublishedBinaryImage;
 
 static PublishedBinaryImage g_images[MAX_DYLD_IMAGES];
 static _Atomic(uint32_t) g_next_index = 0;
 static _Atomic(uint32_t) g_overflowed = 0;
+static _Atomic(uint32_t) g_started = 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);
+static _Atomic(sentrycrashbic_cacheChangeCallback) g_addedCallback = NULL;
+static _Atomic(sentrycrashbic_cacheChangeCallback) g_removedCallback = NULL;
 
-    if (idx >= MAX_DYLD_IMAGES) {
-        atomic_store_explicit(&g_overflowed, 1, memory_order_relaxed);
+static void
+add_dyld_image(const struct mach_header *mh)
+{
+    // Don't add images if the cache is not started
+    if (!atomic_load_explicit(&g_started, memory_order_acquire)) {
         return;
     }
-    
+
+    // 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(mh, &info) || info.dli_fname == NULL) {
         return;
     }
 
-    PublishedBinaryImage* entry = &g_images[idx];
+    // 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) {
+        atomic_store_explicit(&g_overflowed, 1, memory_order_relaxed);
+        return;
+    }
+
+    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 dyld_add_image_cb(const struct mach_header* mh, intptr_t slide) {
+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);
+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;
+            }
+        }
+    }
+}
+
+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)
 {
-    uint32_t count =
-        atomic_load_explicit(&g_next_index, memory_order_acquire);
+    uint32_t count = atomic_load_explicit(&g_next_index, memory_order_acquire);
 
-    if (count > MAX_DYLD_IMAGES) count = MAX_DYLD_IMAGES;
+    if (count > MAX_DYLD_IMAGES)
+        count = MAX_DYLD_IMAGES;
 
     for (uint32_t i = 0; i < count; i++) {
-        PublishedBinaryImage* src = &g_images[i];
+        PublishedBinaryImage *src = &g_images[i];
 
-        if (!atomic_load_explicit(&src->ready, memory_order_acquire)) {
-            return; // stop at first unpublished entry
+        if (atomic_load_explicit(&src->ready, memory_order_acquire)) {
+            callback(&src->image, context);
         }
-
-        callback(&src->image, context);
     }
 }
 
@@ -130,7 +187,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
@@ -141,57 +198,104 @@ 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.
-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)) {
+    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;
     }
-    add_dyld_image(header);
+
+    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;
+    }
+
+    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);
+    }
 }
 
-void
-sentrycrashbic_startCache(void)
+static void
+sentrycrashbic_startCacheImpl(void)
 {
+    // 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;
+    }
+
     if (sentrycrashbic_shouldAddDyld()) {
         sentrycrashdl_initialize();
         sentrycrashbic_addDyldNode();
     }
 
+    dyld_tracker_start();
+}
+
+void
+sentrycrashbic_startCache(void)
+{
     // 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.
-    //
-    // 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();
-    });
+    // 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
 }
 
 void
 sentrycrashbic_stopCache(void)
 {
-    // TODO: Why do we need to support stopping it?
+    // 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
+    atomic_store_explicit(&g_started, 0, memory_order_release);
+    atomic_store_explicit(&g_next_index, 0, memory_order_release);
 }
 
 void
 sentrycrashbic_registerAddedCallback(sentrycrashbic_cacheChangeCallback callback)
 {
-    // TODO: I think this is only for when we used to have on-device symbolication. Should be able to remove now
+    atomic_store_explicit(&g_addedCallback, callback, memory_order_release);
+
+    if (callback != NULL) {
+        // 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)
 {
-    // TODO: I think this is only for when we used to have on-device symbolication. Should be able to remove now
+    atomic_store_explicit(&g_removedCallback, callback, memory_order_release);
 }

Sources/Swift/Core/Helper/SentryBinaryImageCache.swift

@@ -42,8 +42,7 @@ import Foundation
                     return
                 }
                 let image = imagePtr.pointee
-                SentryDependencyContainer.sharedInstance().binaryImageCache.binaryImageAdded(
-                    imageName: image.name, vmAddress: image.vmAddress, address: image.address, size: image.size, uuid: image.uuid)
+                SentryDependencyContainer.sharedInstance().binaryImageCache.binaryImageAdded(image: image)
             }
             sentrycrashbic_registerRemovedCallback { imagePtr in
                 guard let imagePtr else {
@@ -64,14 +63,8 @@ import Foundation
         }
     }
 
-    // We have to expand `SentryCrashBinaryImage` since the model is defined in SentryPrivate
-    @objc(binaryImageAdded:vmAddress:address:size:uuid:)
-    public func binaryImageAdded(imageName: UnsafePointer<CChar>?,
-                                 vmAddress: UInt64,
-                                 address: UInt64,
-                                 size: UInt64,
-                                 uuid: UnsafePointer<UInt8>?) {
-        guard let imageName else {
+    func binaryImageAdded(image: SentryCrashBinaryImage) {
+        guard let imageName = image.name else {
             SentrySDKLog.warning("The image name was NULL. Can't add image to cache.")
             return
         }
@@ -82,10 +75,10 @@ import Foundation
 
         let newImage = SentryBinaryImageInfo(
             name: nameString,
-            uuid: Self.convertUUID(uuid),
-            vmAddress: vmAddress,
-            address: address,
-            size: size
+            uuid: Self.convertUUID(image.uuid),
+            vmAddress: image.vmAddress,
+            address: image.address,
+            size: image.size
         )
 
         lock.synchronized {
@@ -118,18 +111,16 @@ import Foundation
                                                 dispatchQueueWrapper: Dependencies.dispatchQueueWrapper)
         }
     }
-    
-    @objc
-    public static func convertUUID(_ value: UnsafePointer<UInt8>?) -> String? {
+
+    private static func convertUUID(_ value: UnsafePointer<UInt8>?) -> String? {
         guard let value = value else { return nil }
 
         var uuidBuffer = [CChar](repeating: 0, count: 37)
         sentrycrashdl_convertBinaryImageUUID(value, &uuidBuffer)
         return String(cString: uuidBuffer, encoding: .ascii)
     }
-    
-    @objc
-    public func binaryImageRemoved(_ imageAddress: UInt64) {
+
+    func binaryImageRemoved(_ imageAddress: UInt64) {
         lock.synchronized {
             guard let index = indexOfImage(address: imageAddress) else { return }
             self.cache?.remove(at: index)
@@ -166,8 +157,7 @@ import Foundation
         return nil
     }
 
-    @objc(imagePathsForInAppInclude:)
-    public func imagePathsFor(inAppInclude: String) -> Set<String> {
+    func imagePathsFor(inAppInclude: String) -> Set<String> {
         lock.synchronized {
             var imagePaths = Set<String>()
 
@@ -182,8 +172,7 @@ import Foundation
         }
     }
 
-    @objc
-    public func getAllBinaryImages() -> [SentryBinaryImageInfo] {
+    func getAllBinaryImages() -> [SentryBinaryImageInfo] {
         lock.synchronized {
             return cache ?? []
         }

Sources/Swift/SentryCrash/SentryDebugImageProvider.swift

@@ -1,6 +1,8 @@
 // swiftlint:disable missing_docs
 @_implementationOnly import _SentryPrivate
 
+// Wraps `SentryBinaryImageCache` which is not reentrant. It's a faster way to get an image name from an address
+// but cannot be used from async signal safe code.
 @_spi(Private) @objc public class SentryDebugImageProvider: NSObject {
 
     private static let debugImageType = "macho"

Tests/SentryTests/Integrations/Performance/UIViewController/SentryUIViewControllerSwizzlingTests.swift

@@ -200,11 +200,7 @@ class SentryUIViewControllerSwizzlingTests: XCTestCase {
 
         let image = createCrashBinaryImage(0, name: debugDylib)
         SentryDependencyContainer.sharedInstance().binaryImageCache.start(false)
-        SentryDependencyContainer.sharedInstance().binaryImageCache.binaryImageAdded(imageName: image.name,
-                                                                                     vmAddress: image.vmAddress,
-                                                                                     address: image.address,
-                                                                                     size: image.size,
-                                                                                     uuid: image.uuid)
+        SentryDependencyContainer.sharedInstance().binaryImageCache.binaryImageAdded(image: image)
 
         let sut = fixture.sut
         sut.start()