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 race condition when starting bgw thread #785

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

cnicolaescu
Copy link
Contributor

Specifically on Windows, the sentry_thread_spawn macro is defined as follows:

#    define sentry__thread_spawn(ThreadId, Func, Data)                         \
        (*ThreadId = CreateThread(NULL, 0, Func, Data, 0, NULL),               \
            *ThreadId == INVALID_HANDLE_VALUE ? 1 : 0)

We can see that ThreadId is initialized with the value returned by CreateThread, but the new thread might be running before CreateThread returns. If the new thread tries to access bgw->thread_id, the value might not be initialized yet, leading to undefined behaviour / crash.

In this change a new function sentry__thread_get_current_threadid is added, which returns the current thread id in a reliable way.

- specifically on Windows: the `sentry_thread_spawn` macro is defined as follows:

```
#    define sentry__thread_spawn(ThreadId, Func, Data)                         \
        (*ThreadId = CreateThread(NULL, 0, Func, Data, 0, NULL),               \
            *ThreadId == INVALID_HANDLE_VALUE ? 1 : 0)
```

We can see that `ThreadId` is initialized with the value returned by
`CreateThread`, but the new thread might be running before `CreateThread`
returns.

In this change a new function `sentry__thread_get_current_threadid` is
added, which returns the current thread id in a reliable way.
@cnicolaescu
Copy link
Contributor Author

Alternatively, the new thread could be created as suspended (passing the CREATE_SUSPENDED flag to CreateThread), then resumed in a 2nd step (using the ResumeThread API). This allows to correctly initialize bgw->thread_id before the new thread starts running.
This approach involves either:

  • enhanching the sentry__thread_spawn macro
  • creating the suspended thread in sentry__thread_init, then resuming it in sentry__thread_spawn

Please let me know if you'd prefer one of these 2 alternative approaches.

@Swatinem
Copy link
Member

lgtm. If I remember correctly setting the thread name is indeed the only access of the thread_id from within the thread itself. We also only create a single transport thread, and creation should be guarded in the sentry_init lock.

@codecov-commenter
Copy link

Codecov Report

Merging #785 (730f8fd) into master (ed0a62d) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #785      +/-   ##
==========================================
- Coverage   81.33%   81.27%   -0.07%     
==========================================
  Files          53       53              
  Lines        6929     6933       +4     
  Branches     1110     1110              
==========================================
- Hits         5636     5635       -1     
- Misses       1183     1188       +5     
  Partials      110      110              

@supervacuus
Copy link
Collaborator

@cnicolaescu, good catch; thx for the fix. Can you add an entry to the change log to merge this?

@supervacuus supervacuus merged commit 57701f8 into getsentry:master Jan 9, 2023
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.

4 participants