fix(macos): Don't overwrite exception value with notable addresses for AppKit _crashOnException crashes#7734
fix(macos): Don't overwrite exception value with notable addresses for AppKit _crashOnException crashes#7734
Conversation
…r AppKit _crashOnException crashes When an NSException on macOS falls through to the mach exception monitor (via AppKit's _crashOnException:), enhanceValueFromNotableAddresses overwrites the exception value with garbage strings from registers/stack memory (e.g., 0x000000019de89d54 start + 7184). This happens for macOS apps that don't use SentryCrashExceptionApplication or have enableUncaughtNSExceptionReporting disabled. The fix detects this specific crash pattern (EXC_BREAKPOINT with a crashed-thread frame inside AppKit) and skips the notable address enhancement, preserving the original mach exception description. Relates to #1562
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Bug Fixes 🐛
Documentation 📚
Internal Changes 🔧Deps
Other
🤖 This preview updates automatically when you update the PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7734 +/- ##
=============================================
- Coverage 85.366% 85.102% -0.265%
=============================================
Files 487 487
Lines 28996 29012 +16
Branches 12553 12556 +3
=============================================
- Hits 24753 24690 -63
- Misses 4195 4274 +79
Partials 48 48
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
denrase
requested review from
itaybre,
noahsmartin,
philipphofmann and
philprime
as code owners
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Nil image name falsely matches AppKit substring check
- The AppKit frame check now first assigns and validates
imageNameas non-nil before callingrangeOfString:, preventing nil receivers from falsely matching.
- The AppKit frame check now first assigns and validates
Or push these changes by commenting:
@cursor push f56fc09dda
Preview (f56fc09dda)
diff --git a/Sources/Sentry/SentryCrashReportConverter.m b/Sources/Sentry/SentryCrashReportConverter.m
--- a/Sources/Sentry/SentryCrashReportConverter.m
+++ b/Sources/Sentry/SentryCrashReportConverter.m
@@ -543,7 +543,8 @@
for (NSUInteger i = 0; i < limit; i++) {
uintptr_t addr = (uintptr_t)[frames[i][@"instruction_addr"] unsignedLongLongValue];
NSDictionary *image = [self binaryImageForAddress:addr];
- if (image != nil && [image[@"name"] rangeOfString:@"AppKit"].location != NSNotFound) {
+ NSString *imageName = image[@"name"];
+ if (imageName != nil && [imageName rangeOfString:@"AppKit"].location != NSNotFound) {
return YES;
}
}This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| for (NSUInteger i = 0; i < limit; i++) { | ||
| uintptr_t addr = (uintptr_t)[frames[i][@"instruction_addr"] unsignedLongLongValue]; | ||
| NSDictionary *image = [self binaryImageForAddress:addr]; | ||
| if (image != nil && [image[@"name"] rangeOfString:@"AppKit"].location != NSNotFound) { |
There was a problem hiding this comment.
Nil image name falsely matches AppKit substring check
Low Severity
If image[@"name"] is nil, [nil rangeOfString:@"AppKit"] returns a zero-filled NSRange with location = 0. Since NSNotFound is NSIntegerMax, the comparison .location != NSNotFound evaluates to YES, causing a binary image with no name to be falsely identified as AppKit. This would make isMachExceptionFromAppKitCrashOnException return YES incorrectly, suppressing the notable address enhancement for crashes that aren't _crashOnException:.
There was a problem hiding this comment.
I don't think there are images with no names, but we should cover ourselves just in case
itaybre
left a comment
itaybre
left a comment
There was a problem hiding this comment.
Mostly looks good, I am just worried about this logic including other exceptions too
| for (NSUInteger i = 0; i < limit; i++) { | ||
| uintptr_t addr = (uintptr_t)[frames[i][@"instruction_addr"] unsignedLongLongValue]; | ||
| NSDictionary *image = [self binaryImageForAddress:addr]; | ||
| if (image != nil && [image[@"name"] rangeOfString:@"AppKit"].location != NSNotFound) { |
There was a problem hiding this comment.
I don't think there are images with no names, but we should cover ourselves just in case
| for (NSUInteger i = 0; i < limit; i++) { | ||
| uintptr_t addr = (uintptr_t)[frames[i][@"instruction_addr"] unsignedLongLongValue]; | ||
| NSDictionary *image = [self binaryImageForAddress:addr]; | ||
| if (image != nil && [image[@"name"] rangeOfString:@"AppKit"].location != NSNotFound) { |
There was a problem hiding this comment.
m: Are we 100% that both conditions are enough (exception type and appkit present)?
We already removed _crashOnException from the stacktrace, right? If not adding that could help
There was a problem hiding this comment.
That's a good point, will check if we have better heuristics for this so we only filter out unusual messages.
2 participants
📜 Description
When an NSException on macOS falls through to the mach exception monitor (via AppKit's _crashOnException:), enhanceValueFromNotableAddresses overwrites the exception value with "garbage" strings from registers/stack memory (e.g., 0x000000019de89d54 start + 7184). This happens for macOS apps that don't use SentryCrashExceptionApplication or have enableUncaughtNSExceptionReporting disabled.
The fix detects this specific crash pattern (EXC_BREAKPOINT with a crashed-thread frame inside AppKit) and skips the notable address enhancement, preserving the original mach exception description.
💡 Motivation and Context
Relates to #1562
💚 How did you test it?
Unit test. Tested with sample app.
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.