Merge pull request #1011 from bugsnag/nickdowell/fix-thread-recording…

…-deadlock [PLAT-6112] Fix possible deadlock when recording threads
bugsnag · Feb 23, 2021 · 5fddcf2 · 5fddcf2
2 parents 670a768 + 9b7b10c
commit 5fddcf2
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 19 deletions.
diff --git a/Bugsnag/Payload/BugsnagThread+Recording.m b/Bugsnag/Payload/BugsnagThread+Recording.m
@@ -32,34 +32,33 @@ static void backtrace_for_thread(thread_t thread, struct backtrace_t *output) {
     }
 }
 
-static void backtrace_for_callstack(NSArray<NSNumber *> *callStackReturnAddresses, struct backtrace_t *output) {
-    output->length = (int)MIN(callStackReturnAddresses.count, kMaxAddresses);
-    for (int i = 0; i < output->length; i++) {
-        output->addresses[i] = (uintptr_t)callStackReturnAddresses[i].unsignedLongLongValue;
-    }
-}
-
 
 // MARK: -
 
 @implementation BugsnagThread (Recording)
 
 + (NSArray<BugsnagThread *> *)allThreads:(BOOL)allThreads callStackReturnAddresses:(NSArray<NSNumber *> *)callStackReturnAddresses {
+    struct backtrace_t backtrace;
+    backtrace.length = (int)MIN(callStackReturnAddresses.count, kMaxAddresses);
+    for (int i = 0; i < backtrace.length; i++) {
+        backtrace.addresses[i] = (uintptr_t)callStackReturnAddresses[i].unsignedLongLongValue;
+    }
     if (allThreads) {
-        return [BugsnagThread allThreadsWithCallStackReturnAddresses:callStackReturnAddresses];
+        return [BugsnagThread allThreadsWithCurrentThreadBacktrace:&backtrace];
     } else {
-        return @[[BugsnagThread currentThreadWithCallStackReturnAddresses:callStackReturnAddresses]];
+        return @[[BugsnagThread currentThreadWithBacktrace:&backtrace]];
     }
 }
 
-+ (NSArray<BugsnagThread *> *)allThreadsWithCallStackReturnAddresses:(NSArray<NSNumber *> *)callStackReturnAddresses {
++ (NSArray<BugsnagThread *> *)allThreadsWithCurrentThreadBacktrace:(struct backtrace_t *)currentThreadBacktrace {
     thread_t *threads = NULL;
     mach_msg_type_number_t threadCount = 0;
 
     bsg_kscrashsentry_suspend_threads_user();
 
     // While threads are suspended only async-signal-safe functions should be used,
-    // as another threads may have been suspended while holding a lock.
+    // as another threads may have been suspended while holding a lock in malloc,
+    // the Objective-C runtime, or other subsystems.
 
     if (task_threads(mach_task_self(), &threads, &threadCount) != KERN_SUCCESS) {
         bsg_kscrashsentry_resume_threads_user(false);
@@ -71,7 +70,7 @@ @implementation BugsnagThread (Recording)
     for (int i = 0; i < threadCount; i++) {
         BOOL isCurrentThread = MACH_PORT_INDEX(threads[i]) == MACH_PORT_INDEX(bsg_ksmachthread_self());
         if (isCurrentThread) {
-            backtrace_for_callstack(callStackReturnAddresses, &backtraces[i]);
+            backtraces[i].length = 0; // currentThreadBacktrace will be used instead
         } else {
             backtrace_for_thread(threads[i], &backtraces[i]);
         }
@@ -83,9 +82,10 @@ @implementation BugsnagThread (Recording)
 
     for (int i = 0; i < threadCount; i++) {
         BOOL isCurrentThread = MACH_PORT_INDEX(threads[i]) == MACH_PORT_INDEX(bsg_ksmachthread_self());
+        struct backtrace_t *backtrace = isCurrentThread ? currentThreadBacktrace : &backtraces[i];
         [objects addObject:[[BugsnagThread alloc] initWithMachThread:threads[i]
-                                                  backtraceAddresses:backtraces[i].addresses
-                                                     backtraceLength:backtraces[i].length
+                                                  backtraceAddresses:backtrace->addresses
+                                                     backtraceLength:backtrace->length
                                                 errorReportingThread:isCurrentThread
                                                                index:i]];
     }
@@ -98,10 +98,8 @@ @implementation BugsnagThread (Recording)
     return objects;
 }
 
-+ (instancetype)currentThreadWithCallStackReturnAddresses:(NSArray<NSNumber *> *)callStackReturnAddresses {
++ (instancetype)currentThreadWithBacktrace:(struct backtrace_t *)backtrace {
     thread_t thread = mach_thread_self();
-    struct backtrace_t backtrace;
-    backtrace_for_callstack(callStackReturnAddresses, &backtrace);
     thread_t *threads = NULL;
     mach_msg_type_number_t threadCount = 0;
     int threadIndex = 0;
@@ -115,8 +113,8 @@ + (instancetype)currentThreadWithCallStackReturnAddresses:(NSArray<NSNumber *> *
         vm_deallocate(mach_task_self(), (vm_address_t)threads, sizeof(thread_t) * threadCount);
     }
     BugsnagThread *object = [[BugsnagThread alloc] initWithMachThread:thread
-                                                   backtraceAddresses:backtrace.addresses
-                                                      backtraceLength:backtrace.length
+                                                   backtraceAddresses:backtrace->addresses
+                                                      backtraceLength:backtrace->length
                                                  errorReportingThread:YES
                                                                 index:threadIndex];
     mach_port_deallocate(mach_task_self(), thread);

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,9 @@ Changelog
 
 ### Bug fixes
 
+* Fix possible deadlock when recording thread information for handled errors.
+  [1011](https://github.com/bugsnag/bugsnag-cocoa/pull/1011)
+
 * Fix Swift 5.4 fatal error message parsing.
   [1010](https://github.com/bugsnag/bugsnag-cocoa/pull/1010)