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: Guard dereferencing of stack frame pointer in SentryBacktrace #4268

Merged
merged 4 commits into from
Aug 14, 2024

Conversation

indragiek
Copy link
Member

📜 Description

This occasionally crashes when trying to read the stack frame. The stack bounds check is insufficient to catch this case, so we can try adding a guard when reading the memory. We may still get garbage results for returnAddress, but that would just result in an unknown symbol at symbolication time rather than a runtime crash.

💡 Motivation and Context

Fix a crash reported by multiple customers.

💚 How did you test it?

We have no reliable local repro for this crash. I tested to make sure that the basic profiling functionality still works with this check in place.

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

indragiek and others added 2 commits August 9, 2024 19:27
This occasionally crashes when trying to read the stack frame. The stack bounds check is insufficient to catch this case, so we can try adding a guard when reading the memory. We may still get garbage results for `returnAddress`, but that would just result in an unknown symbol at symbolication time rather than a runtime crash.
Copy link

github-actions bot commented Aug 10, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1249.71 ms 1257.65 ms 7.94 ms
Size 21.58 KiB 706.21 KiB 684.63 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
ae1fe61 1231.81 ms 1252.76 ms 20.95 ms
32ac934 1238.86 ms 1243.37 ms 4.52 ms
98a8c16 1234.69 ms 1265.02 ms 30.33 ms
5f8ee7a 1253.96 ms 1264.98 ms 11.02 ms
01a28a9 1225.55 ms 1249.96 ms 24.41 ms
31208ed 1217.45 ms 1246.69 ms 29.24 ms
2719ce6 1211.75 ms 1237.16 ms 25.41 ms
297f460 1234.81 ms 1255.27 ms 20.45 ms
881a955 1230.98 ms 1246.22 ms 15.24 ms
84fb4d9 1237.76 ms 1255.46 ms 17.70 ms

App size

Revision Plain With Sentry Diff
ae1fe61 21.58 KiB 698.91 KiB 677.33 KiB
32ac934 21.58 KiB 616.72 KiB 595.14 KiB
98a8c16 20.76 KiB 431.00 KiB 410.24 KiB
5f8ee7a 22.85 KiB 411.93 KiB 389.08 KiB
01a28a9 22.85 KiB 405.39 KiB 382.55 KiB
31208ed 20.76 KiB 435.26 KiB 414.50 KiB
2719ce6 20.76 KiB 435.13 KiB 414.37 KiB
297f460 21.58 KiB 629.83 KiB 608.24 KiB
881a955 22.85 KiB 407.63 KiB 384.79 KiB
84fb4d9 22.84 KiB 402.57 KiB 379.72 KiB

Previous results on branch: indragiek/fix-stack-frame-deref

Startup times

Revision Plain With Sentry Diff
c28e6db 1229.94 ms 1245.06 ms 15.12 ms

App size

Revision Plain With Sentry Diff
c28e6db 21.58 KiB 700.22 KiB 678.64 KiB

@indragiek indragiek marked this pull request as ready for review August 14, 2024 19:51
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.607%. Comparing base (df2835d) to head (5feda2a).
Report is 1 commits behind head on main.

Files Patch % Lines
Sources/Sentry/SentryBacktrace.cpp 50.000% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4268       +/-   ##
=============================================
+ Coverage   91.601%   91.607%   +0.005%     
=============================================
  Files          617       616        -1     
  Lines        50045     49959       -86     
  Branches     18096     18029       -67     
=============================================
- Hits         45842     45766       -76     
+ Misses        4110      4102        -8     
+ Partials        93        91        -2     
Files Coverage Δ
Sources/Sentry/SentryBacktrace.cpp 85.714% <50.000%> (-1.051%) ⬇️

... and 19 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 df2835d...5feda2a. Read the comment docs.

@armcknight armcknight merged commit 66b6cd4 into main Aug 14, 2024
64 of 66 checks passed
@armcknight armcknight deleted the indragiek/fix-stack-frame-deref branch August 14, 2024 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants