From 5067239798960c6ee4146c09ff2675f769565c20 Mon Sep 17 00:00:00 2001 From: Nick Dowell Date: Mon, 11 Jul 2022 15:56:34 +0100 Subject: [PATCH] Fix a rare crash in `BugsnagBreadcrumbsWriteCrashReport()` (#1430) --- Bugsnag/Breadcrumbs/BugsnagBreadcrumbs.m | 29 +++++++++++++-------- CHANGELOG.md | 3 +++ Tests/BugsnagTests/BugsnagBreadcrumbsTest.m | 15 +---------- 3 files changed, 22 insertions(+), 25 deletions(-) diff --git a/Bugsnag/Breadcrumbs/BugsnagBreadcrumbs.m b/Bugsnag/Breadcrumbs/BugsnagBreadcrumbs.m index ee93979b3..f3889402d 100644 --- a/Bugsnag/Breadcrumbs/BugsnagBreadcrumbs.m +++ b/Bugsnag/Breadcrumbs/BugsnagBreadcrumbs.m @@ -18,6 +18,7 @@ #import "BugsnagConfiguration+Private.h" #import "BugsnagLogger.h" +#import // // 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 *)breadcrumbs { NSMutableArray *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 index 4e4a66ebb..469822ce6 100644 --- 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 index 8873e2a45..db5c73da4 100644 --- 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;