From e2fb3e7bdcef655d954d60c7ddb4ab0fd6231685 Mon Sep 17 00:00:00 2001 From: Noah Martin Date: Fri, 13 Feb 2026 09:40:30 -0800 Subject: [PATCH 1/2] fix: Fill dyld image cache asynchronously to avoid increasing app launch time sentrycrashbic_startCache now dispatches its work on a background queue so that the expensive _dyld_register_func_for_add_image callback (which calls dladdr for every loaded image) does not block the main thread during SDK initialization. Since the cache may not be populated yet when SentryUIViewControllerSwizzling.start() runs, replace its SentryBinaryImageCache lookup with direct synchronous iteration of _dyld_image_count/_dyld_get_image_name. Performance-sensitive users can disable UIViewController swizzling. Also fix a race condition in SentryCrashBinaryImageCache where stopCache resets g_next_index while a concurrent add_dyld_image could increment it afterwards. Move the reset into startCacheImpl and gate iterateOverImages/registerAddedCallback on g_started. --- .../Recording/SentryCrashBinaryImageCache.c | 270 ++++++++++-------- .../Core/Helper/SentryBinaryImageCache.swift | 50 +--- .../SentryUIViewControllerSwizzling.swift | 21 +- .../SentryDebugImageProvider.swift | 2 + Sources/Swift/SentryDependencyContainer.swift | 1 - ...SentryUIViewControllerSwizzlingTests.swift | 36 +-- .../SentryBinaryImageCacheTests.swift | 31 +- .../SentryCrashBinaryImageCacheTests.m | 22 +- .../SentryCrashStackEntryMapperTests.swift | 6 +- .../SentryDebugImageProviderTests.swift | 6 +- 10 files changed, 209 insertions(+), 236 deletions(-) diff --git a/Sources/SentryCrash/Recording/SentryCrashBinaryImageCache.c b/Sources/SentryCrash/Recording/SentryCrashBinaryImageCache.c index 7fe4c4dd046..82bf7da6ec3 100644 --- a/Sources/SentryCrash/Recording/SentryCrashBinaryImageCache.c +++ b/Sources/SentryCrash/Recording/SentryCrashBinaryImageCache.c @@ -1,8 +1,10 @@ #include "SentryCrashBinaryImageCache.h" #include "SentryCrashDynamicLinker.h" +#include #include #include #include +#include #include #include #include @@ -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); } diff --git a/Sources/Swift/Core/Helper/SentryBinaryImageCache.swift b/Sources/Swift/Core/Helper/SentryBinaryImageCache.swift index bc1009097c1..4e4515cd9ae 100644 --- a/Sources/Swift/Core/Helper/SentryBinaryImageCache.swift +++ b/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?, - vmAddress: UInt64, - address: UInt64, - size: UInt64, - uuid: UnsafePointer?) { - 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?) -> String? { + + private static func convertUUID(_ value: UnsafePointer?) -> 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,24 +157,7 @@ import Foundation return nil } - @objc(imagePathsForInAppInclude:) - public func imagePathsFor(inAppInclude: String) -> Set { - lock.synchronized { - var imagePaths = Set() - - guard let cache = self.cache else { return imagePaths } - - for info in cache { - if SentryInAppLogic.isImageNameInApp(info.name, inAppInclude: inAppInclude) { - imagePaths.insert(info.name) - } - } - return imagePaths - } - } - - @objc - public func getAllBinaryImages() -> [SentryBinaryImageInfo] { + func getAllBinaryImages() -> [SentryBinaryImageInfo] { lock.synchronized { return cache ?? [] } diff --git a/Sources/Swift/Core/Integrations/Performance/SentryUIViewControllerSwizzling.swift b/Sources/Swift/Core/Integrations/Performance/SentryUIViewControllerSwizzling.swift index bc608806928..86c162b6cda 100644 --- a/Sources/Swift/Core/Integrations/Performance/SentryUIViewControllerSwizzling.swift +++ b/Sources/Swift/Core/Integrations/Performance/SentryUIViewControllerSwizzling.swift @@ -1,4 +1,5 @@ @_implementationOnly import _SentryPrivate +import MachO #if (os(iOS) || os(tvOS) || os(visionOS)) && !SENTRY_NO_UI_FRAMEWORK import UIKit @@ -19,7 +20,6 @@ class SentryUIViewControllerSwizzling { private let subClassFinder: SentrySubClassFinder private let imagesActedOnSubclassesOfUIViewControllers: NSMutableSet private let processInfoWrapper: SentryProcessInfoSource - private let binaryImageCache: SentryBinaryImageCache private let performanceTracker: SentryUIViewControllerPerformanceTracker init( @@ -28,7 +28,6 @@ class SentryUIViewControllerSwizzling { objcRuntimeWrapper: SentryObjCRuntimeWrapper, subClassFinder: SentrySubClassFinder, processInfoWrapper: SentryProcessInfoSource, - binaryImageCache: SentryBinaryImageCache, performanceTracker: SentryUIViewControllerPerformanceTracker ) { self.options = options @@ -38,19 +37,23 @@ class SentryUIViewControllerSwizzling { self.subClassFinder = subClassFinder self.imagesActedOnSubclassesOfUIViewControllers = NSMutableSet() self.processInfoWrapper = processInfoWrapper - self.binaryImageCache = binaryImageCache self.performanceTracker = performanceTracker } func start() { - for inAppInclude in inAppLogic.inAppIncludes { - let imagePathsToInAppInclude = binaryImageCache.imagePathsFor(inAppInclude: inAppInclude) + let imageNames = (0..<_dyld_image_count()).compactMap { i in + _dyld_get_image_name(i).map { String(cString: $0) } + } - if !imagePathsToInAppInclude.isEmpty { - for imagePath in imagePathsToInAppInclude { - swizzleUIViewControllers(ofImage: imagePath) + for inAppInclude in inAppLogic.inAppIncludes { + var found = false + for imageName in imageNames { + if SentryInAppLogic.isImageNameInApp(imageName, inAppInclude: inAppInclude) { + found = true + swizzleUIViewControllers(ofImage: imageName) } - } else { + } + if !found { SentrySDKLog.warning( "Failed to find the binary image(s) for inAppInclude <\(inAppInclude)> and, therefore can't instrument UIViewControllers in these binaries." ) diff --git a/Sources/Swift/SentryCrash/SentryDebugImageProvider.swift b/Sources/Swift/SentryCrash/SentryDebugImageProvider.swift index 1fb5c94d2cc..24a11b6c8e5 100644 --- a/Sources/Swift/SentryCrash/SentryDebugImageProvider.swift +++ b/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" diff --git a/Sources/Swift/SentryDependencyContainer.swift b/Sources/Swift/SentryDependencyContainer.swift index fe8919d2d76..2fe4c97b0d0 100644 --- a/Sources/Swift/SentryDependencyContainer.swift +++ b/Sources/Swift/SentryDependencyContainer.swift @@ -234,7 +234,6 @@ extension SentryFileManager: SentryFileManagerProtocol { } objcRuntimeWrapper: objcRuntimeWrapper, subClassFinder: subClassFinder, processInfoWrapper: processInfoWrapper, - binaryImageCache: binaryImageCache, performanceTracker: uiViewControllerPerformanceTracker ) diff --git a/Tests/SentryTests/Integrations/Performance/UIViewController/SentryUIViewControllerSwizzlingTests.swift b/Tests/SentryTests/Integrations/Performance/UIViewController/SentryUIViewControllerSwizzlingTests.swift index 1c0844d2e1e..a94387c1f7b 100644 --- a/Tests/SentryTests/Integrations/Performance/UIViewController/SentryUIViewControllerSwizzlingTests.swift +++ b/Tests/SentryTests/Integrations/Performance/UIViewController/SentryUIViewControllerSwizzlingTests.swift @@ -12,13 +12,11 @@ class SentryUIViewControllerSwizzlingTests: XCTestCase { let objcRuntimeWrapper = SentryTestObjCRuntimeWrapper() let subClassFinder: TestSubClassFinder let processInfoWrapper = MockSentryProcessInfo() - let binaryImageCache: SentryBinaryImageCache let performanceTracker = SentryUIViewControllerPerformanceTracker() var options: Options init() { subClassFinder = TestSubClassFinder(dispatchQueue: dispatchQueue, objcRuntimeWrapper: objcRuntimeWrapper, swizzleClassNameExcludes: []) - binaryImageCache = SentryDependencyContainer.sharedInstance().binaryImageCache options = Options.noIntegrations() @@ -29,15 +27,15 @@ class SentryUIViewControllerSwizzlingTests: XCTestCase { } var sut: SentryUIViewControllerSwizzling { - return SentryUIViewControllerSwizzling(options: options, dispatchQueue: dispatchQueue, objcRuntimeWrapper: objcRuntimeWrapper, subClassFinder: subClassFinder, processInfoWrapper: processInfoWrapper, binaryImageCache: binaryImageCache, performanceTracker: performanceTracker) + return SentryUIViewControllerSwizzling(options: options, dispatchQueue: dispatchQueue, objcRuntimeWrapper: objcRuntimeWrapper, subClassFinder: subClassFinder, processInfoWrapper: processInfoWrapper, performanceTracker: performanceTracker) } var sutWithDefaultObjCRuntimeWrapper: SentryUIViewControllerSwizzling { - return SentryUIViewControllerSwizzling(options: options, dispatchQueue: dispatchQueue, objcRuntimeWrapper: SentryDependencyContainer.sharedInstance().objcRuntimeWrapper, subClassFinder: subClassFinder, processInfoWrapper: processInfoWrapper, binaryImageCache: binaryImageCache, performanceTracker: performanceTracker) + return SentryUIViewControllerSwizzling(options: options, dispatchQueue: dispatchQueue, objcRuntimeWrapper: SentryDependencyContainer.sharedInstance().objcRuntimeWrapper, subClassFinder: subClassFinder, processInfoWrapper: processInfoWrapper, performanceTracker: performanceTracker) } var testableSut: TestSentryUIViewControllerSwizzling { - return TestSentryUIViewControllerSwizzling(options: options, dispatchQueue: dispatchQueue, objcRuntimeWrapper: objcRuntimeWrapper, subClassFinder: subClassFinder, processInfoWrapper: processInfoWrapper, binaryImageCache: binaryImageCache, performanceTracker: performanceTracker) + return TestSentryUIViewControllerSwizzling(options: options, dispatchQueue: dispatchQueue, objcRuntimeWrapper: objcRuntimeWrapper, subClassFinder: subClassFinder, processInfoWrapper: processInfoWrapper, performanceTracker: performanceTracker) } var delegate: MockApplication.MockApplicationDelegate { @@ -187,34 +185,6 @@ class SentryUIViewControllerSwizzlingTests: XCTestCase { XCTAssertNotNil(SentrySDK.span) } - /// Xcode 16 introduces a new flag ENABLE_DEBUG_DYLIB (https://developer.apple.com/documentation/xcode/build-settings-reference#Enable-Debug-Dylib-Support) - /// If this flag is enabled, debug builds of app and app extension targets on supported platforms and SDKs - /// will be built with the main binary code in a separate “NAME.debug.dylib”. - /// This test adds this debug.dylib and checks if it gets swizzled. - func testSwizzle_DebugDylib_GetsSwizzled() { - let imageName = String( - cString: class_getImageName(SentryUIViewControllerSwizzlingTests.self)!, - encoding: .utf8)! as NSString - - let debugDylib = "\(imageName).debug.dylib" - - 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) - - let sut = fixture.sut - sut.start() - - let subClassFinderInvocations = fixture.subClassFinder.invocations - let result = subClassFinderInvocations.invocations.filter { $0.imageName == debugDylib } - - XCTAssertEqual(1, result.count) - } - func testSwizzle_fromScene_invalidNotification_NoObject() { let swizzler = fixture.testableSut diff --git a/Tests/SentryTests/SentryBinaryImageCacheTests.swift b/Tests/SentryTests/SentryBinaryImageCacheTests.swift index 277a63d5c47..f4bb91e0b25 100644 --- a/Tests/SentryTests/SentryBinaryImageCacheTests.swift +++ b/Tests/SentryTests/SentryBinaryImageCacheTests.swift @@ -18,11 +18,7 @@ class SentryBinaryImageCacheTests: XCTestCase { } private func addBinaryImageToSut(_ binaryImage: SentryCrashBinaryImage) { - sut.binaryImageAdded(imageName: binaryImage.name, - vmAddress: binaryImage.vmAddress, - address: binaryImage.address, - size: binaryImage.size, - uuid: binaryImage.uuid) + sut.binaryImageAdded(image: binaryImage) } func testBinaryImageAdded() { @@ -56,11 +52,7 @@ class SentryBinaryImageCacheTests: XCTestCase { } func testBinaryImageAdded_WithNilName() { - sut.binaryImageAdded(imageName: nil, - vmAddress: 100, - address: 1_000, - size: 100, - uuid: nil) + sut.binaryImageAdded(image: .init(address: 1_000, vmAddress: 100, size: 100, name: nil, uuid: nil, cpuType: 0, cpuSubType: 0, crashInfoMessage: nil, crashInfoMessage2: nil)) XCTAssertEqual(self.sut.cache?.count, 0) } @@ -129,25 +121,6 @@ class SentryBinaryImageCacheTests: XCTestCase { XCTAssertNil(sut.imageByAddress(399)) } - func testImagePathByName() { - let binaryImage = createCrashBinaryImage(0) - let binaryImage2 = createCrashBinaryImage(1) - addBinaryImageToSut(binaryImage) - addBinaryImageToSut(binaryImage2) - - let paths = sut.imagePathsFor(inAppInclude: "Expected Name at 0") - XCTAssertEqual(paths.first, "Expected Name at 0") - - let paths2 = sut.imagePathsFor(inAppInclude: "Expected Name at 1") - XCTAssertEqual(paths2.first, "Expected Name at 1") - - let bothPaths = sut.imagePathsFor(inAppInclude: "Expected") - XCTAssertEqual(bothPaths, ["Expected Name at 0", "Expected Name at 1"]) - - let didNotFind = sut.imagePathsFor(inAppInclude: "Name at 0") - XCTAssertTrue(didNotFind.isEmpty) - } - func testBinaryImageWithNULLName_DoesNotAddImage() { let address = UInt64(100) diff --git a/Tests/SentryTests/SentryCrash/SentryCrashBinaryImageCacheTests.m b/Tests/SentryTests/SentryCrash/SentryCrashBinaryImageCacheTests.m index de10fee61c8..92b1fa8d91f 100644 --- a/Tests/SentryTests/SentryCrash/SentryCrashBinaryImageCacheTests.m +++ b/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:@""]; + } +} + dispatch_semaphore_t delaySemaphore = NULL; dispatch_semaphore_t delayCalled = NULL; static void @@ -288,7 +299,11 @@ - (void)testSentryBinaryImageCacheIntegration [imageCache start:false]; // by calling start, SentryBinaryImageCache will register a callback with // `SentryCrashBinaryImageCache` that should be called for every image already cached. - XCTAssertEqual(5, imageCache.cache.count); + NSMutableArray *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); addBinaryImage([mach_headers_test_cache[5] pointerValue], 0); XCTAssertEqual(6, imageCache.cache.count); @@ -306,7 +321,10 @@ - (void)assertBinaryImageCacheLength:(int)expected { int counter = 0; sentrycrashbic_iterateOverImages(countNumberOfImagesInCache, &counter); - XCTAssertEqual(counter, expected); + NSMutableArray *names = [NSMutableArray new]; + sentrycrashbic_iterateOverImages(addBinaryImageNameToArray, (__bridge void *)(names)); + XCTAssertEqual( + counter, expected, @"Cache should have %d images but contained %@", expected, names); } - (void)assertCachedBinaryImages diff --git a/Tests/SentryTests/SentryCrash/SentryCrashStackEntryMapperTests.swift b/Tests/SentryTests/SentryCrash/SentryCrashStackEntryMapperTests.swift index b9472326be3..a103f45eb94 100644 --- a/Tests/SentryTests/SentryCrash/SentryCrashStackEntryMapperTests.swift +++ b/Tests/SentryTests/SentryCrash/SentryCrashStackEntryMapperTests.swift @@ -31,11 +31,7 @@ class SentryCrashStackEntryMapperTests: XCTestCase { func testImageFromCache() { let image = createCrashBinaryImage(2_488_998_912) 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) var cursor = SentryCrashStackCursor() cursor.stackEntry.address = 2_488_998_950 diff --git a/Tests/SentryTests/SentryCrash/SentryDebugImageProviderTests.swift b/Tests/SentryTests/SentryCrash/SentryDebugImageProviderTests.swift index d3311b4cfe3..12c7432577e 100644 --- a/Tests/SentryTests/SentryCrash/SentryDebugImageProviderTests.swift +++ b/Tests/SentryTests/SentryCrash/SentryDebugImageProviderTests.swift @@ -19,11 +19,7 @@ class SentryDebugImageProviderTests: XCTestCase { func getSut(images: [SentryCrashBinaryImage] = []) -> SentryDebugImageProvider { cache.start(false) for image in images { - cache.binaryImageAdded(imageName: image.name, - vmAddress: image.vmAddress, - address: image.address, - size: image.size, - uuid: image.uuid) + cache.binaryImageAdded(image: image) } let provider = SentryDebugImageProvider() From d5deb7c78b5ef97e98f9e33f47abf4661250e619 Mon Sep 17 00:00:00 2001 From: Noah Martin Date: Fri, 13 Feb 2026 11:46:07 -0800 Subject: [PATCH 2/2] Fix some corner cases --- .../include/SentryCrashBinaryImageCache.h | 6 +- .../Recording/SentryCrashBinaryImageCache.c | 99 ++++++++----------- .../SentryCrashBinaryImageCacheTests.m | 6 +- 3 files changed, 49 insertions(+), 62 deletions(-) diff --git a/Sources/Sentry/include/SentryCrashBinaryImageCache.h b/Sources/Sentry/include/SentryCrashBinaryImageCache.h index 01fbe576b54..e5e2cc6da50 100644 --- a/Sources/Sentry/include/SentryCrashBinaryImageCache.h +++ b/Sources/Sentry/include/SentryCrashBinaryImageCache.h @@ -20,15 +20,15 @@ void sentrycrashbic_iterateOverImages(sentrycrashbic_imageIteratorCallback index void sentrycrashbic_startCache(void); /** - * Stops the cache from monitoring binary image being loaded or removed. - * This will also clean the cache. + * This is a no-op TODO: Remove this */ void sentrycrashbic_stopCache(void); /** * Register a callback to be called every time a new binary image is added to the cache. * After register, this callback will be called for every image already in the cache, - * this is a thread safe operation. + * this is a thread safe operation. The callback can be called multiple times for the same image + * If the image was being registered on a different thread at the same time the callback is registered. */ void sentrycrashbic_registerAddedCallback(sentrycrashbic_cacheChangeCallback callback); diff --git a/Sources/SentryCrash/Recording/SentryCrashBinaryImageCache.c b/Sources/SentryCrash/Recording/SentryCrashBinaryImageCache.c index 82bf7da6ec3..64018125395 100644 --- a/Sources/SentryCrash/Recording/SentryCrashBinaryImageCache.c +++ b/Sources/SentryCrash/Recording/SentryCrashBinaryImageCache.c @@ -64,14 +64,20 @@ sentry_resetFuncForAddRemoveImage(void) #define MAX_DYLD_IMAGES 4096 +// Entry lifecycle states +#define IMAGE_EMPTY 0 // Slot reserved but data not written, or write failed +#define IMAGE_READY 1 // Published, visible to readers +#define IMAGE_REMOVED 3 // Image was unloaded + typedef struct { - _Atomic(uint32_t) ready; // 0 = not published, 1 = published + _Atomic(uint32_t) state; SentryCrashBinaryImage image; } PublishedBinaryImage; +// g_next_index monotonically increases and is never reset (except in test-only reset). +// Each slot is used at most once, so there are no stale state flags from prior runs. static PublishedBinaryImage g_images[MAX_DYLD_IMAGES]; static _Atomic(uint32_t) g_next_index = 0; -static _Atomic(uint32_t) g_started = 0; static _Atomic(sentrycrashbic_cacheChangeCallback) g_addedCallback = NULL; static _Atomic(sentrycrashbic_cacheChangeCallback) g_removedCallback = NULL; @@ -79,14 +85,7 @@ static _Atomic(sentrycrashbic_cacheChangeCallback) g_removedCallback = NULL; 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; @@ -102,18 +101,16 @@ add_dyld_image(const struct mach_header *mh) } 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. + if (!sentrycrashdl_getBinaryImageForHeader(mh, info.dli_fname, &entry->image, false)) { + // Leave state as IMAGE_EMPTY so the entry is never published. + return; + } + sentrycrashbic_cacheChangeCallback callback = atomic_load_explicit(&g_addedCallback, memory_order_acquire); - // ---- Publish ---- - atomic_store_explicit(&entry->ready, 1, memory_order_release); + atomic_store_explicit(&entry->state, IMAGE_READY, memory_order_release); if (callback != NULL) { callback(&entry->image); @@ -132,22 +129,18 @@ 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); + atomic_store_explicit(&src->state, IMAGE_REMOVED, memory_order_release); if (callback) { callback(&src->image); - return; } + return; } } } @@ -162,10 +155,6 @@ 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) @@ -174,7 +163,7 @@ sentrycrashbic_iterateOverImages(sentrycrashbic_imageIteratorCallback callback, for (uint32_t i = 0; i < count; i++) { PublishedBinaryImage *src = &g_images[i]; - if (atomic_load_explicit(&src->ready, memory_order_acquire)) { + if (atomic_load_explicit(&src->state, memory_order_acquire) == IMAGE_READY) { callback(&src->image, context); } } @@ -214,34 +203,20 @@ sentrycrashbic_addDyldNode(void) 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; } - atomic_store_explicit(&entry->ready, 1, memory_order_release); + atomic_store_explicit(&entry->state, IMAGE_READY, memory_order_release); } 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; - } - - // 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(); } - + // During this call the callback is invoked synchronously for every existing image. dyld_tracker_start(); } @@ -259,29 +234,41 @@ sentrycrashbic_startCache(void) #if defined(SENTRY_TEST) || defined(SENTRY_TEST_CI) sentrycrashbic_startCacheImpl(); #else - dispatch_async( - dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{ sentrycrashbic_startCacheImpl(); }); + static dispatch_once_t once_token = 0; + dispatch_once(&once_token, ^{ + dispatch_async( + dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{ sentrycrashbic_startCacheImpl(); }); + }); #endif } void sentrycrashbic_stopCache(void) +{ } + +// Resetting can create race conditions so should only be done in controlled test environments. +#if defined(SENTRY_TEST) || defined(SENTRY_TEST_CI) +void +sentry_resetBinaryImageCache(void) { - // 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); + uint32_t count = atomic_load_explicit(&g_next_index, memory_order_relaxed); + if (count > MAX_DYLD_IMAGES) + count = MAX_DYLD_IMAGES; + for (uint32_t i = 0; i < count; i++) { + atomic_store_explicit(&g_images[i].state, IMAGE_EMPTY, memory_order_relaxed); + } + atomic_store_explicit(&g_next_index, 0, memory_order_relaxed); + atomic_store_explicit(&g_addedCallback, NULL, memory_order_relaxed); + atomic_store_explicit(&g_removedCallback, NULL, memory_order_relaxed); } +#endif void sentrycrashbic_registerAddedCallback(sentrycrashbic_cacheChangeCallback callback) { atomic_store_explicit(&g_addedCallback, callback, memory_order_release); - if (callback != NULL && atomic_load_explicit(&g_started, memory_order_acquire)) { + 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) @@ -289,8 +276,8 @@ sentrycrashbic_registerAddedCallback(sentrycrashbic_cacheChangeCallback callback for (uint32_t i = 0; i < count; i++) { PublishedBinaryImage *src = &g_images[i]; - if (!atomic_load_explicit(&src->ready, memory_order_acquire)) { - break; + if (atomic_load_explicit(&src->state, memory_order_acquire) != IMAGE_READY) { + continue; } callback(&src->image); } diff --git a/Tests/SentryTests/SentryCrash/SentryCrashBinaryImageCacheTests.m b/Tests/SentryTests/SentryCrash/SentryCrashBinaryImageCacheTests.m index 92b1fa8d91f..de7aebc23b6 100644 --- a/Tests/SentryTests/SentryCrash/SentryCrashBinaryImageCacheTests.m +++ b/Tests/SentryTests/SentryCrash/SentryCrashBinaryImageCacheTests.m @@ -6,11 +6,12 @@ #include #include -// Exposing test only functions from `SentryCrashBinaryImageCache.m` +// Exposing test only functions from `SentryCrashBinaryImageCache.c` void sentry_setRegisterFuncForAddImage(void *addFunction); void sentry_setRegisterFuncForRemoveImage(void *removeFunction); void sentry_resetFuncForAddRemoveImage(void); void sentry_setFuncForBeforeAdd(void (*callback)(void)); +void sentry_resetBinaryImageCache(void); static void (*addBinaryImage)(const struct mach_header *mh, intptr_t vmaddr_slide); static void (*removeBinaryImage)(const struct mach_header *mh, intptr_t vmaddr_slide); @@ -114,8 +115,7 @@ - (void)setUp - (void)tearDown { sentrycrashdl_clearDyld(); - sentry_resetFuncForAddRemoveImage(); - sentrycrashbic_stopCache(); + sentry_resetBinaryImageCache(); sentry_setFuncForBeforeAdd(NULL); [SentryDependencyContainer reset]; }