fix: Avoid crash/deadlock when calling notify() concurrently (#401)

CHANGELOG.md

@@ -1,6 +1,14 @@
 Changelog
 =========
 
+## TBD
+
+### Bug fixes
+
+* Fix possible crash or deadlock arising from calling Bugsnag.notify() from
+  multiple queues concurrently.
+  [#401](https://github.com/bugsnag/bugsnag-cocoa/pull/401)
+
 ## 5.22.4 (2019-07-30)
 
 ### Bug fixes

OSX/Bugsnag.xcodeproj/project.pbxproj

@@ -34,6 +34,9 @@
 		8A2C8FF01C6BC3A200846019 /* SystemConfiguration.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 8A2C8FEF1C6BC3A200846019 /* SystemConfiguration.framework */; };
 		8A2C90441C6C040700846019 /* Cocoa.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 8A2C90401C6C03F000846019 /* Cocoa.framework */; };
 		8A48EF271EAA805D00B70024 /* BugsnagLogger.h in Headers */ = {isa = PBXBuildFile; fileRef = 8A48EF261EAA805D00B70024 /* BugsnagLogger.h */; };
+		8A530CB822FDC38300F0C108 /* BSG_KSCrashIdentifier.h in Headers */ = {isa = PBXBuildFile; fileRef = 8A530CB622FDC38300F0C108 /* BSG_KSCrashIdentifier.h */; };
+		8A530CB922FDC38300F0C108 /* BSG_KSCrashIdentifier.m in Sources */ = {isa = PBXBuildFile; fileRef = 8A530CB722FDC38300F0C108 /* BSG_KSCrashIdentifier.m */; };
+		8A530CC922FDD74300F0C108 /* KSCrashIdentifierTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 8A530CC722FDD73800F0C108 /* KSCrashIdentifierTests.m */; };
 		8A627CD91EC3B75200F7C04E /* BSGSerialization.h in Headers */ = {isa = PBXBuildFile; fileRef = 8A627CD71EC3B75200F7C04E /* BSGSerialization.h */; };
 		8A627CDA1EC3B75200F7C04E /* BSGSerialization.m in Sources */ = {isa = PBXBuildFile; fileRef = 8A627CD81EC3B75200F7C04E /* BSGSerialization.m */; };
 		8A6C6FB12257884C00E8EF24 /* BSGOutOfMemoryWatchdog.m in Sources */ = {isa = PBXBuildFile; fileRef = 8A6C6FAF2257884C00E8EF24 /* BSGOutOfMemoryWatchdog.m */; };
@@ -216,6 +219,9 @@
 		8A2C90401C6C03F000846019 /* Cocoa.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Cocoa.framework; path = System/Library/Frameworks/Cocoa.framework; sourceTree = SDKROOT; };
 		8A2C90421C6C03FF00846019 /* Foundation.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Foundation.framework; path = System/Library/Frameworks/Foundation.framework; sourceTree = SDKROOT; };
 		8A48EF261EAA805D00B70024 /* BugsnagLogger.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = BugsnagLogger.h; path = ../Source/BugsnagLogger.h; sourceTree = SOURCE_ROOT; };
+		8A530CB622FDC38300F0C108 /* BSG_KSCrashIdentifier.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = BSG_KSCrashIdentifier.h; sourceTree = "<group>"; };
+		8A530CB722FDC38300F0C108 /* BSG_KSCrashIdentifier.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = BSG_KSCrashIdentifier.m; sourceTree = "<group>"; };
+		8A530CC722FDD73800F0C108 /* KSCrashIdentifierTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = KSCrashIdentifierTests.m; sourceTree = "<group>"; };
 		8A627CD71EC3B75200F7C04E /* BSGSerialization.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = BSGSerialization.h; path = ../Source/BSGSerialization.h; sourceTree = SOURCE_ROOT; };
 		8A627CD81EC3B75200F7C04E /* BSGSerialization.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = BSGSerialization.m; path = ../Source/BSGSerialization.m; sourceTree = SOURCE_ROOT; };
 		8A6C6FAF2257884C00E8EF24 /* BSGOutOfMemoryWatchdog.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = BSGOutOfMemoryWatchdog.m; path = ../Source/BSGOutOfMemoryWatchdog.m; sourceTree = "<group>"; };
@@ -541,6 +547,8 @@
 				E79E6B2A1F4E3850002B35F9 /* BSG_KSCrashState.h */,
 				E79E6B2B1F4E3850002B35F9 /* BSG_KSCrashType.c */,
 				E79E6B2C1F4E3850002B35F9 /* BSG_KSCrashType.h */,
+				8A530CB622FDC38300F0C108 /* BSG_KSCrashIdentifier.h */,
+				8A530CB722FDC38300F0C108 /* BSG_KSCrashIdentifier.m */,
 				E79E6B2D1F4E3850002B35F9 /* BSG_KSSystemCapabilities.h */,
 				E79E6B2E1F4E3850002B35F9 /* BSG_KSSystemInfo.h */,
 				E79E6B2F1F4E3850002B35F9 /* BSG_KSSystemInfo.m */,
@@ -650,6 +658,7 @@
 				E7CE78A01FD94E60001D07E0 /* KSCrashState_Tests.m */,
 				E7CE789B1FD94E60001D07E0 /* KSDynamicLinker_Tests.m */,
 				E7CE78891FD94E5F001D07E0 /* KSFileUtils_Tests.m */,
+				8A530CC722FDD73800F0C108 /* KSCrashIdentifierTests.m */,
 				E7CE78961FD94E5F001D07E0 /* KSJSONCodec_Tests.m */,
 				E7CE789E1FD94E60001D07E0 /* KSLogger_Tests.m */,
 				E7CE78871FD94E5F001D07E0 /* KSMach_Tests.m */,
@@ -722,6 +731,7 @@
 				E79E6BA11F4E3850002B35F9 /* BSG_KSCrashSentry_CPPException.h in Headers */,
 				8A2C8FD51C6BC2C800846019 /* BugsnagCrashReport.h in Headers */,
 				E79E6B041F4E3847002B35F9 /* BugsnagErrorReportApiClient.h in Headers */,
+				8A530CB822FDC38300F0C108 /* BSG_KSCrashIdentifier.h in Headers */,
 				E79E6BB11F4E3850002B35F9 /* BSG_KSBacktrace_Private.h in Headers */,
 				E79E6B8C1F4E3850002B35F9 /* BSG_KSCrashC.h in Headers */,
 				E79E6BDC1F4E3850002B35F9 /* BSG_KSCrashReportFilterCompletion.h in Headers */,
@@ -897,6 +907,7 @@
 				E79148581FD82B36003EFEBF /* BugsnagFileStore.m in Sources */,
 				E79E6BDA1F4E3850002B35F9 /* BSG_RFC3339DateTool.m in Sources */,
 				E72352C21F55924A00436528 /* BSGConnectivity.m in Sources */,
+				8A530CB922FDC38300F0C108 /* BSG_KSCrashIdentifier.m in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};
@@ -914,6 +925,7 @@
 				E7CE78D51FD94E93001D07E0 /* XCTestCase+KSCrash.m in Sources */,
 				E7CE78CF1FD94E77001D07E0 /* NSError+SimpleConstructor_Tests.m in Sources */,
 				E7CE78BF1FD94E77001D07E0 /* KSCrashSentry_Signal_Tests.m in Sources */,
+				8A530CC922FDD74300F0C108 /* KSCrashIdentifierTests.m in Sources */,
 				4B406C1822CAD96400464D1D /* BugsnagCollectionsBSGDictMergeTest.m in Sources */,
 				E7CE78CB1FD94E77001D07E0 /* KSSysCtl_Tests.m in Sources */,
 				E7CE78D01FD94E77001D07E0 /* RFC3339DateTool_Tests.m in Sources */,

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

@@ -315,15 +315,6 @@ - (void)reportUserException:(NSString *)name
                                     terminateProgram);
 
     free(callstack);
-
-    // If bsg_kscrash_reportUserException() returns, we did not terminate.
-    // Set up IDs and paths for the next crash.
-
-    self.nextCrashID = [NSUUID UUID].UUIDString;
-
-    bsg_kscrash_reinstall(
-        [self.crashReportPath UTF8String], [self.recrashReportPath UTF8String],
-        [self.stateFilePath UTF8String], [self.nextCrashID UTF8String]);
 }
 
 // ============================================================================

Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c

@@ -49,12 +49,6 @@ static volatile sig_atomic_t bsg_g_installed = 0;
 static BSG_KSCrash_Context bsg_g_crashReportContext = {
     .config = {.handlingCrashTypes = BSG_KSCrashTypeProductionSafe}};
 
-/** Path to store the next crash report. */
-static char *bsg_g_crashReportFilePath;
-
-/** Path to store the next crash report (only if the crash manager crashes). */
-static char *bsg_g_recrashReportFilePath;
-
 /** Path to store the state file. */
 static char *bsg_g_stateFilePath;
 
@@ -65,7 +59,7 @@ static const int bsg_filepath_len = 512;
 static const int bsg_error_class_filepath_len = 21;
 static const char bsg_filepath_context_sep = '-';
 
-static inline BSG_KSCrash_Context *crashContext(void) {
+BSG_KSCrash_Context *crashContext(void) {
     return &bsg_g_crashReportContext;
 }
 
@@ -118,11 +112,9 @@ int bsg_create_filepath(char *base, char filepath[bsg_filepath_len], char severi
  *
  * This function gets passed as a callback to a crash handler.
  */
-void bsg_kscrash_i_onCrash(char severity, char *errorClass) {
+void bsg_kscrash_i_onCrash(char severity, char *errorClass, BSG_KSCrash_Context *context) {
     BSG_KSLOG_DEBUG("Updating application state to note crash.");
 
-    BSG_KSCrash_Context *context = crashContext();
-
     bsg_kscrashstate_notifyAppCrash(context->crash.crashType);
 
     if (context->config.printTraceToStdout) {
@@ -131,10 +123,10 @@ void bsg_kscrash_i_onCrash(char severity, char *errorClass) {
 
     if (context->crash.crashedDuringCrashHandling) {
         bsg_kscrashreport_writeMinimalReport(context,
-                                             bsg_g_recrashReportFilePath);
+                                             context->config.recrashReportFilePath);
     } else {
         char filepath[bsg_filepath_len];
-        bsg_create_filepath(bsg_g_crashReportFilePath, filepath, severity, errorClass);
+        bsg_create_filepath((char *)context->config.crashReportFilePath, filepath, severity, errorClass);
         bsg_kscrashreport_writeStandardReport(context, filepath);
     }
 }
@@ -186,11 +178,12 @@ void bsg_kscrash_reinstall(const char *const crashReportFilePath,
     BSG_KSLOG_TRACE("crashID = %s", crashID);
 
     bsg_ksstring_replace((const char **)&bsg_g_stateFilePath, stateFilePath);
-    bsg_ksstring_replace((const char **)&bsg_g_crashReportFilePath,
+
+    BSG_KSCrash_Context *context = crashContext();
+    bsg_ksstring_replace((const char **)&context->config.crashReportFilePath,
                          crashReportFilePath);
-    bsg_ksstring_replace((const char **)&bsg_g_recrashReportFilePath,
+    bsg_ksstring_replace((const char **)&context->config.recrashReportFilePath,
                          recrashReportFilePath);
-    BSG_KSCrash_Context *context = crashContext();
     bsg_ksstring_replace(&context->config.crashID, crashID);
 
     if (!bsg_kscrashstate_init(bsg_g_stateFilePath, &context->state)) {
@@ -206,7 +199,7 @@ BSG_KSCrashType bsg_kscrash_setHandlingCrashTypes(BSG_KSCrashType crashTypes) {
     if (bsg_g_installed) {
         bsg_kscrashsentry_uninstall(~crashTypes);
         crashTypes = bsg_kscrashsentry_installWithContext(
-            &context->crash, crashTypes, bsg_kscrash_i_onCrash);
+            &context->crash, crashTypes, (void(*)(char, char *, void *))bsg_kscrash_i_onCrash);
     }
 
     return crashTypes;

Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.h

@@ -176,6 +176,11 @@ void bsg_kscrash_setThreadTracingEnabled(bool threadTracingEnabled);
 void bsg_kscrash_setWriteBinaryImagesForUserReported(
     bool writeBinaryImagesForUserReported);
 
+/**
+ * The current crash context
+ */
+BSG_KSCrash_Context *crashContext(void);
+
 #ifdef __cplusplus
 }
 #endif

Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashContext.h

@@ -76,6 +76,16 @@ typedef struct {
      * the report file. Application MUST NOT call async-unsafe methods!
      */
     BSGReportCallback onCrashNotify;
+
+    /**
+     * File path to write the crash report
+     */
+    const char *crashReportFilePath;
+
+    /**
+     * File path to write the recrash report, if the crash reporter crashes
+     */
+    const char *recrashReportFilePath;
 } BSG_KSCrash_Configuration;
 
 /** Contextual data used by the crash report writer.

Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashIdentifier.h

@@ -0,0 +1,25 @@
+#ifndef HDR_BSG_KSCrashIdentifier_h
+#define HDR_BSG_KSCrashIdentifier_h
+#include <stdbool.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * Generates a new UUID. Not async signal safe. Caller responsible for
+ * freeing allocated string.
+ */
+const char *bsg_kscrash_generate_report_identifier(void);
+/**
+ * Generates a new path string. Not async signal safe. Caller responsible
+ * for freeing allocated string.
+ */
+const char *bsg_kscrash_generate_report_path(const char *identifier,
+                                             bool is_recrash_report);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif // HDR_BSG_KSCrashIdentifier_h

Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashIdentifier.m

@@ -0,0 +1,23 @@
+#import "BSG_KSCrashIdentifier.h"
+#import "BSG_KSCrashAdvanced.h"
+#import <Foundation/Foundation.h>
+#import <string.h>
+
+const char *bsg_kscrash_generate_report_identifier(void) {
+    return strdup([[[NSUUID UUID] UUIDString] UTF8String]);
+}
+
+const char *bsg_kscrash_generate_report_path(const char *identifier,
+                                             bool is_recrash_report) {
+    if (identifier == NULL) {
+        return NULL;
+    }
+    BSG_KSCrashReportStore *store = [[BSG_KSCrash sharedInstance] crashReportStore];
+    NSString *reportID = [NSString stringWithUTF8String:identifier];
+
+    if (is_recrash_report) {
+        return strdup([[store pathToRecrashReportWithID:reportID] UTF8String]);
+    } else {
+        return strdup([[store pathToFileWithId:reportID] UTF8String]);
+    }
+}

Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashState.m

@@ -358,8 +358,11 @@ void bsg_kscrashstate_notifyAppCrash(BSG_KSCrashType type) {
         state->backgroundDurationSinceLaunch += duration;
         state->backgroundDurationSinceLastCrash += duration;
     }
-    state->crashedThisLaunch |= type != BSG_KSCrashTypeUserReported;
-    bsg_kscrashstate_i_saveState(state, stateFilePath);
+    BOOL didCrash = type != BSG_KSCrashTypeUserReported;
+    state->crashedThisLaunch |= didCrash;
+    if (didCrash) {
+        bsg_kscrashstate_i_saveState(state, stateFilePath);
+    }
 }
 
 const BSG_KSCrash_State *const bsg_kscrashstate_currentState(void) {

Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry.c

@@ -90,7 +90,7 @@ static bool bsg_g_threads_are_running = true;
 BSG_KSCrashType
 bsg_kscrashsentry_installWithContext(BSG_KSCrash_SentryContext *context,
                                      BSG_KSCrashType crashTypes,
-                                     void (*onCrash)(char, char *)) {
+                                     void (*onCrash)(char, char *, void *)) {
     if (bsg_ksmachisBeingTraced()) {
         if (context->reportWhenDebuggerIsAttached) {
             BSG_KSLOG_WARN("KSCrash: App is running in a debugger. Crash "
@@ -200,7 +200,7 @@ void bsg_kscrashsentry_resumeThreads(void) {
 }
 
 void bsg_kscrashsentry_clearContext(BSG_KSCrash_SentryContext *context) {
-    void (*onCrash)(char, char *) = context->onCrash;
+    void (*onCrash)(char, char *, void *) = context->onCrash;
     bool threadTracingEnabled = context->threadTracingEnabled;
     bool reportWhenDebuggerIsAttached = context->reportWhenDebuggerIsAttached;
     bool suspendThreadsForUserReported = context->suspendThreadsForUserReported;

Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry.h

@@ -51,7 +51,7 @@ typedef struct BSG_KSCrash_SentryContext {
     // Caller defined values. Caller must fill these out prior to installation.
 
     /** Called by the crash handler when a crash is detected. */
-    void (*onCrash)(char, char[21]);
+    void (*onCrash)(char, char[21], void *);
 
     /** If true, will suspend threads for user reported exceptions. */
     bool suspendThreadsForUserReported;
@@ -162,7 +162,7 @@ typedef struct BSG_KSCrash_SentryContext {
 BSG_KSCrashType
 bsg_kscrashsentry_installWithContext(BSG_KSCrash_SentryContext *context,
                                      BSG_KSCrashType crashTypes,
-                                     void (*onCrash)(char, char *));
+                                     void (*onCrash)(char, char *, void *));
 
 /** Uninstall crash sentry.
  *

Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_CPPException.mm

@@ -27,6 +27,7 @@
 #include "BSG_KSCrashSentry_CPPException.h"
 #include "BSG_KSCrashSentry_Private.h"
 #include "BSG_KSMach.h"
+#include "BSG_KSCrashC.h"
 
 //#define BSG_KSLogger_LocalLevel TRACE
 #include "BSG_KSLogger.h"
@@ -181,7 +182,7 @@ static void CPPExceptionTerminate(void) {
         BSG_KSLOG_DEBUG(@"Calling main crash handler.");
         char errorClass[21];
         strncpy(errorClass, bsg_g_context->CPPException.name, sizeof(errorClass));
-        bsg_g_context->onCrash('e', errorClass);
+        bsg_g_context->onCrash('e', errorClass, crashContext());
 
         BSG_KSLOG_DEBUG(
             @"Crash handling complete. Restoring original handlers.");

Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_MachException.c

@@ -29,6 +29,7 @@
 
 //#define BSG_KSLogger_LocalLevel TRACE
 #include "BSG_KSLogger.h"
+#include "BSG_KSCrashC.h"
 
 #if BSG_KSCRASH_HAS_MACH
 
@@ -287,7 +288,7 @@ void *ksmachexc_i_handleExceptions(void *const userData) {
         BSG_KSLOG_DEBUG("Calling main crash handler.");
         char errorClass[21];
         strncpy(errorClass, bsg_ksmachexceptionName(bsg_g_context->mach.type), sizeof(errorClass));
-        bsg_g_context->onCrash('e', errorClass);
+        bsg_g_context->onCrash('e', errorClass, crashContext());
 
         BSG_KSLOG_DEBUG(
             "Crash handling complete. Restoring original handlers.");

Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_NSException.m

@@ -27,6 +27,7 @@
 #import "BSG_KSCrashSentry_NSException.h"
 #import "BSG_KSCrashSentry_Private.h"
 #include "BSG_KSMach.h"
+#include "BSG_KSCrashC.h"
 
 //#define BSG_KSLogger_LocalLevel TRACE
 #import "BSG_KSLogger.h"
@@ -130,7 +131,7 @@ void bsg_recordException(NSException *exception) {
         BSG_KSLOG_DEBUG(@"Calling main crash handler.");
         char errorClass[21];
         strncpy(errorClass, bsg_g_context->NSException.name, sizeof(errorClass));
-        bsg_g_context->onCrash('e', errorClass);
+        bsg_g_context->onCrash('e', errorClass, crashContext());
     }
 }
 

Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_Signal.c

@@ -31,6 +31,7 @@
 
 #include "BSG_KSMach.h"
 #include "BSG_KSSignalInfo.h"
+#include "BSG_KSCrashC.h"
 
 //#define BSG_KSLogger_LocalLevel TRACE
 #include "BSG_KSLogger.h"
@@ -118,7 +119,8 @@ void bsg_kssighndl_i_handleSignal(int sigNum, siginfo_t *signalInfo,
                 errorClass[i] = c;
             }
         }
-        bsg_g_context->onCrash('e', errorClass);
+
+        bsg_g_context->onCrash('e', errorClass, crashContext());
 
         BSG_KSLOG_DEBUG(
             "Crash handling complete. Restoring original handlers.");