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

Crashpad crash collection is not thread-safe #931

Open
1 of 3 tasks
stima opened this issue Jan 3, 2024 · 2 comments
Open
1 of 3 tasks

Crashpad crash collection is not thread-safe #931

stima opened this issue Jan 3, 2024 · 2 comments

Comments

@stima
Copy link
Contributor

stima commented Jan 3, 2024

Description

During sentry__crashpad_handler crash_event is not synchronized properly.

When does the problem happen

  • During build
  • During run-time
  • When capturing a hard crash

Environment

  • OS: Unrelated
  • Compiler: Unrelated
  • CMake version and config: Unrelated

Steps To Reproduce

For example sentry_set_tag that executed from one thread and sentry__crashpad_handler that capturing crash in another thread, may lead to a race condition while accessing crash_event variable.

sentry_set_tag call would lead to sentry__scope_flush_unlock call, that would lead to options->backend->flush_scope_func that would lead to crashpad_backend_flush_scope and access to a crash_event that may be freed by that line https://github.com/getsentry/sentry-native/blob/164da7919172b0df9c7b75efbc36e6e897124415/src/backends/sentry_backend_crashpad.cpp#L174C4-L174C4.

@supervacuus supervacuus self-assigned this Jan 8, 2024
@supervacuus supervacuus added bug Something isn't working and removed Waiting for: Product Owner labels Jan 8, 2024
@supervacuus
Copy link
Collaborator

Thanks for raising this, @stima. I was aware of this but forgot to tackle it. The UAF is an issue but could be easily fixed with an incremented ref-count. The more significant issue is that we could mutate the crash event between two threads and potentially in the signal handler (i.e., the synchronization must be async-safe).

@stima
Copy link
Contributor Author

stima commented Jan 8, 2024

@supervacuus, you are welcome, I would agree that second part is more conseptual and actually more general to be it done properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for: Product Owner
Status: Backlog
Development

No branches or pull requests

3 participants