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?
 
 -->
 

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

@@ -52,7 +52,7 @@
 #endif
 
 #ifndef BSG_INITIAL_MACH_BINARY_IMAGE_ARRAY_SIZE
-#define BSG_INITIAL_MACH_BINARY_IMAGE_ARRAY_SIZE 100
+#define BSG_INITIAL_MACH_BINARY_IMAGE_ARRAY_SIZE 400
 #endif
 
 // ============================================================================

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

@@ -10,7 +10,7 @@
 #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
@@ -39,9 +39,16 @@ typedef struct {
 static BSG_Mach_Binary_Images bsg_mach_binary_images;
 
 /**
- * A lock, used to synchronise access to the array of binary image info.
+ * An OS-version-specific 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;
+#if defined(__IPHONE_10_0) || defined(__MAC_10_12) || defined(__TVOS_10_0) || defined(__WATCHOS_3_0)
+    #import <os/lock.h>
+    static os_unfair_lock bsg_mach_binary_images_access_lock = OS_UNFAIR_LOCK_INIT;
+#else
+    #import <libkern/OSAtomic.h>
+    #define OS_SPINLOCK_INIT 0
+    static OSSpinLock bsg_mach_binary_images_access_lock = OS_SPINLOCK_INIT;
+#endif
 
 /**
  * Provide external access to the array of binary image info
@@ -59,7 +66,7 @@ void bsg_mach_binary_image_added(const struct mach_header *mh, intptr_t slide);
 void bsg_mach_binary_image_removed(const struct mach_header *mh, intptr_t slide);
 
 /**
- * Create an empty, mutable NSArray to hold Mach header info
+ * Create an empty array with initial capacity to hold Mach header info.
  */
 void bsg_initialise_mach_binary_headers(size_t initialSize);
 

Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m

@@ -16,40 +16,73 @@
     return &bsg_mach_binary_images;
 }
 
+// MARK: - Code synchronisation wrappers
+
+// os_unfair_lock is available from specific OS versions onwards:
+//     https://developer.apple.com/documentation/os/os_unfair_lock
+//
+// It deprecates OSSpinLock:
+//     https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/spinlock.3.html
+
+void lock() {
+#if defined(__IPHONE_10_0) || defined(__MAC_10_12) || defined(__TVOS_10_0) || defined(__WATCHOS_3_0)
+    os_unfair_lock_lock(&bsg_mach_binary_images_access_lock);
+#else
+    OSSpinLockLock(&bsg_mach_binary_images_access_lock);
+#endif
+}
+
+void unlock() {
+#if defined(__IPHONE_10_0) || defined(__MAC_10_12) || defined(__TVOS_10_0) || defined(__WATCHOS_3_0)
+    os_unfair_lock_unlock(&bsg_mach_binary_images_access_lock);
+#else
+    OSSpinLockUnlock(&bsg_mach_binary_images_access_lock);
+#endif
+}
+
 /**
  * Store a Mach binary image-excapsulating struct in a dynamic array.
  * The array doubles on filling-up.  Typical sizes is expected to be in < 1000 (i.e. 2-3 doublings, at app start-up)
  * This should be called in a threadsafe way; we lock against a simultaneous add and remove.
  */
 void bsg_add_mach_binary_image(BSG_Mach_Binary_Image_Info element) {
 
-    os_unfair_lock_lock(&bsg_mach_binary_images_access_lock);
+    lock();
 
-    // Expand array if necessary
+    // Expand array if necessary.  We're slightly paranoid here.  An OOM is likely to be indicative of bigger problems
+    // but we should still do *our* best not to crash the app.
     if (bsg_mach_binary_images.used == bsg_mach_binary_images.size) {
         bsg_mach_binary_images.size *= 2;
-        bsg_mach_binary_images.contents = (BSG_Mach_Binary_Image_Info *)realloc(bsg_mach_binary_images.contents, bsg_mach_binary_images.size * sizeof(BSG_Mach_Binary_Image_Info));
+        size_t newSize = bsg_mach_binary_images.size * sizeof(BSG_Mach_Binary_Image_Info);
+        void **newData = realloc(bsg_mach_binary_images.contents, newSize);
+
+        // Exit early if we can't allocate memory
+        if (newData == NULL) {
+            return;
+        }
+
+        bsg_mach_binary_images.contents = (BSG_Mach_Binary_Image_Info *)newData;
     }
 
     // Store the value, increment the number of used elements
     bsg_mach_binary_images.contents[bsg_mach_binary_images.used++] = element;
 
-    os_unfair_lock_unlock(&bsg_mach_binary_images_access_lock);
+    unlock();
 }
 
 /**
  * Binary images can only be loaded at most once.  We can use the (file)name as a key, without needing to compare the
  * other fields.  Element order is not important; deletion is accomplished by copying the last item into the deleted
  * position.
  */
-void bsg_remove_mach_binary_image(const char *element_name) {
+void bsg_remove_mach_binary_image(uint64_t imageVmAddr) {
 
-    os_unfair_lock_lock(&bsg_mach_binary_images_access_lock);
+    lock();
 
     for (size_t i=0; i<bsg_mach_binary_images.used; i++) {
         BSG_Mach_Binary_Image_Info item = bsg_mach_binary_images.contents[i];
 
-        if (strcmp(element_name, item.name) == 0) {
+        if (imageVmAddr == item.imageVmAddr) {
             // Note: removal of the last (ith) item involves a redundant copy from last->last.
             if (bsg_mach_binary_images.used >= 2) {
                 bsg_mach_binary_images.contents[i] = bsg_mach_binary_images.contents[--bsg_mach_binary_images.used];
@@ -61,7 +94,7 @@ void bsg_remove_mach_binary_image(const char *element_name) {
         }
     }
 
-    os_unfair_lock_unlock(&bsg_mach_binary_images_access_lock);
+    unlock();
 }
 
 void bsg_initialise_mach_binary_headers(size_t initialSize) {
@@ -165,9 +198,9 @@ void bsg_mach_binary_image_added(const struct mach_header *header, intptr_t slid
  */
 void bsg_mach_binary_image_removed(const struct mach_header *header, intptr_t slide)
 {
-    // Convert header and slide into an info struct
-    BSG_Mach_Binary_Image_Info info;
+    // Convert header into an info struct
+    BSG_Mach_Binary_Image_Info info = { 0 };
     if (bsg_populate_mach_image_info(header, &info)) {
-        bsg_remove_mach_binary_image(info.name);
+        bsg_remove_mach_binary_image(info.imageVmAddr);
     }
 }

Tests/KSCrash/KSMachHeader_Tests.m

@@ -13,7 +13,7 @@
 // Private methods
 void bsg_initialize_binary_images_array(size_t initialSize);
 void bsg_add_mach_binary_image(BSG_Mach_Binary_Image_Info element);
-void bsg_remove_mach_binary_image(const char *element_name);
+void bsg_remove_mach_binary_image(uint64_t imageVmAddr);
 BSG_Mach_Binary_Images *bsg_get_mach_binary_images(void);
 
 const struct mach_header mh = {
@@ -27,9 +27,9 @@
 };
 
 const BSG_Mach_Binary_Image_Info info1 = {.mh = &mh, .imageVmAddr = 12345, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the first", .cputype = 42, .cpusubtype = 27 };
-const BSG_Mach_Binary_Image_Info info2 = {.mh = &mh, .imageVmAddr = 12345, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the second", .cputype = 42, .cpusubtype = 27 };
-const BSG_Mach_Binary_Image_Info info3 = {.mh = &mh, .imageVmAddr = 12345, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the third", .cputype = 42, .cpusubtype = 27 };
-const BSG_Mach_Binary_Image_Info info4 = {.mh = &mh, .imageVmAddr = 12345, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the fourth", .cputype = 42, .cpusubtype = 27 };
+const BSG_Mach_Binary_Image_Info info2 = {.mh = &mh, .imageVmAddr = 23456, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the second", .cputype = 42, .cpusubtype = 27 };
+const BSG_Mach_Binary_Image_Info info3 = {.mh = &mh, .imageVmAddr = 34567, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the third", .cputype = 42, .cpusubtype = 27 };
+const BSG_Mach_Binary_Image_Info info4 = {.mh = &mh, .imageVmAddr = 45678, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the fourth", .cputype = 42, .cpusubtype = 27 };
 
 @interface KSMachHeader_Tests : XCTestCase
 @end
@@ -58,7 +58,7 @@ - (void)testDynamicArray {
     XCTAssertEqual(headers->used, 3);
 
     // Delete - third will be copied
-    bsg_remove_mach_binary_image("header the first");
+    bsg_remove_mach_binary_image(12345);
     XCTAssertEqual(headers->size, 4);
     XCTAssertEqual(headers->used, 2);
 
@@ -67,20 +67,20 @@ - (void)testDynamicArray {
     XCTAssertEqual(headers->size, 4);
 
     // Nothing happens
-    bsg_remove_mach_binary_image("header the first");
+    bsg_remove_mach_binary_image(12345);
     XCTAssertEqual(headers->size, 4);
     XCTAssertEqual(headers->used, 2);
 
-    bsg_remove_mach_binary_image("header the second");
+    bsg_remove_mach_binary_image(23456);
     XCTAssertEqual(headers->size, 4);
     XCTAssertEqual(headers->used, 1);
     XCTAssertEqual(strcmp(headers->contents[0].name, "header the third"), 0);
 
-    bsg_remove_mach_binary_image("header the third");
+    bsg_remove_mach_binary_image(34567);
     XCTAssertEqual(headers->size, 4);
     XCTAssertEqual(headers->used, 0);
 
-    bsg_remove_mach_binary_image("header the third");
+    bsg_remove_mach_binary_image(34567);
     XCTAssertEqual(headers->size, 4);
     XCTAssertEqual(headers->used, 0);
 
@@ -97,7 +97,7 @@ - (void)testRemoveLast1 {
     XCTAssertEqual(headers->size, 2);
     XCTAssertEqual(headers->used, 1);
 
-    bsg_remove_mach_binary_image("header the first");
+    bsg_remove_mach_binary_image(12345);
     XCTAssertEqual(headers->size, 2);
     XCTAssertEqual(headers->used, 0);
 }
@@ -110,11 +110,11 @@ - (void)testRemoveLast2 {
     XCTAssertEqual(headers->size, 2);
     XCTAssertEqual(headers->used, 2);
 
-    bsg_remove_mach_binary_image("header the second");
+    bsg_remove_mach_binary_image(23456);
     XCTAssertEqual(headers->size, 2);
     XCTAssertEqual(headers->used, 1);
 
-    bsg_remove_mach_binary_image("header the first");
+    bsg_remove_mach_binary_image(12345);
     XCTAssertEqual(headers->size, 2);
     XCTAssertEqual(headers->used, 0);
 }
@@ -130,19 +130,19 @@ - (void)testRemoveLast3 {
     XCTAssertEqual(headers->size, 4);
     XCTAssertEqual(headers->used, 4);
 
-    bsg_remove_mach_binary_image("header the fourth");
+    bsg_remove_mach_binary_image(45678);
     XCTAssertEqual(headers->size, 4);
     XCTAssertEqual(headers->used, 3);
 
-    bsg_remove_mach_binary_image("header the first");
+    bsg_remove_mach_binary_image(12345);
     XCTAssertEqual(headers->size, 4);
     XCTAssertEqual(headers->used, 2);
 
-    bsg_remove_mach_binary_image("header the first");
+    bsg_remove_mach_binary_image(12345);
     XCTAssertEqual(headers->size, 4);
     XCTAssertEqual(headers->used, 2);
 
-    bsg_remove_mach_binary_image("header the third");
+    bsg_remove_mach_binary_image(34567);
     XCTAssertEqual(headers->size, 4);
     XCTAssertEqual(headers->used, 1);
 }

iOS/Bugsnag.xcodeproj/project.pbxproj

@@ -7,6 +7,7 @@
 	objects = {
 
 /* Begin PBXBuildFile section */
+		0003F3C52461B29A00AE0C93 /* BSG_KSMachHeaders.h in CopyFiles */ = {isa = PBXBuildFile; fileRef = 0071CB4D2460216200F562B1 /* BSG_KSMachHeaders.h */; };
 		0071CB4B2460213200F562B1 /* BSG_KSMachHeaders.m in Sources */ = {isa = PBXBuildFile; fileRef = 0071CB4A2460213200F562B1 /* BSG_KSMachHeaders.m */; };
 		0071CB4C2460213200F562B1 /* BSG_KSMachHeaders.m in Sources */ = {isa = PBXBuildFile; fileRef = 0071CB4A2460213200F562B1 /* BSG_KSMachHeaders.m */; };
 		0071CB4F246025AF00F562B1 /* KSMachHeader_Tests.m in Sources */ = {isa = PBXBuildFile; fileRef = 0071CB4E246025AF00F562B1 /* KSMachHeader_Tests.m */; };
@@ -327,6 +328,7 @@
 			dstPath = "include/${PRODUCT_NAME}";
 			dstSubfolderSpec = 16;
 			files = (
+				0003F3C52461B29A00AE0C93 /* BSG_KSMachHeaders.h in CopyFiles */,
 				8A3C590923965D9400B344AA /* BugsnagPlugin.h in CopyFiles */,
 				E79148251FD828E6003EFEBF /* BugsnagKeys.h in CopyFiles */,
 				E79148261FD828E6003EFEBF /* BugsnagSessionTracker.h in CopyFiles */,