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 accidentally mutating CONTEXT when stack walking #77

Merged
merged 1 commit into from
Jan 17, 2023

Conversation

Swatinem
Copy link
Member

We are currently investigating a customer with wrong client-side stack traces (possibly a mismatch of thread context vs exception context?)

While investigating, we discovered that we were mutating the thread context unintentionally, and the resulting minidump had broken threads contexts which were all pointing to the very bottom RtlUserThreadStart frame.

@supervacuus
Copy link
Collaborator

LGTM, I wasn't aware that StackWalk64 mutates the CONTEXT with every step. While the docs are very clear that what we did before was not correct:

This context may be modified, so do not pass a context record that should not be modified.

It was unclear that it uses the CONTEXT as a stack-walk state. In any case, I also tested this, and yes, this creates a copy on the stack as expected, leaving the heap data untouched as we walk.

@Swatinem Swatinem merged commit 918fd31 into getsentry Jan 17, 2023
@Swatinem Swatinem deleted the fix/mut-ctx branch January 17, 2023 13:53
supervacuus added a commit to getsentry/sentry-native that referenced this pull request Feb 7, 2023
- Switch Crashpad transport on Linux to use libcurl ([#803](#803), [crashpad#75](getsentry/crashpad#75), [crashpad#79](getsentry/crashpad#79))
- Avoid accidentally mutating CONTEXT when client-side stack walking in Crashpad ([#803](#803), [crashpad#77](getsentry/crashpad#77))
- Fix various mingw compilation issues ([#794](#794), [crashpad#78](getsentry/crashpad#78))
- Updated Crashpad backend to 2023-02-07. ([#803](#803), [crashpad#80](getsentry/crashpad#80))
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.

2 participants