-
-
Notifications
You must be signed in to change notification settings - Fork 371
chore: Increase maximum stacktrace length to 500 #6536
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| #define MAX_STACKTRACE_LENGTH 100 | ||
| #define MAX_STACKTRACE_LENGTH 500 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Increasing MAX_STACKTRACE_LENGTH causes stack allocation to exceed background thread limits, leading to overflow.
Severity: CRITICAL | Confidence: 0.98
🔍 Detailed Analysis
Increasing MAX_STACKTRACE_LENGTH from 100 to 500 causes the SentryThreadInfo struct to grow from ~4 KB to ~20 KB per thread. When getCurrentThreadsWithStackTrace() is called on a background thread, the Variable Length Array threadsInfos allocates approximately 1.37 MB on the stack for 70 threads. This allocation exceeds the typical 512 KB stack limit for background threads, leading to a stack overflow when multiple threads are suspended and the call stack is moderately deep.
💡 Suggested Fix
Reduce MAX_STACKTRACE_LENGTH to a value that keeps total stack allocation below 512 KB for 70 threads, or refactor SentryThreadInfo allocation to use heap memory instead of the stack.
🤖 Prompt for AI Agent
Fix this bug. In
Sources/SentryCrash/Recording/Tools/SentryCrashStackCursor_MachineContext.h at line 36:
Increasing `MAX_STACKTRACE_LENGTH` from 100 to 500 causes the `SentryThreadInfo` struct
to grow from ~4 KB to ~20 KB per thread. When `getCurrentThreadsWithStackTrace()` is
called on a background thread, the Variable Length Array `threadsInfos` allocates
approximately 1.37 MB on the stack for 70 threads. This allocation exceeds the typical
512 KB stack limit for background threads, leading to a stack overflow when multiple
threads are suspended and the call stack is moderately deep.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: @itaybre we should try to write a test for this, maybe using a limited recursive method with parallel threads. It should be an best-effort unit test as this could potentially introduce flakiness
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6536 +/- ##
=============================================
+ Coverage 86.112% 86.459% +0.347%
=============================================
Files 451 451
Lines 27593 27488 -105
Branches 11987 11970 -17
=============================================
+ Hits 23761 23766 +5
+ Misses 3786 3678 -108
+ Partials 46 44 -2
... and 22 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| db9572a | 1200.27 ms | 1234.80 ms | 34.53 ms |
| 5258fb8 | 1207.92 ms | 1234.51 ms | 26.59 ms |
| 2481950 | 1221.04 ms | 1248.98 ms | 27.94 ms |
| 2be5991 | 1228.55 ms | 1264.65 ms | 36.10 ms |
| 7d88965 | 1228.86 ms | 1248.53 ms | 19.67 ms |
| ab0ba7e | 1216.08 ms | 1242.40 ms | 26.31 ms |
| 139db8b | 1231.50 ms | 1258.19 ms | 26.69 ms |
| 78af7a9 | 1225.75 ms | 1256.98 ms | 31.23 ms |
| 5db87fa | 1218.88 ms | 1251.53 ms | 32.65 ms |
| b5a7583 | 1238.22 ms | 1263.94 ms | 25.71 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| db9572a | 23.75 KiB | 858.69 KiB | 834.93 KiB |
| 5258fb8 | 23.75 KiB | 874.45 KiB | 850.70 KiB |
| 2481950 | 23.74 KiB | 872.74 KiB | 849.00 KiB |
| 2be5991 | 23.75 KiB | 994.73 KiB | 970.98 KiB |
| 7d88965 | 23.75 KiB | 994.72 KiB | 970.98 KiB |
| ab0ba7e | 23.75 KiB | 904.54 KiB | 880.79 KiB |
| 139db8b | 23.75 KiB | 920.64 KiB | 896.89 KiB |
| 78af7a9 | 23.75 KiB | 990.00 KiB | 966.26 KiB |
| 5db87fa | 23.75 KiB | 926.65 KiB | 902.90 KiB |
| b5a7583 | 23.75 KiB | 913.44 KiB | 889.68 KiB |
| #endif | ||
|
|
||
| #define MAX_STACKTRACE_LENGTH 100 | ||
| #define MAX_STACKTRACE_LENGTH 500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h: This has a significant impact on memory usage when allocating an array of SentryThreadInfo here
| SentryThreadInfo threadsInfos[numSuspendedThreads]; |
because we allocate an array of stackEntries here
sentry-cocoa/Sources/Sentry/SentryDefaultThreadInspector.m
Lines 23 to 27 in d14bd20
| typedef struct { | |
| SentryCrashThread thread; | |
| SentryCrashStackEntry stackEntries[MAX_STACKTRACE_LENGTH]; | |
| int stackLength; | |
| } SentryThreadInfo; |
We should evaluate what the impact is and see if it's acceptable. I'm pretty sure we can improve this for the SentryDefaultThreadInspector, so it uses less memory.
This also came up in #1923 (comment)
This PR increases the stacktrace limit to 500 frames in sync to the limits defined in https://develop.sentry.dev/sdk/data-model/event-payloads/stacktrace/#attributes
Part of #5913