Fix a rare crash in BugsnagBreadcrumbsWriteCrashReport() (#1430)

bugsnag · Jul 11, 2022 · 5067239 · 5067239
1 parent fa7abec
commit 5067239
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 25 deletions.
diff --git a/Bugsnag/Breadcrumbs/BugsnagBreadcrumbs.m b/Bugsnag/Breadcrumbs/BugsnagBreadcrumbs.m
@@ -18,6 +18,7 @@
 #import "BugsnagConfiguration+Private.h"
 #import "BugsnagLogger.h"
 
+#import <stdatomic.h>
 
 //
 // Breadcrumbs are stored as a linked list of JSON encoded C strings
@@ -29,7 +30,8 @@
     char jsonData[]; // MUST be null terminated
 };
 
-static struct bsg_breadcrumb_list_item *g_breadcrumbs_head;
+static _Atomic(struct bsg_breadcrumb_list_item *) g_breadcrumbs_head;
+static atomic_bool g_writing_crash_report;
 
 #pragma mark -
 
@@ -64,7 +66,7 @@ - (instancetype)initWithConfiguration:(BugsnagConfiguration *)config {
 - (NSArray<BugsnagBreadcrumb *> *)breadcrumbs {
     NSMutableArray<BugsnagBreadcrumb *> *breadcrumbs = [NSMutableArray array];
     @synchronized (self) {
-        for (struct bsg_breadcrumb_list_item *item = g_breadcrumbs_head; item != NULL; item = item->next) {
+        for (struct bsg_breadcrumb_list_item *item = atomic_load(&g_breadcrumbs_head); item != NULL; item = item->next) {
             NSError *error = nil;
             NSData *data = [NSData dataWithBytesNoCopy:item->jsonData length:strlen(item->jsonData) freeWhenDone:NO];
             NSDictionary *JSONObject = BSGJSONDictionaryFromData(data, 0, &error);
@@ -128,20 +130,20 @@ - (void)addBreadcrumbWithData:(NSData *)data writeToDisk:(BOOL)writeToDisk {
         const BOOL deleteOld = fileNumber >= self.maxBreadcrumbs;
         self.nextFileNumber = fileNumber + 1;
 
-        if (g_breadcrumbs_head) {
-            struct bsg_breadcrumb_list_item *tail = g_breadcrumbs_head;
+        struct bsg_breadcrumb_list_item *head = atomic_load(&g_breadcrumbs_head);
+        if (head) {
+            struct bsg_breadcrumb_list_item *tail = head;
             while (tail->next) {
                 tail = tail->next;
             }
             tail->next = newItem;
             if (deleteOld) {
-                struct bsg_breadcrumb_list_item *head = g_breadcrumbs_head;
-                g_breadcrumbs_head = head->next;
-                head->next = NULL;
+                atomic_store(&g_breadcrumbs_head, head->next);
+                while (atomic_load(&g_writing_crash_report)) { continue; }
                 free(head);
             }
         } else {
-            g_breadcrumbs_head = newItem;
+            atomic_store(&g_breadcrumbs_head, newItem);
         }
 
         if (!writeToDisk) {
@@ -196,8 +198,7 @@ - (BOOL)shouldSendBreadcrumb:(BugsnagBreadcrumb *)crumb {
 
 - (void)removeAllBreadcrumbs {
     @synchronized (self) {
-        struct bsg_breadcrumb_list_item *item = g_breadcrumbs_head;
-        g_breadcrumbs_head = NULL;
+        struct bsg_breadcrumb_list_item *item = atomic_exchange(&g_breadcrumbs_head, NULL);
         while (item) {
             struct bsg_breadcrumb_list_item *next = item->next;
             free(item);
@@ -288,11 +289,17 @@ - (void)deleteBreadcrumbFiles {
 #pragma mark -
 
 void BugsnagBreadcrumbsWriteCrashReport(const BSG_KSCrashReportWriter *writer) {
+    atomic_store(&g_writing_crash_report, true);
+
     writer->beginArray(writer, "breadcrumbs");
 
-    for (struct bsg_breadcrumb_list_item *item = g_breadcrumbs_head; item != NULL; item = item->next) {
+    struct bsg_breadcrumb_list_item *item = atomic_load(&g_breadcrumbs_head);
+    while (item) {
         writer->addJSONElement(writer, NULL, item->jsonData);
+        item = item->next;
     }
 
     writer->endContainer(writer);
+
+    atomic_store(&g_writing_crash_report, false);
 }
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,9 @@ Changelog
 * Prevent reporting of OOMs on simulators. 
   [#1421](https://github.com/bugsnag/bugsnag-cocoa/pull/1421)
 
+* Fix a rare crash in `BugsnagBreadcrumbsWriteCrashReport()`.
+  [#1430](https://github.com/bugsnag/bugsnag-cocoa/pull/1430)
+
 ## 6.19.0 (2022-06-29)
 
 ### Enhancements

diff --git a/Tests/BugsnagTests/BugsnagBreadcrumbsTest.m b/Tests/BugsnagTests/BugsnagBreadcrumbsTest.m
@@ -28,13 +28,11 @@
     char *buffer;
 };
 
-#if BSG_HAVE_MACH_THREADS
 static int json_buffer_append(const char *data, size_t length, struct json_buffer *buffer) {
     memcpy(buffer->buffer + buffer->length, data, length);
     buffer->length += length;
     return BSG_KSJSON_OK;
 }
-#endif
 
 static int addJSONData(const char *data, size_t length, NSMutableData *userData) {
     [userData appendBytes:data length:length];
@@ -460,7 +458,6 @@ - (void)testCrashReportWriter {
     XCTAssertEqualObjects(breadcrumbs[2][@"metaData"], @{});
 }
 
-#if BSG_HAVE_MACH_THREADS
 static void * executeBlock(void *ptr) {
     ((__bridge_transfer dispatch_block_t)ptr)();
     return NULL;
@@ -473,7 +470,7 @@ - (void)testCrashReportWriterConcurrency {
 #endif
     //
     // The aim of this test is to ensure that BugsnagBreadcrumbsWriteCrashReport will insert only valid JSON
-    // into a crash report when other threads are (paused while) updating the breadcrumbs linked list.
+    // into a crash report when other threads are updating the breadcrumbs linked list.
     //
     // So that the test spends less time serialising breadcrumbs and more time updating the linked list, the
     // breadcrumb data is precomputed and not written to disk.
@@ -510,11 +507,6 @@ - (void)testCrashReportWriterConcurrency {
     for (int i = 0; i < 5000; i++) {
         buffer.length = 0;
 
-        // BugsnagBreadcrumbsWriteCrashReport() requires other threads to be suspended.
-        for (int i = 0; i < threadCount; i++) {
-            thread_suspend(machThreads[i]);
-        }
-
         BSG_KSJSONEncodeContext context;
         BSG_KSCrashReportWriter writer;
         bsg_kscrw_i_prepareReportWriter(&writer, &context);
@@ -523,10 +515,6 @@ - (void)testCrashReportWriterConcurrency {
         BugsnagBreadcrumbsWriteCrashReport(&writer);
         writer.endContainer(&writer);
 
-        for (int i = 0; i < threadCount; i++) {
-            thread_resume(machThreads[i]);
-        }
-
         NSError *error = nil;
         NSData *data = [NSData dataWithBytesNoCopy:buffer.buffer length:buffer.length freeWhenDone:NO];
         if (![NSJSONSerialization JSONObjectWithData:data options:0 error:&error]) {
@@ -545,7 +533,6 @@ - (void)testCrashReportWriterConcurrency {
         pthread_join(threads[i], NULL);
     }
 }
-#endif
 
 - (void)testPerformance {
     NSInteger maxBreadcrumbs = 100;