fix: Potential deadlock in app hang detection (#4063)

The app hang detection retrieves the stacktraces of all threads when it detects an app hang. The logic for retrieving the stacktrace uses sentrycrashdl_dladdr, which isn't async-safe, although it claims to be because it accesses _dyld_get_image_header, which acquires a lock. Therefore, in rare cases, retrieving the stacktrace for all threads can lead to a deadlock. We fix this by skipping symbolication, which calls sentrycrashdl_dladdr when debug is false, which we we thought we did in Fixes GH-4056
getsentry · Jun 14, 2024 · e145ca1 · e145ca1
1 parent ec879f7
commit e145ca1
Show file tree

Hide file tree

Showing 10 changed files with 86 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Fixes
+
+- Fix potential deadlock in app hang detection (#4063)
+
 ## 8.29.0
 
 ### Features

diff --git a/Sources/Sentry/SentryCrashStackEntryMapper.m b/Sources/Sentry/SentryCrashStackEntryMapper.m
@@ -29,7 +29,9 @@ - (SentryFrame *)sentryCrashStackEntryToSentryFrame:(SentryCrashStackEntry)stack
 {
     SentryFrame *frame = [[SentryFrame alloc] init];
 
-    frame.symbolAddress = sentry_formatHexAddressUInt64(stackEntry.symbolAddress);
+    if (stackEntry.symbolAddress != 0) {
+        frame.symbolAddress = sentry_formatHexAddressUInt64(stackEntry.symbolAddress);
+    }
 
     frame.instructionAddress = sentry_formatHexAddressUInt64(stackEntry.address);
 

diff --git a/Sources/Sentry/SentryStacktraceBuilder.m b/Sources/Sentry/SentryStacktraceBuilder.m
@@ -42,7 +42,7 @@ - (SentryStacktrace *)retrieveStacktraceFromCursor:(SentryCrashStackCursor)stack
             // skip the marker frame
             continue;
         }
-        if (self.symbolicate == false || stackCursor.symbolicate(&stackCursor)) {
+        if (self.symbolicate == NO || stackCursor.symbolicate(&stackCursor)) {
             frame = [self.crashStackEntryMapper mapStackEntryWithCursor:stackCursor];
             [frames addObject:frame];
         }

diff --git a/Sources/Sentry/SentryThreadInspector.m b/Sources/Sentry/SentryThreadInspector.m
@@ -18,6 +18,7 @@
 
 @property (nonatomic, strong) SentryStacktraceBuilder *stacktraceBuilder;
 @property (nonatomic, strong) id<SentryCrashMachineContextWrapper> machineContextWrapper;
+@property (nonatomic, assign) BOOL symbolicate;
 
 @end
 
@@ -31,7 +32,7 @@
 // calling into not async-signal-safe code while there are suspended threads.
 unsigned int
 getStackEntriesFromThread(SentryCrashThread thread, struct SentryCrashMachineContext *context,
-    SentryCrashStackEntry *buffer, unsigned int maxEntries)
+    SentryCrashStackEntry *buffer, unsigned int maxEntries, bool symbolicate)
 {
     sentrycrashmc_getContextForThread(thread, context, NO);
     SentryCrashStackCursor stackCursor;
@@ -42,7 +43,7 @@
     while (stackCursor.advanceCursor(&stackCursor)) {
         if (entries == maxEntries)
             break;
-        if (stackCursor.symbolicate(&stackCursor)) {
+        if (symbolicate == false || stackCursor.symbolicate(&stackCursor)) {
             buffer[entries] = stackCursor.stackEntry;
             entries++;
         }
@@ -55,10 +56,12 @@ @implementation SentryThreadInspector
 
 - (id)initWithStacktraceBuilder:(SentryStacktraceBuilder *)stacktraceBuilder
        andMachineContextWrapper:(id<SentryCrashMachineContextWrapper>)machineContextWrapper
+                    symbolicate:(BOOL)symbolicate
 {
     if (self = [super init]) {
         self.stacktraceBuilder = stacktraceBuilder;
         self.machineContextWrapper = machineContextWrapper;
+        self.symbolicate = symbolicate;
     }
     return self;
 }
@@ -77,7 +80,8 @@ - (instancetype)initWithOptions:(SentryOptions *)options
     id<SentryCrashMachineContextWrapper> machineContextWrapper =
         [[SentryCrashDefaultMachineContextWrapper alloc] init];
     return [self initWithStacktraceBuilder:stacktraceBuilder
-                  andMachineContextWrapper:machineContextWrapper];
+                  andMachineContextWrapper:machineContextWrapper
+                               symbolicate:options.debug];
 }
 
 - (SentryStacktrace *)stacktraceForCurrentThreadAsyncUnsafe
@@ -140,6 +144,8 @@ - (SentryStacktrace *)stacktraceForCurrentThreadAsyncUnsafe
         thread_act_array_t suspendedThreads = NULL;
         mach_msg_type_number_t numSuspendedThreads = 0;
 
+        bool symbolicate = self.symbolicate;
+
         // SentryThreadInspector is crashing when there is too many threads.
         // We add a limit of 70 threads because in test with up to 100 threads it seems fine.
         // We are giving it an extra safety margin.
@@ -159,7 +165,7 @@ - (SentryStacktrace *)stacktraceForCurrentThreadAsyncUnsafe
         for (int i = 0; i < numSuspendedThreads; i++) {
             if (suspendedThreads[i] != currentThread) {
                 int numberOfEntries = getStackEntriesFromThread(suspendedThreads[i], context,
-                    threadsInfos[i].stackEntries, MAX_STACKTRACE_LENGTH);
+                    threadsInfos[i].stackEntries, MAX_STACKTRACE_LENGTH, symbolicate);
                 threadsInfos[i].stackLength = numberOfEntries;
             } else {
                 // We can't use 'getStackEntriesFromThread' to retrieve stack frames from the

diff --git a/Sources/Sentry/include/SentryThreadInspector.h b/Sources/Sentry/include/SentryThreadInspector.h
@@ -10,7 +10,8 @@ NS_ASSUME_NONNULL_BEGIN
 SENTRY_NO_INIT
 
 - (id)initWithStacktraceBuilder:(SentryStacktraceBuilder *)stacktraceBuilder
-       andMachineContextWrapper:(id<SentryCrashMachineContextWrapper>)machineContextWrapper;
+       andMachineContextWrapper:(id<SentryCrashMachineContextWrapper>)machineContextWrapper
+                    symbolicate:(BOOL)symbolicate;
 
 - (instancetype)initWithOptions:(SentryOptions *)options;
 

diff --git a/Sources/SentryCrash/Recording/Tools/SentryCrashDynamicLinker.h b/Sources/SentryCrash/Recording/Tools/SentryCrashDynamicLinker.h
@@ -99,7 +99,11 @@ uint32_t sentrycrashdl_imageNamed(const char *const imageName, bool exactMatch);
  */
 const uint8_t *sentrycrashdl_imageUUID(const char *const imageName, bool exactMatch);
 
-/** async-safe version of dladdr.
+/**
+ * ATTENTION: This method isn't async-safe as it accesses @c _dyld_get_image_header, which acquires
+ * a lock. We plan on removing this method with
+ * https://github.com/getsentry/sentry-cocoa/issues/2996.
+ *
  *
  * This method searches the dynamic loader for information about any image
  * containing the specified address. It may not be entirely successful in

diff --git a/Sources/SentryCrash/Recording/Tools/SentryCrashSymbolicator.c b/Sources/SentryCrash/Recording/Tools/SentryCrashSymbolicator.c
@@ -68,6 +68,9 @@ symbolicate_internal(SentryCrashStackCursor *cursor, bool asyncUnsafe)
     if (asyncUnsafe) {
         symbols_succeed = dladdr((void *)cursor->stackEntry.address, &symbolsBuffer) != 0;
     } else {
+        // sentrycrashdl_dladdr isn't async safe, but we've been using it for
+        // crashes since the beginning. We plan on removing it with
+        // https://github.com/getsentry/sentry-cocoa/issues/2996.
         symbols_succeed = sentrycrashdl_dladdr(
             CALL_INSTRUCTION_FROM_RETURN_ADDRESS(cursor->stackEntry.address), &symbolsBuffer);
     }

diff --git a/Tests/SentryTests/SentryCrash/SentryCrashStackEntryMapperTests.swift b/Tests/SentryTests/SentryCrash/SentryCrashStackEntryMapperTests.swift
@@ -28,6 +28,15 @@ class SentryCrashStackEntryMapperTests: XCTestCase {
         XCTAssertEqual("0x000000008e902bf0", frame.symbolAddress ?? "")
     }
 
+    func testSymbolAddress_IsZero() {
+        var cursor = SentryCrashStackCursor()
+        cursor.stackEntry.symbolAddress = 0
+
+        let frame = sut.mapStackEntry(with: cursor)
+
+        XCTAssertNil(frame.symbolAddress)
+    }
+
     func testInstructionAddress() {
         var cursor = SentryCrashStackCursor()
         cursor.stackEntry.address = 2_412_813_376

diff --git a/Tests/SentryTests/SentryCrash/SentryThreadInspectorTests.swift b/Tests/SentryTests/SentryCrash/SentryThreadInspectorTests.swift
@@ -17,7 +17,7 @@ class SentryThreadInspectorTests: XCTestCase {
 
             return SentryThreadInspector(
                 stacktraceBuilder: stacktraceBuilder,
-                andMachineContextWrapper: machineContextWrapper
+                andMachineContextWrapper: machineContextWrapper, symbolicate: symbolicate
             )
         }
     }
@@ -47,18 +47,51 @@ class SentryThreadInspectorTests: XCTestCase {
         XCTAssertTrue(30 < stacktrace?.frames.count ?? 0, "Not enough stacktrace frames.")
     }
 
-    func testStacktraceHasFrames_forEveryThread() {
-        assertStackForEveryThread()
+    func testGetCurrentThreadsWithStacktrace_WithSymbolication() {
+        let queue = DispatchQueue(label: "test-queue", attributes: [.concurrent, .initiallyInactive])
+
+        let expect = expectation(description: "Read every thread")
+        expect.expectedFulfillmentCount = 10
+
+        let sut = self.fixture.getSut(testWithRealMachineContextWrapper: true)
+        for _ in 0..<10 {
+
+            queue.async {
+                let actual = sut.getCurrentThreadsWithStackTrace()
+
+                // Sometimes during tests its possible to have one thread without frames
+                // We just need to make sure we retrieve frame information for at least one other thread than the main thread
+                var threadsWithFrames = 0
+
+                for thr in actual {
+                    if (thr.stacktrace?.frames.count ?? 0) >= 1 {
+                        threadsWithFrames += 1
+                    }
+
+                    for frame in thr.stacktrace?.frames ?? [] {
+                        XCTAssertNotNil(frame.instructionAddress)
+                        XCTAssertNotNil(frame.imageAddress)
+                    }
+                }
+
+                XCTAssertTrue(threadsWithFrames > 1, "Not enough threads with frames")
+
+                expect.fulfill()
+            }
+        }
+
+        queue.activate()
+        wait(for: [expect], timeout: 10)
     }
 
-    func assertStackForEveryThread() {
-
-        let queue = DispatchQueue(label: "defaultphil", attributes: [.concurrent, .initiallyInactive])
+    func testGetCurrentThreadsWithStacktrace_WithoutSymbolication() {
+        let queue = DispatchQueue(label: "test-queue", attributes: [.concurrent, .initiallyInactive])
 
         let expect = expectation(description: "Read every thread")
         expect.expectedFulfillmentCount = 10
 
-        let sut = self.fixture.getSut(testWithRealMachineContextWrapper: true)
+        let sut = self.fixture.getSut(testWithRealMachineContextWrapper: true, symbolicate: false)
+
         for _ in 0..<10 {
 
             queue.async {
@@ -72,6 +105,12 @@ class SentryThreadInspectorTests: XCTestCase {
                     if (thr.stacktrace?.frames.count ?? 0) >= 1 {
                         threadsWithFrames += 1
                     }
+
+                    for frame in thr.stacktrace?.frames ?? [] {
+                        XCTAssertNotNil(frame.instructionAddress)
+                        XCTAssertNotNil(frame.imageAddress)
+                        XCTAssertNil(frame.symbolAddress)
+                    }
                 }
 
                 XCTAssertTrue(threadsWithFrames > 1, "Not enough threads with frames")

diff --git a/Tests/SentryTests/SentryCrash/TestThreadInspector.swift b/Tests/SentryTests/SentryCrash/TestThreadInspector.swift
@@ -9,7 +9,7 @@ class TestThreadInspector: SentryThreadInspector {
         let inAppLogic = SentryInAppLogic(inAppIncludes: [], inAppExcludes: [])
         let crashStackEntryMapper = SentryCrashStackEntryMapper(inAppLogic: inAppLogic)
         let stacktraceBuilder = SentryStacktraceBuilder(crashStackEntryMapper: crashStackEntryMapper)
-        return TestThreadInspector(stacktraceBuilder: stacktraceBuilder, andMachineContextWrapper: SentryCrashDefaultMachineContextWrapper())
+        return TestThreadInspector(stacktraceBuilder: stacktraceBuilder, andMachineContextWrapper: SentryCrashDefaultMachineContextWrapper(), symbolicate: false)
     }
 
     override func stacktraceForCurrentThreadAsyncUnsafe() -> SentryStacktrace? {