bugsnag · nickdowell · Jul 23, 2021 · Jul 22, 2021 · Jul 22, 2021
diff --git a/Bugsnag.xcodeproj/project.pbxproj b/Bugsnag.xcodeproj/project.pbxproj
@@ -171,9 +171,9 @@
 		008967A82486D43700DC48C2 /* KSCrashIdentifierTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 008966E92486D43700DC48C2 /* KSCrashIdentifierTests.m */; };
 		008967A92486D43700DC48C2 /* KSCrashIdentifierTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 008966E92486D43700DC48C2 /* KSCrashIdentifierTests.m */; };
 		008967AA2486D43700DC48C2 /* KSCrashIdentifierTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 008966E92486D43700DC48C2 /* KSCrashIdentifierTests.m */; };
-		008967AB2486D43700DC48C2 /* KSMach_Tests.m in Sources */ = {isa = PBXBuildFile; fileRef = 008966EA2486D43700DC48C2 /* KSMach_Tests.m */; };
-		008967AC2486D43700DC48C2 /* KSMach_Tests.m in Sources */ = {isa = PBXBuildFile; fileRef = 008966EA2486D43700DC48C2 /* KSMach_Tests.m */; };
-		008967AD2486D43700DC48C2 /* KSMach_Tests.m in Sources */ = {isa = PBXBuildFile; fileRef = 008966EA2486D43700DC48C2 /* KSMach_Tests.m */; };
+		008967AB2486D43700DC48C2 /* BSG_KSMachTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 008966EA2486D43700DC48C2 /* BSG_KSMachTests.m */; };
+		008967AC2486D43700DC48C2 /* BSG_KSMachTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 008966EA2486D43700DC48C2 /* BSG_KSMachTests.m */; };
+		008967AD2486D43700DC48C2 /* BSG_KSMachTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 008966EA2486D43700DC48C2 /* BSG_KSMachTests.m */; };
 		008967B42486D9D800DC48C2 /* BugsnagBreadcrumbs.m in Sources */ = {isa = PBXBuildFile; fileRef = 008967B22486D9D700DC48C2 /* BugsnagBreadcrumbs.m */; };
 		008967B52486D9D800DC48C2 /* BugsnagBreadcrumbs.m in Sources */ = {isa = PBXBuildFile; fileRef = 008967B22486D9D700DC48C2 /* BugsnagBreadcrumbs.m */; };
 		008967B62486D9D800DC48C2 /* BugsnagBreadcrumbs.m in Sources */ = {isa = PBXBuildFile; fileRef = 008967B22486D9D700DC48C2 /* BugsnagBreadcrumbs.m */; };
@@ -1132,7 +1132,7 @@
 		008966E72486D43700DC48C2 /* KSCrashSentry_Signal_Tests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = KSCrashSentry_Signal_Tests.m; sourceTree = "<group>"; };
 		008966E82486D43700DC48C2 /* KSString_Tests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = KSString_Tests.m; sourceTree = "<group>"; };
 		008966E92486D43700DC48C2 /* KSCrashIdentifierTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = KSCrashIdentifierTests.m; sourceTree = "<group>"; };
-		008966EA2486D43700DC48C2 /* KSMach_Tests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = KSMach_Tests.m; sourceTree = "<group>"; };
+		008966EA2486D43700DC48C2 /* BSG_KSMachTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = BSG_KSMachTests.m; sourceTree = "<group>"; };
 		008967AE2486D6E100DC48C2 /* Makefile */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; path = Makefile; sourceTree = SOURCE_ROOT; };
 		008967B22486D9D700DC48C2 /* BugsnagBreadcrumbs.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = BugsnagBreadcrumbs.m; sourceTree = "<group>"; };
 		008967B32486D9D700DC48C2 /* BugsnagBreadcrumbs.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = BugsnagBreadcrumbs.h; sourceTree = "<group>"; };
@@ -1478,6 +1478,8 @@
 		008966D32486D43700DC48C2 /* KSCrash */ = {
 			isa = PBXGroup;
 			children = (
+				008966D82486D43700DC48C2 /* BSG_KSMachHeadersTests.m */,
+				008966EA2486D43700DC48C2 /* BSG_KSMachTests.m */,
 				008966D62486D43700DC48C2 /* FileBasedTestCase.h */,
 				008966E42486D43700DC48C2 /* FileBasedTestCase.m */,
 				008966E92486D43700DC48C2 /* KSCrashIdentifierTests.m */,
@@ -1489,8 +1491,6 @@
 				008966E52486D43700DC48C2 /* KSFileUtils_Tests.m */,
 				008966E02486D43700DC48C2 /* KSJSONCodec_Tests.m */,
 				008966DA2486D43700DC48C2 /* KSLogger_Tests.m */,
-				008966EA2486D43700DC48C2 /* KSMach_Tests.m */,
-				008966D82486D43700DC48C2 /* BSG_KSMachHeadersTests.m */,
 				008966E12486D43700DC48C2 /* KSSignalInfo_Tests.m */,
 				008966E82486D43700DC48C2 /* KSString_Tests.m */,
 				008966D52486D43700DC48C2 /* KSSysCtl_Tests.m */,
@@ -2704,7 +2704,7 @@
 				008967542486D43700DC48C2 /* BugsnagOnCrashTest.m in Sources */,
 				008967152486D43700DC48C2 /* BugsnagCollectionsTests.m in Sources */,
 				01E8765E256684E700F4B70A /* URLSessionMock.m in Sources */,
-				008967AB2486D43700DC48C2 /* KSMach_Tests.m in Sources */,
+				008967AB2486D43700DC48C2 /* BSG_KSMachTests.m in Sources */,
 				0089672A2486D43700DC48C2 /* BugsnagStacktraceTest.m in Sources */,
 				01847DAC26441A5E00ADA4C7 /* BSGInternalErrorReporterTests.m in Sources */,
 				0163BF5925823D8D008DC28B /* NotificationBreadcrumbTests.m in Sources */,
@@ -2858,7 +2858,7 @@
 				008967012486D43700DC48C2 /* BugsnagEventPersistLoadTest.m in Sources */,
 				008967702486D43700DC48C2 /* NSError+SimpleConstructor_Tests.m in Sources */,
 				0089671C2486D43700DC48C2 /* BugsnagSessionTest.m in Sources */,
-				008967AC2486D43700DC48C2 /* KSMach_Tests.m in Sources */,
+				008967AC2486D43700DC48C2 /* BSG_KSMachTests.m in Sources */,
 				0163BF5A25823D8D008DC28B /* NotificationBreadcrumbTests.m in Sources */,
 				01BDB1FD25DEBFB300A91FAF /* BSGEventUploadKSCrashReportOperationTests.m in Sources */,
 				00896A452486DBF000DC48C2 /* BugsnagConfigurationTests.m in Sources */,
@@ -3022,7 +3022,7 @@
 				008967022486D43700DC48C2 /* BugsnagEventPersistLoadTest.m in Sources */,
 				008967712486D43700DC48C2 /* NSError+SimpleConstructor_Tests.m in Sources */,
 				0089671D2486D43700DC48C2 /* BugsnagSessionTest.m in Sources */,
-				008967AD2486D43700DC48C2 /* KSMach_Tests.m in Sources */,
+				008967AD2486D43700DC48C2 /* BSG_KSMachTests.m in Sources */,
 				00896A462486DBF000DC48C2 /* BugsnagConfigurationTests.m in Sources */,
 				0089674A2486D43700DC48C2 /* BugsnagUserTest.m in Sources */,
 				0089673B2486D43700DC48C2 /* BugsnagEventFromKSCrashReportTest.m in Sources */,

diff --git a/Bugsnag.xcodeproj/xcshareddata/xcschemes/Bugsnag-iOS.xcscheme b/Bugsnag.xcodeproj/xcshareddata/xcschemes/Bugsnag-iOS.xcscheme
@@ -28,6 +28,13 @@
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
       shouldUseLaunchSchemeArgsEnv = "YES"
       codeCoverageEnabled = "YES">
+      <AdditionalOptions>
+         <AdditionalOption
+            key = "MallocScribble"
+            value = ""
+            isEnabled = "YES">
+         </AdditionalOption>
+      </AdditionalOptions>
       <Testables>
          <TestableReference
             skipped = "NO">

diff --git a/Bugsnag.xcodeproj/xcshareddata/xcschemes/Bugsnag-macOS.xcscheme b/Bugsnag.xcodeproj/xcshareddata/xcschemes/Bugsnag-macOS.xcscheme
@@ -28,6 +28,13 @@
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
       shouldUseLaunchSchemeArgsEnv = "YES"
       codeCoverageEnabled = "YES">
+      <AdditionalOptions>
+         <AdditionalOption
+            key = "MallocScribble"
+            value = ""
+            isEnabled = "YES">
+         </AdditionalOption>
+      </AdditionalOptions>
       <Testables>
          <TestableReference
             skipped = "NO">

diff --git a/Bugsnag.xcodeproj/xcshareddata/xcschemes/Bugsnag-tvOS.xcscheme b/Bugsnag.xcodeproj/xcshareddata/xcschemes/Bugsnag-tvOS.xcscheme
@@ -28,6 +28,13 @@
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
       shouldUseLaunchSchemeArgsEnv = "YES"
       codeCoverageEnabled = "YES">
+      <AdditionalOptions>
+         <AdditionalOption
+            key = "MallocScribble"
+            value = ""
+            isEnabled = "YES">
+         </AdditionalOption>
+      </AdditionalOptions>
       <Testables>
          <TestableReference
             skipped = "NO">

@@ -326,31 +326,41 @@ bool bsg_ksmachgetThreadQueueName(const thread_t thread, char *const buffer,
         return false;
     }
 
-    const char *queue_name = dispatch_queue_get_label(dispatch_queue);
-    if (queue_name == NULL) {
+    // If the thread is being / has recently been destroyed, we can end up with
+    // pointers to deallocated memory. Using vm_read_overwrite allows us to
+    // avoid crashing in this scenario, but we need to check that the copied
+    // data looks like a valid string.
+    const void *ptr = dispatch_queue_get_label(dispatch_queue);
+
+    vm_size_t bytesRead = 0;
+    // Not using bsg_ksmachcopyMem because it does not return bytesRead
+    kr = vm_read_overwrite(mach_task_self(),
+                           (vm_address_t)ptr, (vm_size_t)bufLength - 1,
+                           (vm_address_t)buffer, &bytesRead);
+    if (kr != KERN_SUCCESS) {
         BSG_KSLOG_TRACE("Error while getting dispatch queue name : %p",
                         dispatch_queue);
+        buffer[0] = '\0';
         return false;
     }
-    BSG_KSLOG_TRACE("Dispatch queue name: %s", queue_name);
-    size_t length = strlen(queue_name);
+
+    buffer[bytesRead] = '\0';
+
+    BSG_KSLOG_TRACE("Dispatch queue name: %s", buffer);
 
     // Queue label must be a null terminated string.
     size_t iLabel;
-    for (iLabel = 0; iLabel < length + 1; iLabel++) {
-        if (queue_name[iLabel] < ' ' || queue_name[iLabel] > '~') {
+    for (iLabel = 0; iLabel < bytesRead; iLabel++) {
+        if (buffer[iLabel] < ' ' || buffer[iLabel] > '~') {
             break;
         }
     }
-    if (queue_name[iLabel] != 0) {
+    if (buffer[iLabel] != 0) {
         // Found a non-null, invalid char.
         BSG_KSLOG_TRACE("Queue label contains invalid chars");
+        buffer[0] = '\0';
         return false;
     }
-    bufLength =
-        MIN(length, bufLength - 1); // just strlen, without null-terminator
-    strncpy(buffer, queue_name, bufLength);
-    buffer[bufLength] = 0; // terminate string
     BSG_KSLOG_TRACE("Queue label = %s", buffer);
     return true;
 }

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,13 @@
 Changelog
 =========
 
+## TBD
+
+### Bug fixes
+
+* Fix another rare crash in `bsg_ksmachgetThreadQueueName`.
+  [#1157](https://github.com/bugsnag/bugsnag-cocoa/pull/1157)
+
 ## 6.10.2 (2021-07-14)
 
 ### Bug fixes

diff --git a/Tests/KSCrash/KSMach_Tests.m → Tests/KSCrash/BSG_KSMachTests.m b/Tests/KSCrash/KSMach_Tests.m → Tests/KSCrash/BSG_KSMachTests.m
@@ -54,6 +54,13 @@ - (void) main
 @end
 
 
+void * executeBlock(void *ptr)
+{
+    ((__bridge_transfer dispatch_block_t)ptr)();
+    return NULL;
+}
+
+
 @interface bsg_ksmachTests : XCTestCase @end
 
 @implementation bsg_ksmachTests
@@ -185,37 +192,87 @@ - (void) testTimeDifferenceInSeconds
     XCTAssertEqualWithAccuracy(diff, cfDiff, 0.001);
 }
 
-// TODO: Disabling this until I figure out what's wrong with queue names.
-//- (void) testGetQueueName
-//{
-//    kern_return_t kr;
-//    const task_t thisTask = mach_task_self();
-//    thread_act_array_t threads;
-//    mach_msg_type_number_t numThreads;
-//    
-//    kr = task_threads(thisTask, &threads, &numThreads);
-//    XCTAssertTrue(kr == KERN_SUCCESS, @"");
-//    
-//    bool success = false;
-//    char buffer[100];
-//    for(mach_msg_type_number_t i = 0; i < numThreads; i++)
-//    {
-//        thread_t thread = threads[i];
-//        if(bsg_ksmachgetThreadQueueName(thread, buffer, sizeof(buffer)))
-//        {
-//            success = true;
-//            break;
-//        }
-//    }
-//    
-//    for(mach_msg_type_number_t i = 0; i < numThreads; i++)
-//    {
-//        mach_port_deallocate(thisTask, threads[i]);
-//    }
-//    vm_deallocate(thisTask, (vm_address_t)threads, sizeof(thread_t) * numThreads);
-//    
-//    XCTAssertTrue(success, @"");
-//}
+- (void) testGetQueueNameWithMainThread
+{
+    char name[32] = "";
+
+    XCTAssertTrue(bsg_ksmachgetThreadQueueName(bsg_ksmachthread_self(),
+                                               name, sizeof(name)));
+
+    XCTAssertEqualObjects(@(name), @"com.apple.main-thread");
+}
+
+- (void) testGetQueueNameWithNonDispatchThread
+{
+    pthread_t thread;
+
+    pthread_create(&thread, NULL, executeBlock,
+                   (__bridge_retained void *)^{
+        char name[32] = "";
+
+        XCTAssertFalse(bsg_ksmachgetThreadQueueName(bsg_ksmachthread_self(),
+                                                    name, sizeof(name)));
+    });
+
+    pthread_join(thread, NULL);
+}
+
+- (void) testGetQueueNameWithInvalidThread
+{
+    char name[32] = "";
+
+    XCTAssertFalse(bsg_ksmachgetThreadQueueName(0xdeadbeef,
+                                                name, sizeof(name)));
+}
+
+- (void) testGetQueueNameWithEphemeralThreads
+{
+    //
+    // This test aims to trigger potential crashes when getting the queue name
+    // of dying / recently deceased threads.
+    //
+    // For more effective detection of bugs, enable "Malloc Scribble" under the
+    // Scheme's Diagnostics options, or set the "MallocScribble" environment
+    // variable.
+    //
+
+    __block int finished = 0;
+
+    dispatch_async(dispatch_queue_create(NULL, 0), ^{
+        dispatch_group_t group = dispatch_group_create();
+
+        for (int i = 0; i < 100000; i++) {
+            char *label = NULL;
+            asprintf(&label, "%d", i);
+            dispatch_group_async(group, dispatch_queue_create(label, 0), ^{
+                usleep(10);
+            });
+            free(label);
+        }
+
+        dispatch_group_notify(group, dispatch_queue_create(NULL, 0), ^{
+            finished = 1;
+        });
+    });
+
+    while (!finished) {
+        char name[5];
+        thread_act_array_t threads;
+        mach_msg_type_number_t i, threadCount = 0;
+        task_threads(mach_task_self(), &threads, &threadCount);
+
+        for (i = 0; i < threadCount; i++) {
+            bzero(name, sizeof(name));
+            if (bsg_ksmachgetThreadQueueName(threads[i], name, sizeof(name))) {
+                XCTAssertEqual(name[sizeof(name) - 1], '\0',
+                               @"queue name must be NULL terminated");
+            }
+        }
+
+        vm_deallocate(mach_task_self(), (vm_address_t)threads,
+                      sizeof(threads[0]) * threadCount);
+    }
+}
 
 - (void) testThreadState
 {