fix(utils): Remove global buffer used when writing files to disk (#442)

write(2) already intelligently handles buffering/writing, so this buffer
is unneeded in addition to being unsafe for the user-initiated reporting
case.

The buffer was added in kstenerud/KSCrash@be9bedb
and thus the original intent is lost to the mists.

CHANGELOG.md

@@ -1,6 +1,14 @@
 Changelog
 =========
 
+## TBD
+
+## Bug fixes
+
+* Fix possible report corruption when using `notify()` from multiple threads
+  when configured to skip capturing/reporting background thread contents
+  (generally only Unity games).
+
 ## 5.23.0 (2019-12-10)
 
 This release removes support for reporting 'partial' or 'minimal' crash reports

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

@@ -1658,9 +1658,6 @@ void bsg_kscrashreport_writeStandardReport(
 
     bsg_ksjsonendEncode(bsg_getJsonContext(writer));
 
-    if (!bsg_ksfuflushWriteBuffer(fd)) {
-        BSG_KSLOG_ERROR("Failed to flush write buffer");
-    }
     close(fd);
 }
 

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

@@ -263,9 +263,6 @@ bool bsg_kscrashstate_i_saveState(const BSG_KSCrash_State *const state,
         goto done;
     }
     result = bsg_ksjsonendEncode(&JSONContext);
-    if (!bsg_ksfuflushWriteBuffer(fd)) {
-        BSG_KSLOG_ERROR(@"Failed to flush write buffer");
-    }
 
 done:
     close(fd);

Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSFileUtils.c

@@ -33,11 +33,6 @@
 #include <string.h>
 #include <unistd.h>
 
-#define BUFFER_SIZE 65536
-
-char charBuffer[BUFFER_SIZE];
-ssize_t bufferLen = 0;
-
 const char *bsg_ksfulastPathEntry(const char *const path) {
     if (path == NULL) {
         return NULL;
@@ -47,32 +42,42 @@ const char *bsg_ksfulastPathEntry(const char *const path) {
     return lastFile == NULL ? path : lastFile + 1;
 }
 
-bool bsg_ksfuflushWriteBuffer(const int fd) {
-    const char *pos = charBuffer;
-    while (bufferLen > 0) {
-        ssize_t bytesWritten = write(fd, pos, (size_t)bufferLen);
-        if (bytesWritten == -1) {
-            BSG_KSLOG_ERROR("Could not write to fd %d: %s", fd,
-                            strerror(errno));
-            return false;
-        }
-        bufferLen -= bytesWritten;
-        pos += bytesWritten;
-    }
-    return true;
-}
-
 bool bsg_ksfuwriteBytesToFD(const int fd, const char *const bytes,
                             ssize_t length) {
+    ssize_t bytesRemaining = length;
+    ssize_t bytesWritten = 0;
+    const char *unwrittenBytes = (const char *)bytes;
 
-    for (ssize_t k = 0; k < length; k++) {
-        if (bufferLen >= BUFFER_SIZE) {
-            if (!bsg_ksfuflushWriteBuffer(fd)) {
-                return false;
-            }
+    // write(2) attempts to write the entire length but may write less than
+    // that, the exact number of bytes are specified in the return value. In
+    // the (unlikely) event that the bytes written are less than length, this
+    // function retries with the remaining bytes until all bytes are written,
+    // handling potential error cases as needed.
+    while (bytesRemaining > 0) {
+      bytesWritten = write(fd, unwrittenBytes, bytesRemaining);
+      if (bytesWritten == -1) {
+        // Retry as-is if a signal interrupt occurred, as its a recoverable
+        // error. Otherwise exit early as the file descriptor cannot be written
+        // (due to lack of disk space, invalid fd, invalid file offset etc).
+        //
+        // write(2): Upon successful completion the number of bytes which were
+        // written is returned.  Otherwise, a -1 is returned and the global
+        // variable errno is set to indicate the error.
+        //
+        // ERRORS
+        //   The write(), writev(), and pwrite() system calls will fail and
+        //   the file pointer will remain unchanged if:
+        //
+        //   ...
+        //   [EINTR] A signal interrupts the write before it could be completed.
+        if (errno != EINTR) {
+          return false; // Unrecoverable
         }
-        charBuffer[bufferLen] = bytes[k];
-        bufferLen++;
+      } else {
+        bytesRemaining -= bytesWritten;
+        unwrittenBytes += bytesWritten;
+      }
     }
+
     return true;
 }

Tests/KSCrash/KSFileUtils_Tests.m

@@ -55,7 +55,6 @@ - (void) testWriteBytesToFD
     XCTAssertTrue(fd >= 0, @"");
     bool result = bsg_ksfuwriteBytesToFD(fd, [expected cStringUsingEncoding:NSUTF8StringEncoding], stringLength);
     XCTAssertTrue(result, @"");
-    bsg_ksfuflushWriteBuffer(fd);
     NSString* actual = [NSString stringWithContentsOfFile:path encoding:NSUTF8StringEncoding error:&error];
     XCTAssertNil(error, @"");
     XCTAssertEqualObjects(actual, expected, @"");

features/fixtures/ios-swift-cocoapods/iOSTestApp.xcodeproj/project.pbxproj

@@ -17,6 +17,7 @@
 		8A72A0382396574F00328051 /* CustomPluginNotifierDescriptionScenario.m in Sources */ = {isa = PBXBuildFile; fileRef = 8A72A0372396574F00328051 /* CustomPluginNotifierDescriptionScenario.m */; };
 		8A840FBA21AF5C450041DBFA /* SwiftAssertion.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8A840FB921AF5C450041DBFA /* SwiftAssertion.swift */; };
 		8A98400320FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.m in Sources */ = {isa = PBXBuildFile; fileRef = 8A98400220FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.m */; };
+		8AA05A2F23BE700C00C7AD00 /* ManyConcurrentNotifyNoBackgroundThreads.m in Sources */ = {isa = PBXBuildFile; fileRef = 8AA05A2E23BE700C00C7AD00 /* ManyConcurrentNotifyNoBackgroundThreads.m */; };
 		8AA7C2D72327DD3D002255B2 /* ConfigChangesAfterStartScenarios.m in Sources */ = {isa = PBXBuildFile; fileRef = 8AA7C2D52327DD3D002255B2 /* ConfigChangesAfterStartScenarios.m */; };
 		8AB1081923301FE600672818 /* ReleaseStageScenarios.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8AB1081823301FE600672818 /* ReleaseStageScenarios.swift */; };
 		8AB8866420404DD30003E444 /* AppDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8AB8866320404DD30003E444 /* AppDelegate.swift */; };
@@ -92,6 +93,8 @@
 		8A840FB921AF5C450041DBFA /* SwiftAssertion.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = SwiftAssertion.swift; sourceTree = "<group>"; };
 		8A98400120FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = AutoSessionCustomVersionScenario.h; sourceTree = "<group>"; };
 		8A98400220FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = AutoSessionCustomVersionScenario.m; sourceTree = "<group>"; };
+		8AA05A2D23BE700C00C7AD00 /* ManyConcurrentNotifyNoBackgroundThreads.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = ManyConcurrentNotifyNoBackgroundThreads.h; sourceTree = "<group>"; };
+		8AA05A2E23BE700C00C7AD00 /* ManyConcurrentNotifyNoBackgroundThreads.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = ManyConcurrentNotifyNoBackgroundThreads.m; sourceTree = "<group>"; };
 		8AA7C2D52327DD3D002255B2 /* ConfigChangesAfterStartScenarios.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ConfigChangesAfterStartScenarios.m; sourceTree = "<group>"; };
 		8AA7C2D62327DD3D002255B2 /* ConfigChangesAfterStartScenarios.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ConfigChangesAfterStartScenarios.h; sourceTree = "<group>"; };
 		8AB1081823301FE600672818 /* ReleaseStageScenarios.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ReleaseStageScenarios.swift; sourceTree = "<group>"; };
@@ -333,6 +336,8 @@
 				8AF8FCAD22BD23BA00A967CA /* HandledInternalNotifyScenario.swift */,
 				8A72A0362396574F00328051 /* CustomPluginNotifierDescriptionScenario.h */,
 				8A72A0372396574F00328051 /* CustomPluginNotifierDescriptionScenario.m */,
+				8AA05A2D23BE700C00C7AD00 /* ManyConcurrentNotifyNoBackgroundThreads.h */,
+				8AA05A2E23BE700C00C7AD00 /* ManyConcurrentNotifyNoBackgroundThreads.m */,
 			);
 			path = scenarios;
 			sourceTree = "<group>";
@@ -456,6 +461,7 @@
 				F429561C9CFE8750B030F369 /* NSExceptionScenario.swift in Sources */,
 				8A98400320FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.m in Sources */,
 				F42955E0916B8851F074D9B3 /* UserEmailScenario.swift in Sources */,
+				8AA05A2F23BE700C00C7AD00 /* ManyConcurrentNotifyNoBackgroundThreads.m in Sources */,
 				F4295968571A4118D6A4606A /* UserEnabledScenario.swift in Sources */,
 				F4295A036B228AF608641699 /* UserDisabledScenario.swift in Sources */,
 				8A14F0F62282D4AE00337B05 /* ReportOOMsDisabledScenario.m in Sources */,

features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/ManyConcurrentNotifyNoBackgroundThreads.h

@@ -0,0 +1,5 @@
+#import "Scenario.h"
+
+@interface ManyConcurrentNotifyNoBackgroundThreads : Scenario
+
+@end

features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/ManyConcurrentNotifyNoBackgroundThreads.m

@@ -0,0 +1,46 @@
+#import "ManyConcurrentNotifyNoBackgroundThreads.h"
+
+@interface ManyConcurrentNotifyNoBackgroundThreads ()
+@property (nonatomic) dispatch_queue_t queue1;
+@property (nonatomic) dispatch_queue_t queue2;
+@end
+
+@interface BarError : NSError
+@end
+@implementation BarError
+@end
+
+@implementation ManyConcurrentNotifyNoBackgroundThreads
+
+- (instancetype)initWithConfig:(BugsnagConfiguration *)config {
+    if (self = [super initWithConfig:config]) {
+        _queue1 = dispatch_queue_create("Log Queue 1", DISPATCH_QUEUE_CONCURRENT);
+        _queue2 = dispatch_queue_create("Log Queue 2", DISPATCH_QUEUE_CONCURRENT);
+    }
+    return self;
+}
+
+- (void)run {
+    for (int i = 0; i < 4; i++) {
+        NSString *message = [NSString stringWithFormat:@"Err %ld", (long)i];
+        [self logError:[BarError errorWithDomain:@"com.example"
+                                            code:401 + i
+                                        userInfo:@{NSLocalizedDescriptionKey: message}]];
+    }
+}
+
+- (void)logError:(NSError *)error {
+    dispatch_async(self.queue1, ^{
+        [Bugsnag notifyError:error];
+    });
+    dispatch_async(self.queue2, ^{
+        [Bugsnag notifyError:error];
+    });
+}
+
+- (void)startBugsnag {
+    self.config.autoTrackSessions = NO;
+    [super startBugsnag];
+    [Bugsnag setSuspendThreadsForUserReported:NO];
+}
+@end

features/handled_errors.feature

@@ -67,3 +67,17 @@ Scenario: Reporting handled errors concurrently
         | FooError                | Err 1   |
         | FooError                | Err 2   |
         | FooError                | Err 3   |
+
+Scenario: Reporting handled errors concurrently in an environment without background thread reporting
+    When I run "ManyConcurrentNotifyNoBackgroundThreads"
+    And I wait for a request
+    Then the request is valid for the error reporting API
+    And the "Bugsnag-API-Key" header equals "a35a2a72bd230ac0aa0f52715bbdc6aa"
+    And the payload field "notifier.name" equals "iOS Bugsnag Notifier"
+    And the payload field "events" is an array with 8 elements
+    And each event in the payload matches one of:
+        | exceptions.0.errorClass | exceptions.0.message |
+        | BarError                | Err 0   |
+        | BarError                | Err 1   |
+        | BarError                | Err 2   |
+        | BarError                | Err 3   |