From 2fb60c2129fcc12052d33e88e73ab7e5758bc0a2 Mon Sep 17 00:00:00 2001 From: Delisa Mason Date: Wed, 20 Mar 2019 09:28:06 +0000 Subject: [PATCH] fix: Use exception stack when available For thrown exceptions, use the exception stack trace rather than a generated one to allow notify() to be called from a different thread or handler. --- CHANGELOG.md | 14 ++++++++ Source/BugsnagCrashReport.h | 3 +- Source/BugsnagCrashSentry.h | 1 + Source/BugsnagCrashSentry.m | 2 ++ Source/BugsnagNotifier.m | 31 +++++++---------- .../Source/KSCrash/Recording/BSG_KSCrash.h | 2 ++ .../Source/KSCrash/Recording/BSG_KSCrash.m | 11 +++++++ .../Source/KSCrash/Recording/BSG_KSCrashC.c | 8 ++++- .../Source/KSCrash/Recording/BSG_KSCrashC.h | 4 +++ .../Recording/Sentry/BSG_KSCrashSentry_User.c | 33 ++++++++++++------- .../Recording/Sentry/BSG_KSCrashSentry_User.h | 5 ++- .../iOSTestApp.xcodeproj/project.pbxproj | 6 ++++ .../scenarios/NSExceptionShiftScenario.h | 17 ++++++++++ .../scenarios/NSExceptionShiftScenario.m | 29 ++++++++++++++++ features/handled_errors.feature | 14 ++++++++ 15 files changed, 146 insertions(+), 34 deletions(-) create mode 100644 features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/NSExceptionShiftScenario.h create mode 100644 features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/NSExceptionShiftScenario.m diff --git a/CHANGELOG.md b/CHANGELOG.md index 82d479923..4df50932b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,20 @@ Changelog ========= +## TBD + +### Bug fixes + +* Fix generating an incorrect stacktrace used when logging an exception to + Bugsnag from a location other than the original call site (for example, from a + logging function or across threads). If an exception was raised/thrown, then + the resulting Bugsnag report from `notify()` will now use the `NSException` + instance's call stack addresses to construct the stacktrace, ignoring depth. + This fixes an issue in macOS exception reporting where `reportException` is + reporting the handler code stacktrace rather than the reported exception + stack. + [#334](https://github.com/bugsnag/bugsnag-cocoa/pull/334) + ## 5.19.0 (2019-02-28) Note for Carthage users: this release updates the Xcode configuration to the settings recommended by Xcode 10. diff --git a/Source/BugsnagCrashReport.h b/Source/BugsnagCrashReport.h index 027a9fde1..f39c822d6 100644 --- a/Source/BugsnagCrashReport.h +++ b/Source/BugsnagCrashReport.h @@ -181,7 +181,8 @@ __deprecated_msg("Use toJson: instead."); */ @property(readonly, copy, nonnull) NSDictionary *overrides; /** - * Number of frames to discard at the top of the stacktrace + * Number of frames to discard at the top of the generated stacktrace. + * Stacktraces from raised exceptions are unaffected. */ @property(readwrite) NSUInteger depth; /** diff --git a/Source/BugsnagCrashSentry.h b/Source/BugsnagCrashSentry.h index 0913fb022..3c19c1f42 100644 --- a/Source/BugsnagCrashSentry.h +++ b/Source/BugsnagCrashSentry.h @@ -20,6 +20,7 @@ - (void)reportUserException:(NSString *)reportName reason:(NSString *)reportMessage + originalException:(NSException *)ex handledState:(NSDictionary *)handledState appState:(NSDictionary *)appState callbackOverrides:(NSDictionary *)overrides diff --git a/Source/BugsnagCrashSentry.m b/Source/BugsnagCrashSentry.m index 3a64a1707..809d48bdf 100644 --- a/Source/BugsnagCrashSentry.m +++ b/Source/BugsnagCrashSentry.m @@ -43,6 +43,7 @@ - (void)install:(BugsnagConfiguration *)config - (void)reportUserException:(NSString *)reportName reason:(NSString *)reportMessage + originalException:(NSException *)ex handledState:(NSDictionary *)handledState appState:(NSDictionary *)appState callbackOverrides:(NSDictionary *)overrides @@ -52,6 +53,7 @@ - (void)reportUserException:(NSString *)reportName [[BSG_KSCrash sharedInstance] reportUserException:reportName reason:reportMessage + originalException:ex handledState:handledState appState:appState callbackOverrides:overrides diff --git a/Source/BugsnagNotifier.m b/Source/BugsnagNotifier.m index afef2f6bf..8801601d8 100644 --- a/Source/BugsnagNotifier.m +++ b/Source/BugsnagNotifier.m @@ -443,8 +443,10 @@ - (void)notifyError:(NSError *)error [BugsnagHandledState handledStateWithSeverityReason:HandledError severity:BSGSeverityWarning attrValue:error.domain]; - [self notify:NSStringFromClass([error class]) - message:error.localizedDescription + NSException *wrapper = [NSException exceptionWithName:NSStringFromClass([error class]) + reason:error.localizedDescription + userInfo:error.userInfo]; + [self notify:wrapper handledState:state block:^(BugsnagCrashReport *_Nonnull report) { NSMutableDictionary *metadata = [report.metaData mutableCopy]; @@ -472,20 +474,14 @@ - (void)notifyException:(NSException *)exception handledStateWithSeverityReason:UserSpecifiedSeverity severity:severity attrValue:nil]; - [self notify:exception.name ?: NSStringFromClass([exception class]) - message:exception.reason - handledState:state - block:block]; + [self notify:exception handledState:state block:block]; } - (void)notifyException:(NSException *)exception block:(void (^)(BugsnagCrashReport *))block { BugsnagHandledState *state = [BugsnagHandledState handledStateWithSeverityReason:HandledException]; - [self notify:exception.name ?: NSStringFromClass([exception class]) - message:exception.reason - handledState:state - block:block]; + [self notify:exception handledState:state block:block]; } - (void)internalClientNotify:(NSException *_Nonnull)exception @@ -506,20 +502,14 @@ - (void)internalClientNotify:(NSException *_Nonnull)exception severity:BSGParseSeverity(severity) attrValue:logLevel]; - [self notify:exception.name ?: NSStringFromClass([exception class]) - message:exception.reason - handledState:state - block:^(BugsnagCrashReport *_Nonnull report) { - if (block) { - block(report); - } - }]; + [self notify:exception handledState:state block:block]; } -- (void)notify:(NSString *)exceptionName - message:(NSString *)message +- (void)notify:(NSException *)exception handledState:(BugsnagHandledState *_Nonnull)handledState block:(void (^)(BugsnagCrashReport *))block { + NSString *exceptionName = exception.name ?: NSStringFromClass([exception class]); + NSString *message = exception.reason; [self.sessionTracker handleHandledErrorEvent]; BugsnagCrashReport *report = [[BugsnagCrashReport alloc] @@ -550,6 +540,7 @@ - (void)notify:(NSString *)exceptionName [self.crashSentry reportUserException:reportName reason:reportMessage + originalException:exception handledState:[handledState toJson] appState:[self.state toDictionary] callbackOverrides:report.overrides diff --git a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.h b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.h index 91fefa031..ef9712b58 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.h +++ b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.h @@ -173,6 +173,7 @@ typedef enum { * * @param name The exception name (for namespacing exception types). * @param reason A description of why the exception occurred + * @param exception The exception which was thrown (if any) * @param handledState The severity, reason, and handled-ness of the report * @param appState breadcrumbs and other app environmental info * @param overrides Report fields overridden by callbacks, collated in the @@ -185,6 +186,7 @@ typedef enum { */ - (void)reportUserException:(NSString *)name reason:(NSString *)reason + originalException:(NSException *)exception handledState:(NSDictionary *)handledState appState:(NSDictionary *)appState callbackOverrides:(NSDictionary *)overrides diff --git a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m index 13ec842ae..8611a9493 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m +++ b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m @@ -347,6 +347,7 @@ - (void)deleteAllReports { - (void)reportUserException:(NSString *)name reason:(NSString *)reason + originalException:(NSException *)exception handledState:(NSDictionary *)handledState appState:(NSDictionary *)appState callbackOverrides:(NSDictionary *)overrides @@ -356,7 +357,17 @@ - (void)reportUserException:(NSString *)name terminateProgram:(BOOL)terminateProgram { const char *cName = [name cStringUsingEncoding:NSUTF8StringEncoding]; const char *cReason = [reason cStringUsingEncoding:NSUTF8StringEncoding]; + NSArray *addresses = [exception callStackReturnAddresses]; + NSUInteger numFrames = [addresses count]; + uintptr_t *callstack = malloc(numFrames * sizeof(*callstack)); + for (NSUInteger i = 0; i < numFrames; i++) { + callstack[i] = [addresses[i] unsignedLongValue]; + } + if (numFrames > 0) { + depth = 0; // reset depth if the stack does not need to be generated + } bsg_kscrash_reportUserException(cName, cReason, + callstack, numFrames, [handledState[@"currentSeverity"] UTF8String], [self encodeAsJSONString:handledState], [self encodeAsJSONString:overrides], diff --git a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c index 0165b7de5..ce937d1c5 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c +++ b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c @@ -285,6 +285,8 @@ void bsg_kscrash_setCrashNotifyCallback( } void bsg_kscrash_reportUserException(const char *name, const char *reason, + uintptr_t *stackAddresses, + unsigned long stackLength, const char *severity, const char *handledState, const char *overrides, @@ -293,7 +295,11 @@ void bsg_kscrash_reportUserException(const char *name, const char *reason, const char *config, int discardDepth, bool terminateProgram) { - bsg_kscrashsentry_reportUserException(name, reason, severity, handledState, overrides, + bsg_kscrashsentry_reportUserException(name, reason, + stackAddresses, + stackLength, + severity, + handledState, overrides, metadata, appState, config, discardDepth, terminateProgram); } diff --git a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.h b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.h index 2aeb81cc8..089858ff9 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.h +++ b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.h @@ -176,6 +176,8 @@ void bsg_kscrash_setCrashNotifyCallback( * * @param name The exception name (for namespacing exception types). * @param reason A description of why the exception occurred. + * @param stackAddresses An array of addresses or NULL + * @param stackLength The number of addresses in stackAddresses * @param handledState The severity, reason, and handled-ness of the report * @param appState breadcrumbs and other app environmental info * @param overrides Report fields overridden by callbacks, collated in the @@ -187,6 +189,8 @@ void bsg_kscrash_setCrashNotifyCallback( * Terminate the program instead. */ void bsg_kscrash_reportUserException(const char *name, const char *reason, + uintptr_t *stackAddresses, + unsigned long stackLength, const char *severity, const char *handledState, const char *overrides, diff --git a/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.c b/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.c index e7a397e9d..1b1768f35 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.c +++ b/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.c @@ -47,7 +47,11 @@ void bsg_kscrashsentry_uninstallUserExceptionHandler(void) { bsg_g_context = NULL; } -void bsg_kscrashsentry_reportUserException(const char *name, const char *reason, + +void bsg_kscrashsentry_reportUserException(const char *name, + const char *reason, + uintptr_t *stackAddresses, + unsigned long stackLength, const char *severity, const char *handledState, const char *overrides, @@ -67,14 +71,23 @@ void bsg_kscrashsentry_reportUserException(const char *name, const char *reason, bsg_kscrashsentry_suspendThreads(); } - BSG_KSLOG_DEBUG("Fetching call stack."); - int callstackCount = 100; - uintptr_t callstack[callstackCount]; - callstackCount = backtrace((void **)callstack, callstackCount); - if (callstackCount <= 0) { - BSG_KSLOG_ERROR("backtrace() returned call stack length of %d", - callstackCount); - callstackCount = 0; + + if (stackAddresses != NULL && stackLength > 0) { + bsg_g_context->stackTrace = stackAddresses; + bsg_g_context->stackTraceLength = (int)stackLength; + } else { + BSG_KSLOG_DEBUG("Fetching call stack."); + int callstackCount = 100; + uintptr_t callstack[callstackCount]; + callstackCount = backtrace((void **)callstack, callstackCount); + if (callstackCount <= 0) { + BSG_KSLOG_ERROR("backtrace() returned call stack length of %d", + callstackCount); + callstackCount = 0; + } + BSG_KSLOG_DEBUG("Filling out stack context entries."); + bsg_g_context->stackTrace = callstack; + bsg_g_context->stackTraceLength = callstackCount; } BSG_KSLOG_DEBUG("Filling out context."); @@ -82,8 +95,6 @@ void bsg_kscrashsentry_reportUserException(const char *name, const char *reason, bsg_g_context->offendingThread = bsg_ksmachthread_self(); bsg_g_context->registersAreValid = false; bsg_g_context->crashReason = reason; - bsg_g_context->stackTrace = callstack; - bsg_g_context->stackTraceLength = callstackCount; bsg_g_context->userException.name = name; bsg_g_context->userException.handledState = handledState; bsg_g_context->userException.overrides = overrides; diff --git a/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.h b/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.h index f68f583e9..1d6744976 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.h +++ b/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.h @@ -54,7 +54,8 @@ void bsg_kscrashsentry_uninstallUserExceptionHandler(void); * @param name The exception name (for namespacing exception types). * * @param reason A description of why the exception occurred. - * + * @param stackAddresses An array of addresses or NULL + * @param stackLength The number of addresses in stackAddresses * @param handledState The severity, reason, and handled-ness of the report * @param appState breadcrumbs and other app environmental info * @param overrides Report fields overridden by callbacks, collated in the @@ -67,6 +68,8 @@ void bsg_kscrashsentry_uninstallUserExceptionHandler(void); * Terminate the program instead. */ void bsg_kscrashsentry_reportUserException(const char *name, const char *reason, + uintptr_t *stackAddresses, + unsigned long stackLength, const char *severity, const char *handledState, const char *overrides, diff --git a/features/fixtures/ios-swift-cocoapods/iOSTestApp.xcodeproj/project.pbxproj b/features/fixtures/ios-swift-cocoapods/iOSTestApp.xcodeproj/project.pbxproj index 7fb842597..f4e24ad6c 100644 --- a/features/fixtures/ios-swift-cocoapods/iOSTestApp.xcodeproj/project.pbxproj +++ b/features/fixtures/ios-swift-cocoapods/iOSTestApp.xcodeproj/project.pbxproj @@ -7,6 +7,7 @@ objects = { /* Begin PBXBuildFile section */ + 8A32DB8222424E3000EDD92F /* NSExceptionShiftScenario.m in Sources */ = {isa = PBXBuildFile; fileRef = 8A32DB8122424E3000EDD92F /* NSExceptionShiftScenario.m */; }; 8A840FBA21AF5C450041DBFA /* SwiftAssertion.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8A840FB921AF5C450041DBFA /* SwiftAssertion.swift */; }; 8A98400320FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.m in Sources */ = {isa = PBXBuildFile; fileRef = 8A98400220FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.m */; }; 8AB8866420404DD30003E444 /* AppDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8AB8866320404DD30003E444 /* AppDelegate.swift */; }; @@ -58,6 +59,8 @@ /* Begin PBXFileReference section */ 4994F05E0421A0B037DD2CC5 /* Pods_iOSTestApp.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_iOSTestApp.framework; sourceTree = BUILT_PRODUCTS_DIR; }; + 8A32DB8022424E3000EDD92F /* NSExceptionShiftScenario.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = NSExceptionShiftScenario.h; sourceTree = ""; }; + 8A32DB8122424E3000EDD92F /* NSExceptionShiftScenario.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = NSExceptionShiftScenario.m; sourceTree = ""; }; 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 = ""; }; @@ -204,6 +207,8 @@ F42953DE2BB41023C0B07F41 /* scenarios */ = { isa = PBXGroup; children = ( + 8A32DB8022424E3000EDD92F /* NSExceptionShiftScenario.h */, + 8A32DB8122424E3000EDD92F /* NSExceptionShiftScenario.m */, 8A840FB921AF5C450041DBFA /* SwiftAssertion.swift */, F42957FA1A3724BFBDC22E14 /* NSExceptionScenario.swift */, F4295F595986A279FA3BDEA7 /* UserEmailScenario.swift */, @@ -410,6 +415,7 @@ F4295836C8AF75547C675E8D /* ReleasedObjectScenario.m in Sources */, F42958881D3F34A76CADE4EC /* SwiftCrash.swift in Sources */, F42955DB6D08642528917FAB /* CxxExceptionScenario.mm in Sources */, + 8A32DB8222424E3000EDD92F /* NSExceptionShiftScenario.m in Sources */, F42954B7318A02824C65C514 /* ObjCMsgSendScenario.m in Sources */, F42953498545B853CC0B635E /* NullPointerScenario.m in Sources */, 8A840FBA21AF5C450041DBFA /* SwiftAssertion.swift in Sources */, diff --git a/features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/NSExceptionShiftScenario.h b/features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/NSExceptionShiftScenario.h new file mode 100644 index 000000000..eb4db79fd --- /dev/null +++ b/features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/NSExceptionShiftScenario.h @@ -0,0 +1,17 @@ +// +// NSExceptionShiftScenario.h +// macOSTestApp +// +// Created by Delisa on 3/20/19. +// Copyright © 2019 Bugsnag Inc. All rights reserved. +// + +#import "Scenario.h" + +NS_ASSUME_NONNULL_BEGIN + +@interface NSExceptionShiftScenario : Scenario + +@end + +NS_ASSUME_NONNULL_END diff --git a/features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/NSExceptionShiftScenario.m b/features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/NSExceptionShiftScenario.m new file mode 100644 index 000000000..1943127b3 --- /dev/null +++ b/features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/NSExceptionShiftScenario.m @@ -0,0 +1,29 @@ +#import "NSExceptionShiftScenario.h" +#import + +@implementation NSExceptionShiftScenario + +- (void)startBugsnag { + self.config.shouldAutoCaptureSessions = NO; + [super startBugsnag]; +} + +- (void)run { + [self causeAnException]; +} + +- (void)causeAnException { + @try { + @throw [NSException exceptionWithName:@"Tertiary failure" + reason:@"invalid invariant" + userInfo:nil]; + } @catch (NSException *exception) { + [self shouldNotBeInStacktrace:exception]; + } +} + +- (void)shouldNotBeInStacktrace:(NSException *)exception { + [Bugsnag notify:exception]; +} + +@end diff --git a/features/handled_errors.feature b/features/handled_errors.feature index 502c33116..eb493d192 100644 --- a/features/handled_errors.feature +++ b/features/handled_errors.feature @@ -27,3 +27,17 @@ Scenario: Reporting a handled exception And the payload field "events" is an array with 1 element And the exception "errorClass" equals "HandledExceptionScenario" And the exception "message" equals "Message: HandledExceptionScenario" + +Scenario: Reporting a handled exception's stacktrace + When I run "NSExceptionShiftScenario" + Then I should receive a request + And the request is a valid for the error reporting API + And the "Bugsnag-API-Key" header equals "a35a2a72bd230ac0aa0f52715bbdc6aa" + And the payload notifier name is correct + And the payload field "events" is an array with 1 element + And the exception "errorClass" equals "Tertiary failure" + And the exception "message" equals "invalid invariant" + And the "method" of stack frame 0 equals "__exceptionPreprocess" + And the "method" of stack frame 1 equals "objc_exception_throw" + And the "method" of stack frame 2 equals "-[NSExceptionShiftScenario causeAnException]" + And the "method" of stack frame 3 equals "-[NSExceptionShiftScenario run]"