starboard: Check pthread_join completes successfully #7110
Conversation
pthread_join is one of fundamental invariants in the code, ensuring that a thread is no longer running after it returns. Especially, no more callbacks are called from this point and after. A failure in pthread_join breaches this invariant and leads to an state that code doesn't expect (e.g. the supposedly-completed thread is still calling the callbacks)
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly identifies and fixes a critical issue where the return value of pthread_join was not being checked. This could lead to serious problems like use-after-free errors and race conditions if a thread failed to join properly. The change to use SB_CHECK_EQ ensures that any failure in pthread_join will be caught, preventing the application from continuing in an unstable state. The changes are applied consistently across multiple files, and the minor update from NULL to nullptr is a good C++ modernization. Overall, this is an excellent and important improvement for the stability of the code.
kjyoun
changed the title
The code relies on a fundamental invariant: after pthread_join() returns, the joined thread is guaranteed to have terminated. This means no more callbacks will be made from that thread. A failure in `pthread_join()` breaches this invariant and leads to an unexpected state where the supposedly-completed thread is still running and calling callbacks. This can lead to use-after-free errors or other race conditions. This change adds a check to enforce this invariant, preventing the application from continuing in a corrupt state. Bug: 443334872 (cherry picked from commit fb82fe8)
|
|
||
| if (audio_out_thread_) { | ||
| pthread_join(*audio_out_thread_, nullptr); | ||
| SB_CHECK_EQ(pthread_join(*audio_out_thread_, nullptr), 0); |
There was a problem hiding this comment.
The code is correct and is what we should have implemented in the first place.
Just wanted to bring up that, if under certain situations the function returns a value other than 0, the app may continue to run with the old code, but will crash in production with the new code.
There was a problem hiding this comment.
Crashing the app with a CHECK when pthread_join() fails is the intended behavior because it provides an immediate and clear stack trace.
The audio pipeline's destruction sequence relies on the guarantee that pthread_join() succeeds. Its success ensures the audio thread has terminated and will no longer issue callbacks. If this invariant is broken, a callback could be invoked during the destruction process, causing the app to crash in a corrupt state (see b/443334872#comment3).
Crashing immediately with a CHECK pinpoints the exact cause, preventing a more confusing crash later due to a race condition. This gives us a much cleaner picture of the problem at the earliest possible point.
…fully (#7120) Refer to the original PR: #7110 The code relies on a fundamental invariant: after pthread_join() returns, the joined thread is guaranteed to have terminated. This means no more callbacks will be made from that thread. A failure in `pthread_join()` breaches this invariant and leads to an unexpected state where the supposedly-completed thread is still running and calling callbacks. This can lead to use-after-free errors or other race conditions. This change adds a check to enforce this invariant, preventing the application from continuing in a corrupt state. #vibe-coded Bug: 443334872 --------- Co-authored-by: Kyujung Youn <kjyoun@google.com>
The code relies on a fundamental invariant: after pthread_join() returns, the joined thread is guaranteed to have terminated. This means no more callbacks will be made from that thread. A failure in `pthread_join()` breaches this invariant and leads to an unexpected state where the supposedly-completed thread is still running and calling callbacks. This can lead to use-after-free errors or other race conditions. This change adds a check to enforce this invariant, preventing the application from continuing in a corrupt state. #vibe-coded Bug: 443334872 original-hexsha: fb82fe8
The code relies on a fundamental invariant: after pthread_join() returns, the joined thread is guaranteed to have terminated. This means no more callbacks will be made from that thread. A failure in `pthread_join()` breaches this invariant and leads to an unexpected state where the supposedly-completed thread is still running and calling callbacks. This can lead to use-after-free errors or other race conditions. This change adds a check to enforce this invariant, preventing the application from continuing in a corrupt state. #vibe-coded Bug: 443334872
The code relies on a fundamental invariant: after pthread_join() returns, the joined thread is guaranteed to have terminated. This means no more callbacks will be made from that thread. A failure in `pthread_join()` breaches this invariant and leads to an unexpected state where the supposedly-completed thread is still running and calling callbacks. This can lead to use-after-free errors or other race conditions. This change adds a check to enforce this invariant, preventing the application from continuing in a corrupt state. #vibe-coded Bug: 443334872 original-hexsha: fb82fe8
3 participants
The code relies on a fundamental invariant: after pthread_join() returns, the joined thread is guaranteed to have terminated. This means no more callbacks will be made from that thread.
A failure in
pthread_join()breaches this invariant and leads to an unexpected state where the supposedly-completed thread is still running and calling callbacks. This can lead to use-after-free errors or other race conditions. This change adds a check to enforce this invariant, preventing the application from continuing in a corrupt state.#vibe-coded
Bug: 443334872