Fix potential deadlock when generating a crash report

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 100
+#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,14 @@ 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)
+{
     writer->beginArray(writer, key);
     {
-        for (uint32_t iImg = 0; iImg < imageCount; iImg++) {
-            bsg_kscrw_i_writeBinaryImage(writer, NULL, iImg);
+        BSG_Mach_Binary_Images *images = bsg_get_mach_binary_images();
+        for (uint32_t i = 0; i < images->used; i++) {
+            // Threads are suspended at this point; no need to synchronise/lock
+            bsg_kscrw_i_writeBinaryImage(writer, NULL, &(images->contents[i]));
         }
     }
     writer->endContainer(writer);

Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h

@@ -0,0 +1,66 @@
+//
+//  BSG_KSMachHeaders.h
+//  Bugsnag
+//
+//  Created by Robin Macharg on 04/05/2020.
+//  Copyright © 2020 Bugsnag. All rights reserved.
+//
+
+#ifndef BSG_KSMachHeaders_h
+#define BSG_KSMachHeaders_h
+
+#import <mach/machine.h>
+#import <os/lock.h>
+
+/**
+ * An encapsulation of the Mach header - either 64 or 32 bit, along with some additional information required for
+ * detailing a crash report's binary images.
+ */
+typedef struct {
+    const struct mach_header *mh;      /* The mach_header - 32 or 64 bit */
+    uint64_t imageVmAddr;
+    uint64_t imageSize;
+    uint8_t *uuid;
+    const char* name;
+    cpu_type_t cputype;          /* cpu specifier */
+    cpu_subtype_t cpusubtype;    /* machine specifier */
+} BSG_Mach_Binary_Image_Info;
+
+/**
+ * MARK: - A Dynamic array container
+ * See: https://stackoverflow.com/a/3536261/2431627
+ */
+typedef struct {
+    size_t used;
+    size_t size;
+    BSG_Mach_Binary_Image_Info *contents;
+} BSG_Mach_Binary_Images;
+
+static BSG_Mach_Binary_Images bsg_mach_binary_images;
+
+/**
+ * A lock, used to synchronise access to the array of binary image info.
+ */
+static os_unfair_lock bsg_mach_binary_images_access_lock = OS_UNFAIR_LOCK_INIT;
+
+/**
+ * Provide external access to the array of binary image info
+ */
+BSG_Mach_Binary_Images *bsg_get_mach_binary_images(void);
+
+/**
+ * Called when a binary image is loaded.
+ */
+void bsg_mach_binary_image_added(const struct mach_header *mh, intptr_t slide);
+
+/**
+ * Called when a binary image is unloaded.
+ */
+void bsg_mach_binary_image_removed(const struct mach_header *mh, intptr_t slide);
+
+/**
+ * Create an empty, mutable NSArray to hold Mach header info
+ */
+void bsg_initialise_mach_binary_headers(size_t initialSize);
+
+#endif /* BSG_KSMachHeaders_h */