diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c33a66c2..6394cf04a 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c index a9ea67e11..46af061dd 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c +++ b/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); } diff --git a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashState.m b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashState.m index 7c47457d7..d2008efaa 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashState.m +++ b/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); diff --git a/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSFileUtils.c b/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSFileUtils.c index 029590042..16b326a97 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSFileUtils.c +++ b/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSFileUtils.c @@ -43,11 +43,6 @@ #define BSG_KSFU_WriteFmtBufferSize 1024 #endif -#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; @@ -57,33 +52,43 @@ 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) { - - for (ssize_t k = 0; k < length; k++) { - if (bufferLen >= BUFFER_SIZE) { - if (!bsg_ksfuflushWriteBuffer(fd)) { - return false; - } + ssize_t bytesRemaining = length; + ssize_t bytesWritten = 0; + const char *unwrittenBytes = (const char *)bytes; + + // 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; } diff --git a/Tests/KSCrash/KSFileUtils_Tests.m b/Tests/KSCrash/KSFileUtils_Tests.m index 85e2f1d9a..66029afa9 100755 --- a/Tests/KSCrash/KSFileUtils_Tests.m +++ b/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, @""); diff --git a/features/fixtures/ios-swift-cocoapods/iOSTestApp.xcodeproj/project.pbxproj b/features/fixtures/ios-swift-cocoapods/iOSTestApp.xcodeproj/project.pbxproj index 8444df44d..7e1f4ac84 100644 --- a/features/fixtures/ios-swift-cocoapods/iOSTestApp.xcodeproj/project.pbxproj +++ b/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 = ""; }; 8A98400120FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = AutoSessionCustomVersionScenario.h; sourceTree = ""; }; 8A98400220FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = AutoSessionCustomVersionScenario.m; sourceTree = ""; }; + 8AA05A2D23BE700C00C7AD00 /* ManyConcurrentNotifyNoBackgroundThreads.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = ManyConcurrentNotifyNoBackgroundThreads.h; sourceTree = ""; }; + 8AA05A2E23BE700C00C7AD00 /* ManyConcurrentNotifyNoBackgroundThreads.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = ManyConcurrentNotifyNoBackgroundThreads.m; sourceTree = ""; }; 8AA7C2D52327DD3D002255B2 /* ConfigChangesAfterStartScenarios.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ConfigChangesAfterStartScenarios.m; sourceTree = ""; }; 8AA7C2D62327DD3D002255B2 /* ConfigChangesAfterStartScenarios.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ConfigChangesAfterStartScenarios.h; sourceTree = ""; }; 8AB1081823301FE600672818 /* ReleaseStageScenarios.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ReleaseStageScenarios.swift; sourceTree = ""; }; @@ -333,6 +336,8 @@ 8AF8FCAD22BD23BA00A967CA /* HandledInternalNotifyScenario.swift */, 8A72A0362396574F00328051 /* CustomPluginNotifierDescriptionScenario.h */, 8A72A0372396574F00328051 /* CustomPluginNotifierDescriptionScenario.m */, + 8AA05A2D23BE700C00C7AD00 /* ManyConcurrentNotifyNoBackgroundThreads.h */, + 8AA05A2E23BE700C00C7AD00 /* ManyConcurrentNotifyNoBackgroundThreads.m */, ); path = scenarios; sourceTree = ""; @@ -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 */, diff --git a/features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/ManyConcurrentNotifyNoBackgroundThreads.h b/features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/ManyConcurrentNotifyNoBackgroundThreads.h new file mode 100644 index 000000000..b41a0b7c0 --- /dev/null +++ b/features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/ManyConcurrentNotifyNoBackgroundThreads.h @@ -0,0 +1,5 @@ +#import "Scenario.h" + +@interface ManyConcurrentNotifyNoBackgroundThreads : Scenario + +@end diff --git a/features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/ManyConcurrentNotifyNoBackgroundThreads.m b/features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/ManyConcurrentNotifyNoBackgroundThreads.m new file mode 100644 index 000000000..6eefd97ca --- /dev/null +++ b/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 diff --git a/features/handled_errors.feature b/features/handled_errors.feature index 37bea9e73..c7c2c51b4 100644 --- a/features/handled_errors.feature +++ b/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 |