Updated to use dynamic array, added test

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

@@ -221,15 +221,15 @@ - (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;
     }
-
-    // Maintain a cache of info about dynamically loaded binary images
-    [self listenForLoadedBinaries];
 
 #if BSG_KSCRASH_HAS_UIKIT
     NSNotificationCenter *nCenter = [NSNotificationCenter defaultCenter];
@@ -266,6 +266,9 @@ - (BOOL)install {
  */
 - (void)listenForLoadedBinaries {
     bsg_initialise_mach_binary_headers();
+
+    // 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);
 }

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

@@ -1113,14 +1113,12 @@ void bsg_kscrw_i_writeBinaryImage(const BSG_KSCrashReportWriter *const writer,
 void bsg_kscrw_i_writeBinaryImages(const BSG_KSCrashReportWriter *const writer,
                                    const char *const key)
 {
-    // Get the array of loaded binary images and a count
-    size_t imageCount;
-    BSG_Mach_Binary_Image_Info *info = bsg_mach_header_array(&imageCount);
-
     writer->beginArray(writer, key);
     {
-        for (uint32_t iImg = 0; iImg < imageCount; iImg++) {
-            bsg_kscrw_i_writeBinaryImage(writer, NULL, &info[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

@@ -9,13 +9,14 @@
 #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 {
-    // Removal of loaded binary images is a relative rarity.  It's simpler to mark an entry than reallocate the array
-    bool deleted;
     const struct mach_header *mh;      /* The mach_header - 32 or 64 bit */
     uint64_t imageVmAddr;
     uint64_t imageSize;
@@ -30,25 +31,20 @@ typedef struct {
  * See: https://stackoverflow.com/a/3536261/2431627
  */
 typedef struct {
-  BSG_Mach_Binary_Image_Info *contents;
-  size_t used;
-  size_t size;
+    size_t used;
+    size_t size;
+    BSG_Mach_Binary_Image_Info *contents;
 } BSG_Mach_Binary_Images;
 
-void initializeBinaryImages(BSG_Mach_Binary_Images *array, size_t initialSize);
-void addBinaryImage(BSG_Mach_Binary_Images *array, BSG_Mach_Binary_Image_Info element);
-bool deleteBinaryImage(BSG_Mach_Binary_Images *array, const char *element_name);
-void freeBinaryImages(BSG_Mach_Binary_Images *array);
+static BSG_Mach_Binary_Images bsg_mach_binary_images;
 
-/**
- * Returns a C array of structs describing the loaded Mach binaries
- *
- * @param count A reference to the length of the array
- *
- * @returns A reference to an array of BSG_Mach_Binary_Image_Info structs containing info
- *          about a loaded Mach binary.
- */
-BSG_Mach_Binary_Image_Info* bsg_mach_header_array(size_t *count);
+static os_unfair_lock bsg_mach_binary_images_access_lock = OS_UNFAIR_LOCK_INIT;
+
+void bsg_initialize_binary_images_array(BSG_Mach_Binary_Images *array, size_t initialSize);
+void bsg_add_mach_binary_image(BSG_Mach_Binary_Images *array, BSG_Mach_Binary_Image_Info element);
+void bsg_remove_mach_binary_image(BSG_Mach_Binary_Images *array, const char *element_name);
+void bsg_free_binary_images_array(BSG_Mach_Binary_Images *array);
+BSG_Mach_Binary_Images *bsg_get_mach_binary_images(void);
 
 /**
  * Called when a binary image is loaded.

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

@@ -12,57 +12,73 @@
 #import "BSG_KSCrash.h"
 #import "BSG_KSMachHeaders.h"
 
-static NSMutableDictionary<NSString *, NSValue*> *bsg_mach_binary_images_info;
-static BSG_Mach_Binary_Images bsg_binary_images;
-
 @interface BSG_KSCrash ()
 @end
 
-void initializeBinaryImages(BSG_Mach_Binary_Images *array, size_t initialSize) {
-  array->contents = (BSG_Mach_Binary_Image_Info *)malloc(initialSize * sizeof(BSG_Mach_Binary_Image_Info));
-  array->used = 0;
-  array->size = initialSize;
+void bsg_initialize_binary_images_array(BSG_Mach_Binary_Images *array, size_t initialSize) {
+    array->contents = (BSG_Mach_Binary_Image_Info *)malloc(initialSize * sizeof(BSG_Mach_Binary_Image_Info));
+    array->used = 0;
+    array->size = initialSize;
+}
+
+BSG_Mach_Binary_Images *bsg_get_mach_binary_images() {
+    return &bsg_mach_binary_images;
 }
 
-void addBinaryImage(BSG_Mach_Binary_Images *array, BSG_Mach_Binary_Image_Info element) {
-  // a->used is the number of used entries, because a->array[a->used++] updates a->used only *after* the array has been accessed.
-  // Therefore a->used can go up to a->size
-  if (array->used == array->size) {
-    array->size *= 2;
-    array->contents = (BSG_Mach_Binary_Image_Info *)realloc(array->contents, array->size * sizeof(BSG_Mach_Binary_Image_Info));
-  }
-  array->contents[array->used++] = element;
+/**
+ * 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_Images *array, BSG_Mach_Binary_Image_Info element) {
+
+    os_unfair_lock_lock(&bsg_mach_binary_images_access_lock);
+
+    // Expand array if necessary
+    if (array->used == array->size) {
+        array->size *= 2;
+        array->contents = (BSG_Mach_Binary_Image_Info *)realloc(array->contents, array->size * sizeof(BSG_Mach_Binary_Image_Info));
+    }
+
+    // Store the value, increment the number of used elements
+    array->contents[array->used++] = element;
+
+    os_unfair_lock_unlock(&bsg_mach_binary_images_access_lock);
 }
 
 /**
  * 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.
  */
-bool deleteBinaryImage(BSG_Mach_Binary_Images *array, const char *element_name) {
+void bsg_remove_mach_binary_image(BSG_Mach_Binary_Images *array, const char *element_name) {
+
+    os_unfair_lock_lock(&bsg_mach_binary_images_access_lock);
+
     for (size_t i=0; i<array->used; i++) {
         BSG_Mach_Binary_Image_Info item = array->contents[i];
         if (strcmp(element_name, item.name) == 0) {
-
-            // !!!!!
-            array->contents[i] = array->contents[array->used--];
-
-            break;
+            if (array->used >= 2) {
+                array->contents[i] = array->contents[--array->used];
+            }
+            else {
+                array->used = 0;
+            }
+            break; // an image can only be loaded singularly
         }
     }
 
-    return false;
+    os_unfair_lock_unlock(&bsg_mach_binary_images_access_lock);
 }
 
-void freeBinaryImages(BSG_Mach_Binary_Images *array) {
+void bsg_free_binary_images_array(BSG_Mach_Binary_Images *array) {
   free(array->contents);
   array->contents = NULL;
   array->used = array->size = 0;
 }
 
 void bsg_initialise_mach_binary_headers() {
-    bsg_mach_binary_images_info = [NSMutableDictionary<NSString *, NSValue*> new];
-    initializeBinaryImages(&bsg_binary_images, 100);
+    bsg_initialize_binary_images_array(&bsg_mach_binary_images, 100);
 }
 
 uintptr_t bsg_ksdlfirstCmdAfterHeader(const struct mach_header *const header);
@@ -146,17 +162,14 @@ bool populate_info(const struct mach_header *header, BSG_Mach_Binary_Image_Info
  *
  * @param header A mach_header structure
  *
- * @param slide The VM offset of the binary image
+ * @param slide A virtual memory slide amount. The virtual memory slide amount specifies the difference between the
+ *              address at which the image was linked and the address at which the image is loaded.  Unused.
  */
 void bsg_mach_binary_image_added(const struct mach_header *header, intptr_t slide)
 {
     BSG_Mach_Binary_Image_Info info = { 0 };
     if (populate_info(header, &info)) {
-        NSString *key = [NSString stringWithUTF8String:info.name];
-        NSValue *value = [NSValue valueWithBytes:&info
-                                        objCType:@encode(BSG_Mach_Binary_Image_Info)];
-
-        [bsg_mach_binary_images_info setValue:value forKey:key];
+        bsg_add_mach_binary_image(&bsg_mach_binary_images, info);
     }
 }
 
@@ -168,44 +181,6 @@ void bsg_mach_binary_image_removed(const struct mach_header *header, intptr_t sl
     // Convert header and slide into an info struct
     BSG_Mach_Binary_Image_Info info;
     if (populate_info(header, &info)) {
-        NSString *key = [NSString stringWithUTF8String:info.name];
-        [bsg_mach_binary_images_info removeObjectForKey:key];
-    }
-}
-
-/**
- * Returns a C array of structs describing the loaded Mach binaries
- */
-BSG_Mach_Binary_Image_Info* bsg_mach_header_array(size_t *count) {
-    @synchronized (@"bsg_mach_header_array") {
-
-        // Pass out the number of binary images
-        *count = [bsg_mach_binary_images_info count] ;
-
-        // How big is our struct?
-        const size_t mach_header_size = sizeof(BSG_Mach_Binary_Image_Info);
-
-        // Heap allocate the array of binary image info structs
-        BSG_Mach_Binary_Image_Info *headers = (BSG_Mach_Binary_Image_Info *)calloc(*count, mach_header_size);
-
-        // Copy the info into an array
-        for (size_t i=0; i<*count; ++i) {
-
-            // Reconstitute struct from NSValue
-            BSG_Mach_Binary_Image_Info info;
-
-            [[[bsg_mach_binary_images_info allValues] objectAtIndex:i] getValue:&info];
-
-            headers[i].cpusubtype = info.cpusubtype;
-            headers[i].cputype = info.cputype;
-            headers[i].imageSize = info.imageSize;
-            headers[i].imageVmAddr = info.imageVmAddr;
-            headers[i].uuid = info.uuid;
-            headers[i].mh = info.mh;
-            headers[i].name = info.name;
-        }
-
-        return headers;
+        bsg_remove_mach_binary_image(&bsg_mach_binary_images, info.name);
     }
 }
-

Tests/KSCrash/KSMachHeader_Tests.m

@@ -0,0 +1,111 @@
+//
+//  KSMachHeader_Tests.m
+//  Tests
+//
+//  Created by Robin Macharg on 04/05/2020.
+//  Copyright © 2020 Bugsnag. All rights reserved.
+//
+
+#import <XCTest/XCTest.h>
+#import "BSG_KSMachHeaders.h"
+#import <mach-o/dyld.h>
+
+const struct mach_header mh = {
+    .magic = 1,
+    .cputype = 1,
+    .cpusubtype = 1,
+    .filetype = 1,
+    .ncmds = 1,
+    .sizeofcmds = 1,
+    .flags = 1
+};
+
+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
+};
+
+@interface KSMachHeader_Tests : XCTestCase
+@end
+
+@implementation KSMachHeader_Tests
+
+- (void)testDynamicArray {
+    BSG_Mach_Binary_Images headers;
+    bsg_initialize_binary_images_array(&headers, 2);
+    XCTAssertEqual(headers.size, 2);
+    XCTAssertEqual(headers.used, 0);
+
+    // Add
+    bsg_add_mach_binary_image(&headers, info1);
+    XCTAssertEqual(headers.size, 2);
+    XCTAssertEqual(headers.used, 1);
+
+    bsg_add_mach_binary_image(&headers, info2);
+    XCTAssertEqual(headers.size, 2);
+    XCTAssertEqual(headers.used, 2);
+
+    // Expand - double size
+    bsg_add_mach_binary_image(&headers, info3);
+    XCTAssertEqual(headers.size, 4);
+    XCTAssertEqual(headers.used, 3);
+
+    // Delete - third will be copied
+    bsg_remove_mach_binary_image(&headers, "header the first");
+    XCTAssertEqual(headers.size, 4);
+    XCTAssertEqual(headers.used, 2);
+
+    XCTAssertEqual(strcmp(headers.contents[0].name, "header the third"), 0);
+    XCTAssertEqual(strcmp(headers.contents[1].name, "header the second"), 0);
+    XCTAssertEqual(headers.size, 4);
+
+    // Nothing happens
+    bsg_remove_mach_binary_image(&headers, "header the first");
+    XCTAssertEqual(headers.size, 4);
+    XCTAssertEqual(headers.used, 2);
+
+    bsg_remove_mach_binary_image(&headers, "header the second");
+    XCTAssertEqual(headers.size, 4);
+    XCTAssertEqual(headers.used, 1);
+    XCTAssertEqual(strcmp(headers.contents[0].name, "header the third"), 0);
+
+    bsg_remove_mach_binary_image(&headers, "header the third");
+    XCTAssertEqual(headers.size, 4);
+    XCTAssertEqual(headers.used, 0);
+
+    bsg_remove_mach_binary_image(&headers, "header the third");
+    XCTAssertEqual(headers.size, 4);
+    XCTAssertEqual(headers.used, 0);
+
+    // Readd
+    bsg_add_mach_binary_image(&headers, info1);
+    XCTAssertEqual(headers.size, 4);
+    XCTAssertEqual(headers.used, 1);
+}
+
+@end