Fix potential deadlock when generating a crash report

.github/PULL_REQUEST_TEMPLATE.md

@@ -37,6 +37,7 @@ Related to #
 
 * Added a changelog entry?
 * Checked the scope to ensure the commits are only related to the goal above?
+* Added any new headers to the "Copy Files" stage of the static iOS target?
 
 -->
 

CHANGELOG.md

@@ -1,6 +1,14 @@
 Changelog
 =========
 
+## TBD
+
+## Bug Fixes
+
+* Fixed an issue where an app could deadlock during a crash if unfavourable 
+  timing caused DYLD lock contention.
+  [#580](https://github.com/bugsnag/bugsnag-cocoa/pull/580)
+
 ## 5.23.1 (2020-04-08)
 
 ## Bug fixes

OSX/Bugsnag.xcodeproj/project.pbxproj

@@ -7,6 +7,8 @@
 	objects = {
 
 /* Begin PBXBuildFile section */
+		0071CB53246074BD00F562B1 /* BSG_KSMachHeaders.m in Sources */ = {isa = PBXBuildFile; fileRef = 0071CB51246074BD00F562B1 /* BSG_KSMachHeaders.m */; };
+		0071CB54246074BD00F562B1 /* BSG_KSMachHeaders.h in Headers */ = {isa = PBXBuildFile; fileRef = 0071CB52246074BD00F562B1 /* BSG_KSMachHeaders.h */; };
 		4B406C1822CAD96400464D1D /* BugsnagCollectionsBSGDictMergeTest.m in Sources */ = {isa = PBXBuildFile; fileRef = 4B406C1622CAD96400464D1D /* BugsnagCollectionsBSGDictMergeTest.m */; };
 		4B406C1922CAD96400464D1D /* BugsnagCollectionsBSGDictSetSafeObjectTest.m in Sources */ = {isa = PBXBuildFile; fileRef = 4B406C1722CAD96400464D1D /* BugsnagCollectionsBSGDictSetSafeObjectTest.m */; };
 		4B775FD422CBE02F004839C5 /* BugsnagCollectionsBSGDictInsertIfNotNilTest.m in Sources */ = {isa = PBXBuildFile; fileRef = 4B775FD222CBE02A004839C5 /* BugsnagCollectionsBSGDictInsertIfNotNilTest.m */; };
@@ -183,6 +185,8 @@
 /* End PBXContainerItemProxy section */
 
 /* Begin PBXFileReference section */
+		0071CB51246074BD00F562B1 /* BSG_KSMachHeaders.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = BSG_KSMachHeaders.m; sourceTree = "<group>"; };
+		0071CB52246074BD00F562B1 /* BSG_KSMachHeaders.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = BSG_KSMachHeaders.h; sourceTree = "<group>"; };
 		4B406C1622CAD96400464D1D /* BugsnagCollectionsBSGDictMergeTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = BugsnagCollectionsBSGDictMergeTest.m; sourceTree = "<group>"; };
 		4B406C1722CAD96400464D1D /* BugsnagCollectionsBSGDictSetSafeObjectTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = BugsnagCollectionsBSGDictSetSafeObjectTest.m; sourceTree = "<group>"; };
 		4B775FD222CBE02A004839C5 /* BugsnagCollectionsBSGDictInsertIfNotNilTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = BugsnagCollectionsBSGDictInsertIfNotNilTest.m; sourceTree = "<group>"; };
@@ -582,6 +586,8 @@
 		E79E6B411F4E3850002B35F9 /* Tools */ = {
 			isa = PBXGroup;
 			children = (
+				0071CB52246074BD00F562B1 /* BSG_KSMachHeaders.h */,
+				0071CB51246074BD00F562B1 /* BSG_KSMachHeaders.m */,
 				E79E6B421F4E3850002B35F9 /* BSG_KSArchSpecific.h */,
 				E79E6B431F4E3850002B35F9 /* BSG_KSBacktrace.c */,
 				E79E6B441F4E3850002B35F9 /* BSG_KSBacktrace.h */,
@@ -720,6 +726,7 @@
 				E79E6B9C1F4E3850002B35F9 /* BSG_KSSystemInfo.h in Headers */,
 				8A2C8FD71C6BC2C800846019 /* BugsnagMetaData.h in Headers */,
 				E79E6B021F4E3847002B35F9 /* BugsnagCrashSentry.h in Headers */,
+				0071CB54246074BD00F562B1 /* BSG_KSMachHeaders.h in Headers */,
 				E79E6B911F4E3850002B35F9 /* BSG_KSCrashReport.h in Headers */,
 				8A48EF271EAA805D00B70024 /* BugsnagLogger.h in Headers */,
 				E79E6BCC1F4E3850002B35F9 /* BSG_KSSingleton.h in Headers */,
@@ -895,6 +902,7 @@
 				E79E6BA81F4E3850002B35F9 /* BSG_KSCrashSentry_NSException.m in Sources */,
 				E79E6B901F4E3850002B35F9 /* BSG_KSCrashReport.c in Sources */,
 				8A2C8FD81C6BC2C800846019 /* BugsnagMetaData.m in Sources */,
+				0071CB53246074BD00F562B1 /* BSG_KSMachHeaders.m in Sources */,
 				E79E6BA51F4E3850002B35F9 /* BSG_KSCrashSentry_MachException.c in Sources */,
 				E79E6BB41F4E3850002B35F9 /* BSG_KSDynamicLinker.c in Sources */,
 				E79E6B031F4E3847002B35F9 /* BugsnagCrashSentry.m in Sources */,

Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m

@@ -24,13 +24,15 @@
 // THE SOFTWARE.
 //
 
+#import <mach-o/dyld.h>
 #import "BSG_KSCrashAdvanced.h"
 
 #import "BSG_KSCrashC.h"
 #import "BSG_KSCrashCallCompletion.h"
 #import "BSG_KSJSONCodecObjC.h"
 #import "BSG_KSSingleton.h"
 #import "BSG_KSSystemCapabilities.h"
+#import "BSG_KSMachHeaders.h"
 #import "NSError+BSG_SimpleConstructor.h"
 
 //#define BSG_KSLogger_LocalLevel TRACE
@@ -49,6 +51,10 @@
 #define BSG_KSCRASH_DefaultReportFilesDirectory @"KSCrashReports"
 #endif
 
+#ifndef BSG_INITIAL_MACH_BINARY_IMAGE_ARRAY_SIZE
+#define BSG_INITIAL_MACH_BINARY_IMAGE_ARRAY_SIZE 400
+#endif
+
 // ============================================================================
 #pragma mark - Constants -
 // ============================================================================
@@ -219,13 +225,16 @@ - (NSString *)stateFilePath {
 }
 
 - (BOOL)install {
+    // Maintain a cache of info about dynamically loaded binary images
+    [self listenForLoadedBinaries];
+
     _handlingCrashTypes = bsg_kscrash_install(
         [self.crashReportPath UTF8String], [self.recrashReportPath UTF8String],
         [self.stateFilePath UTF8String], [self.nextCrashID UTF8String]);
     if (self.handlingCrashTypes == 0) {
         return false;
     }
-
+    
 #if BSG_KSCRASH_HAS_UIKIT
     NSNotificationCenter *nCenter = [NSNotificationCenter defaultCenter];
     [nCenter addObserver:self
@@ -253,6 +262,21 @@ - (BOOL)install {
     return true;
 }
 
+/**
+ * Set up listeners for un/loaded frameworks.  Maintaining our own list of framework Mach
+ * headers means that we avoid potential deadlock situations where we try and suspend
+ * lock-holding threads prior to loading mach headers as part of our normal event handling
+ * behaviour.
+ */
+- (void)listenForLoadedBinaries {
+    bsg_initialise_mach_binary_headers(BSG_INITIAL_MACH_BINARY_IMAGE_ARRAY_SIZE);
+
+    // Note: Internally, access to DYLD's binary image store is guarded by an OSSpinLock.  We therefore don't need to
+    // add additional guards around our access.
+    _dyld_register_func_for_remove_image(&bsg_mach_binary_image_removed);
+    _dyld_register_func_for_add_image(&bsg_mach_binary_image_added);
+}
+
 - (void)sendAllReportsWithCompletion:
     (BSG_KSCrashReportFilterCompletion)onCompletion {
     [self.crashReportStore pruneFilesLeaving:self.maxStoredReports];

Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c

@@ -36,6 +36,7 @@
 #include "BSG_KSObjC.h"
 #include "BSG_KSSignalInfo.h"
 #include "BSG_KSString.h"
+#include "BSG_KSMachHeaders.h"
 
 //#define BSG_kSLogger_LocalLevel TRACE
 #include "BSG_KSLogger.h"
@@ -1084,70 +1085,21 @@ int bsg_kscrw_i_threadIndex(const thread_t thread) {
  *
  * @param key The object key, if needed.
  *
- * @param index Which image to write about.
+ * @param info Cached info about the binary image.
  */
 void bsg_kscrw_i_writeBinaryImage(const BSG_KSCrashReportWriter *const writer,
-                                  const char *const key, const uint32_t index) {
-    const struct mach_header *header = _dyld_get_image_header(index);
-    if (header == NULL) {
-        return;
-    }
-
-    uintptr_t cmdPtr = bsg_ksdlfirstCmdAfterHeader(header);
-    if (cmdPtr == 0) {
-        return;
-    }
-
-    // Look for the TEXT segment to get the image size.
-    // Also look for a UUID command.
-    uint64_t imageSize = 0;
-    uint64_t imageVmAddr = 0;
-    uint8_t *uuid = NULL;
-
-    for (uint32_t iCmd = 0; iCmd < header->ncmds; iCmd++) {
-        struct load_command *loadCmd = (struct load_command *)cmdPtr;
-        switch (loadCmd->cmd) {
-        case LC_SEGMENT: {
-            struct segment_command *segCmd = (struct segment_command *)cmdPtr;
-            if (strcmp(segCmd->segname, SEG_TEXT) == 0) {
-                imageSize = segCmd->vmsize;
-                imageVmAddr = segCmd->vmaddr;
-            }
-            break;
-        }
-        case LC_SEGMENT_64: {
-            struct segment_command_64 *segCmd =
-                (struct segment_command_64 *)cmdPtr;
-            if (strcmp(segCmd->segname, SEG_TEXT) == 0) {
-                imageSize = segCmd->vmsize;
-                imageVmAddr = segCmd->vmaddr;
-            }
-            break;
-        }
-        case LC_UUID: {
-            struct uuid_command *uuidCmd = (struct uuid_command *)cmdPtr;
-            uuid = uuidCmd->uuid;
-            break;
-        }
-        }
-        cmdPtr += loadCmd->cmdsize;
-    }
-
+                                  const char *const key,
+                                  const BSG_Mach_Binary_Image_Info *info)
+{
     writer->beginObject(writer, key);
     {
-        writer->addUIntegerElement(writer, BSG_KSCrashField_ImageAddress,
-                                   (uintptr_t)header);
-        writer->addUIntegerElement(writer, BSG_KSCrashField_ImageVmAddress,
-                                   imageVmAddr);
-        writer->addUIntegerElement(writer, BSG_KSCrashField_ImageSize,
-                                   imageSize);
-        writer->addStringElement(writer, BSG_KSCrashField_Name,
-                                 _dyld_get_image_name(index));
-        writer->addUUIDElement(writer, BSG_KSCrashField_UUID, uuid);
-        writer->addIntegerElement(writer, BSG_KSCrashField_CPUType,
-                                  header->cputype);
-        writer->addIntegerElement(writer, BSG_KSCrashField_CPUSubType,
-                                  header->cpusubtype);
+        writer->addUIntegerElement(writer, BSG_KSCrashField_ImageAddress, (uintptr_t)info->mh);
+        writer->addUIntegerElement(writer, BSG_KSCrashField_ImageVmAddress,          info->imageVmAddr);
+        writer->addUIntegerElement(writer, BSG_KSCrashField_ImageSize,               info->imageSize);
+        writer->addStringElement(writer, BSG_KSCrashField_Name,                      info->name);
+        writer->addUUIDElement(writer, BSG_KSCrashField_UUID,                        info->uuid);
+        writer->addIntegerElement(writer, BSG_KSCrashField_CPUType,                  info->cputype);
+        writer->addIntegerElement(writer, BSG_KSCrashField_CPUSubType,               info->cpusubtype);
     }
     writer->endContainer(writer);
 }
@@ -1159,13 +1111,16 @@ void bsg_kscrw_i_writeBinaryImage(const BSG_KSCrashReportWriter *const writer,
  * @param key The object key, if needed.
  */
 void bsg_kscrw_i_writeBinaryImages(const BSG_KSCrashReportWriter *const writer,
-                                   const char *const key) {
-    const uint32_t imageCount = _dyld_image_count();
-
+                                   const char *const key)
+{
+    const uint32_t imageCount = (uint32_t)bsg_dyld_image_count();
+
     writer->beginArray(writer, key);
     {
-        for (uint32_t iImg = 0; iImg < imageCount; iImg++) {
-            bsg_kscrw_i_writeBinaryImage(writer, NULL, iImg);
+        for (uint32_t i = 0; i < imageCount; i++) {
+            // Threads are suspended at this point; no need to synchronise/lock
+            BSG_Mach_Binary_Image_Info *info = bsg_dyld_get_image_info(i);
+            bsg_kscrw_i_writeBinaryImage(writer, NULL, info);
         }
     }
     writer->endContainer(writer);

Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.c

@@ -26,17 +26,18 @@
 
 #include "BSG_KSDynamicLinker.h"
 #include "BSG_KSArchSpecific.h"
+#include "BSG_KSMachHeaders.h"
 
 #include <limits.h>
 #include <mach-o/nlist.h>
 #include <string.h>
 
 uint32_t bsg_ksdlimageNamed(const char *const imageName, bool exactMatch) {
     if (imageName != NULL) {
-        const uint32_t imageCount = _dyld_image_count();
-
+        const size_t imageCount = bsg_dyld_image_count();
+        
         for (uint32_t iImg = 0; iImg < imageCount; iImg++) {
-            const char *name = _dyld_get_image_name(iImg);
+            const char *name = bsg_dyld_get_image_name(iImg);
             if (name == NULL) {
                 continue; // name is null if the index is out of range per dyld(3)
             } else if (exactMatch) {
@@ -57,7 +58,7 @@ const uint8_t *bsg_ksdlimageUUID(const char *const imageName, bool exactMatch) {
     if (imageName != NULL) {
         const uint32_t iImg = bsg_ksdlimageNamed(imageName, exactMatch);
         if (iImg != UINT32_MAX) {
-            const struct mach_header *header = _dyld_get_image_header(iImg);
+            const struct mach_header *header =  bsg_dyld_get_image_header(iImg);
             if (header != NULL) {
                 uintptr_t cmdPtr = bsg_ksdlfirstCmdAfterHeader(header);
                 if (cmdPtr != 0) {
@@ -96,15 +97,15 @@ uintptr_t bsg_ksdlfirstCmdAfterHeader(const struct mach_header *const header) {
 }
 
 uint32_t bsg_ksdlimageIndexContainingAddress(const uintptr_t address) {
-    const uint32_t imageCount = _dyld_image_count();
+    const size_t imageCount = bsg_dyld_image_count();
     const struct mach_header *header = 0;
 
     for (uint32_t iImg = 0; iImg < imageCount; iImg++) {
-        header = _dyld_get_image_header(iImg);
+        header = bsg_dyld_get_image_header(iImg);
         if (header != NULL) {
             // Look for a segment command with this address within its range.
             uintptr_t addressWSlide =
-                address - (uintptr_t)_dyld_get_image_vmaddr_slide(iImg);
+                address - bsg_dyld_get_image_vmaddr_slide(iImg);
             uintptr_t cmdPtr = bsg_ksdlfirstCmdAfterHeader(header);
             if (cmdPtr == 0) {
                 continue;
@@ -115,16 +116,20 @@ uint32_t bsg_ksdlimageIndexContainingAddress(const uintptr_t address) {
                 if (loadCmd->cmd == LC_SEGMENT) {
                     const struct segment_command *segCmd =
                         (struct segment_command *)cmdPtr;
-                    if (addressWSlide >= segCmd->vmaddr &&
-                        addressWSlide < segCmd->vmaddr + segCmd->vmsize) {
-                        return iImg;
+                    if ( strcmp(segCmd->segname, "__TEXT") == 0 ) {
+                        if (addressWSlide >= segCmd->vmaddr &&
+                            addressWSlide < segCmd->vmaddr + segCmd->vmsize) {
+                            return iImg;
+                        }
                     }
                 } else if (loadCmd->cmd == LC_SEGMENT_64) {
                     const struct segment_command_64 *segCmd =
                         (struct segment_command_64 *)cmdPtr;
-                    if (addressWSlide >= segCmd->vmaddr &&
-                        addressWSlide < segCmd->vmaddr + segCmd->vmsize) {
-                        return iImg;
+                    if ( strcmp(segCmd->segname, "__TEXT") == 0 ) {
+                        if (addressWSlide >= segCmd->vmaddr &&
+                            addressWSlide < segCmd->vmaddr + segCmd->vmsize) {
+                            return iImg;
+                        }
                     }
                 }
                 cmdPtr += loadCmd->cmdsize;
@@ -135,7 +140,7 @@ uint32_t bsg_ksdlimageIndexContainingAddress(const uintptr_t address) {
 }
 
 uintptr_t bsg_ksdlsegmentBaseOfImageIndex(const uint32_t idx) {
-    const struct mach_header *header = _dyld_get_image_header(idx);
+    const struct mach_header *header =  bsg_dyld_get_image_header(idx);
     if (header == NULL) {
         return 0;
     }
@@ -176,20 +181,21 @@ bool bsg_ksdldladdr(const uintptr_t address, Dl_info *const info) {
     if (idx == UINT_MAX) {
         return false;
     }
-    const struct mach_header *header = _dyld_get_image_header(idx);
+
+    const struct mach_header *header = bsg_dyld_get_image_header(idx);
     if (header == NULL) {
         return false;
     }
     const uintptr_t imageVMAddrSlide =
-        (uintptr_t)_dyld_get_image_vmaddr_slide(idx);
+        bsg_dyld_get_image_vmaddr_slide(idx);
     const uintptr_t addressWithSlide = address - imageVMAddrSlide;
     const uintptr_t segmentBase =
         bsg_ksdlsegmentBaseOfImageIndex(idx) + imageVMAddrSlide;
     if (segmentBase == 0) {
         return false;
     }
 
-    info->dli_fname = _dyld_get_image_name(idx);
+    info->dli_fname = bsg_dyld_get_image_name(idx);
     info->dli_fbase = (void *)header;
 
     // Find symbol tables and get whichever symbol is closest to the address.
@@ -244,12 +250,12 @@ bool bsg_ksdldladdr(const uintptr_t address, Dl_info *const info) {
 
 const void *bsg_ksdlgetSymbolAddrInImage(uint32_t imageIdx,
                                          const char *symbolName) {
-    const struct mach_header *header = _dyld_get_image_header(imageIdx);
+    const struct mach_header *header = bsg_dyld_get_image_header(imageIdx);
     if (header == NULL) {
         return NULL;
     }
     const uintptr_t imageVMAddrSlide =
-        (uintptr_t)_dyld_get_image_vmaddr_slide(imageIdx);
+        bsg_dyld_get_image_vmaddr_slide(imageIdx);
     const uintptr_t segmentBase =
         bsg_ksdlsegmentBaseOfImageIndex(imageIdx) + imageVMAddrSlide;
     if (segmentBase == 0) {
@@ -290,7 +296,7 @@ const void *bsg_ksdlgetSymbolAddrInImage(uint32_t imageIdx,
 }
 
 const void *bsg_ksdlgetSymbolAddrInAnyImage(const char *symbolName) {
-    const uint32_t imageCount = _dyld_image_count();
+    const size_t imageCount = bsg_dyld_image_count();
 
     for (uint32_t iImg = 0; iImg < imageCount; iImg++) {
         const void *symbolAddr = bsg_ksdlgetSymbolAddrInImage(iImg, symbolName);