Merge pull request #1148 from bugsnag/nickdowell/fix-threads-stack-ov…

…erflow

[PLAT-6973] Fix stack overflow in +[BugsnagThread allThreadsWithCurrentThreadBacktrace:]

Bugsnag/Payload/BugsnagThread.m

@@ -213,19 +213,23 @@ + (NSDictionary *)enhanceThreadInfo:(NSDictionary *)thread
     thread_t *threads = NULL;
     mach_msg_type_number_t threadCount = 0;
 
+    if (task_threads(mach_task_self(), &threads, &threadCount) != KERN_SUCCESS) {
+        return @[];
+    }
+
+    NSMutableArray *objects = [NSMutableArray arrayWithCapacity:threadCount];
+
+    struct backtrace_t *backtraces = calloc(threadCount, sizeof(struct backtrace_t));
+    if (!backtraces) {
+        goto cleanup;
+    }
+
     suspend_threads();
 
     // While threads are suspended only async-signal-safe functions should be used,
     // 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) {
-        resume_threads();
-        return @[];
-    }
-
-    struct backtrace_t backtraces[threadCount];
-
     for (mach_msg_type_number_t i = 0; i < threadCount; i++) {
         BOOL isCurrentThread = MACH_PORT_INDEX(threads[i]) == MACH_PORT_INDEX(bsg_ksmachthread_self());
         if (isCurrentThread) {
@@ -237,8 +241,6 @@ + (NSDictionary *)enhanceThreadInfo:(NSDictionary *)thread
 
     resume_threads();
 
-    NSMutableArray *objects = [NSMutableArray arrayWithCapacity:threadCount];
-
     for (mach_msg_type_number_t 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];
@@ -249,11 +251,13 @@ + (NSDictionary *)enhanceThreadInfo:(NSDictionary *)thread
                                                                index:i]];
     }
 
+cleanup:
     for (mach_msg_type_number_t i = 0; i < threadCount; i++) {
         mach_port_deallocate(mach_task_self(), threads[i]);
     }
     vm_deallocate(mach_task_self(), (vm_address_t)threads, sizeof(thread_t) * threadCount);
 
+    free(backtraces);
     return objects;
 }
 

CHANGELOG.md

@@ -5,6 +5,9 @@ Changelog
 
 ### Bug fixes
 
+* Fix a potential stack overflow in `+[BugsnagThread allThreadsWithCurrentThreadBacktrace:]`.
+  [#1148](https://github.com/bugsnag/bugsnag-cocoa/pull/1148)
+
 * Fix `NSNull` handling in `+[BugsnagError errorFromJson:]` and `+[BugsnagStackframe frameFromJson:]`.
   [#1143](https://github.com/bugsnag/bugsnag-cocoa/pull/1143)
   [#1138](https://github.com/bugsnag/bugsnag-cocoa/issues/1138)

Tests/BugsnagThreadTests.m

@@ -222,6 +222,26 @@ - (void)testAllThreads {
     XCTAssertGreaterThan(threads.count, 1);
 }
 
+- (void)testAllThreadsFromBackgroundDoesNotOverflowStack {
+    const int threadCount = 1000;
+    dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
+    for (int i = 0; i < threadCount; i++) {
+        [NSThread detachNewThreadSelector:@selector(waitForSemaphore:) toTarget:self withObject:semaphore];
+    }
+    __block NSArray<BugsnagThread *> *threads = nil;
+    dispatch_sync(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{
+        threads = [BugsnagThread allThreads:YES callStackReturnAddresses:NSThread.callStackReturnAddresses];
+    });
+    XCTAssertGreaterThan(threads.count, threadCount);
+    for (int i = 0; i < threadCount; i++) {
+        dispatch_semaphore_signal(semaphore);
+    }
+}
+
+- (void)waitForSemaphore:(dispatch_semaphore_t)semaphore {
+    dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
+}
+
 - (void)testCurrentThread {
     NSArray<BugsnagThread *> *threads = [BugsnagThread allThreads:NO callStackReturnAddresses:NSThread.callStackReturnAddresses];
     [threads[0].stacktrace makeObjectsPerformSelector:@selector(symbolicateIfNeeded)];