Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid call to possibly crashing mach_thread_deallocate #3364

Merged
merged 4 commits into from
Oct 30, 2023

Conversation

armcknight
Copy link
Member

In an attempt to fix #3354, but unverified because we have no repro.

The crash report in the attached issue points to line 49 in SentryThreadHandle.cpp, but that is not actually the line crashing if you look at the source code, which is constructing a C++ unique pointer, the line above that is actually a macro: when it expands in place, the actual line of code crashing on line 49 is actually the call to mach_port_deallocate wrapped in the macro–see the definition for SENTRY_PROF_LOG_KERN_RETURN:

#define SENTRY_PROF_LOG_KERN_RETURN(statement) \
({ \
const kern_return_t __log_kr = statement; \
if (__log_kr != KERN_SUCCESS) { \
SENTRY_PROF_LOG_ERROR("%s failed with kern return code: %d, description: %s", \
#statement, __log_kr, sentry::kernelReturnCodeDescription(__log_kr)); \
} \
__log_kr; \
})
)

We hope that avoiding the call to mach_port_deallocate avoids the whatever issue is occurring, and also that the new logic is potentially more efficient. See https://codereview.chromium.org/276043002/ for more information.

@codecov
Copy link

codecov bot commented Oct 28, 2023

Codecov Report

Merging #3364 (b855bb6) into main (2124551) will increase coverage by 0.009%.
The diff coverage is 100.000%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3364       +/-   ##
=============================================
+ Coverage   89.251%   89.260%   +0.009%     
=============================================
  Files          500       500               
  Lines        54657     54619       -38     
  Branches     19618     19601       -17     
=============================================
- Hits         48782     48753       -29     
+ Misses        5004      4997        -7     
+ Partials       871       869        -2     
Files Coverage Δ
Sources/Sentry/SentryBacktrace.cpp 86.607% <ø> (ø)
Sources/Sentry/SentryThreadHandle.cpp 70.879% <100.000%> (+1.480%) ⬆️

... and 11 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2124551...b855bb6. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Oct 28, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1243.76 ms 1256.28 ms 12.52 ms
Size 22.85 KiB 411.75 KiB 388.91 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3f1be0f 1208.12 ms 1225.72 ms 17.60 ms
72c8d84 1238.96 ms 1247.34 ms 8.38 ms
28333b6 1186.29 ms 1225.18 ms 38.89 ms
381380f 1201.56 ms 1220.08 ms 18.52 ms
8f397a7 1224.66 ms 1236.48 ms 11.82 ms
eaf3840 1264.94 ms 1274.04 ms 9.10 ms
b483671 1217.20 ms 1236.82 ms 19.62 ms
3a31fc9 1237.35 ms 1249.02 ms 11.67 ms
2124551 1265.50 ms 1276.44 ms 10.94 ms
a176fc4 1226.24 ms 1247.50 ms 21.26 ms

App size

Revision Plain With Sentry Diff
3f1be0f 20.76 KiB 414.44 KiB 393.69 KiB
72c8d84 22.85 KiB 408.88 KiB 386.03 KiB
28333b6 20.76 KiB 424.69 KiB 403.93 KiB
381380f 20.76 KiB 436.29 KiB 415.53 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
eaf3840 22.84 KiB 401.67 KiB 378.83 KiB
b483671 20.76 KiB 434.72 KiB 413.96 KiB
3a31fc9 20.76 KiB 414.45 KiB 393.69 KiB
2124551 22.85 KiB 411.69 KiB 388.84 KiB
a176fc4 22.84 KiB 403.24 KiB 380.39 KiB

Previous results on branch: armcknight/fix/3223-backtrace-kern-invalid-address

Startup times

Revision Plain With Sentry Diff
0ffd532 1245.10 ms 1248.20 ms 3.10 ms

App size

Revision Plain With Sentry Diff
0ffd532 22.85 KiB 411.64 KiB 388.79 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a changelog entry, @armcknight. LGTM

@armcknight armcknight merged commit 728804f into main Oct 30, 2023
@armcknight armcknight deleted the armcknight/fix/3223-backtrace-kern-invalid-address branch October 30, 2023 19:25
@github-actions
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- avoid call to possibly crashing `mach_thread_deallocate` ([#3364](https://github.com/getsentry/sentry-cocoa/pull/3364))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 6ef8a51

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants