Fix race condition in assigning the pthread_t object in main thread - #25209
Conversation
| // Assign the pthread_t object over immediately, so that by the time pthread_create_js() | ||
| // is dispatched to a pthread and the pthread main runs, the value will be visible to | ||
| // the thread to examine. | ||
| __atomic_store_n(res, new, __ATOMIC_SEQ_CST); |
There was a problem hiding this comment.
In the musl codebase you can/should use the a_store macro instead I think.
There was a problem hiding this comment.
This is not musl codebase file? This is our own, no?
There was a problem hiding this comment.
This file uses the musl atomics convetions, and its at-least-in-part based on the musl version of the same code. See all the a_store, a_cas and __wait calls that exist already.
There was a problem hiding this comment.
a_store only assigns an integer, so it would splice the pointer in a 64-bit build.
Would you like
#ifdef __wasm64__
a_store_l((long*)res, (long)new);
#else
a_store((int*)res, (int)new);
#endifinstead?
There was a problem hiding this comment.
This makes me wonder how musl deals with this issue.
The core here is heavily based on ./system/lib/libc/musl/src/thread/pthread_create.c including the final *res = new; assignment.
There was a problem hiding this comment.
LGTM then with an extra comment about why we are not using a_store here.
I'm still curious why the pointer needs to be assigned though, isn't it enough for the struct pthread to be populated correctly? Where does the target thread depend on *res* being set?
There was a problem hiding this comment.
(Also, would the standard C11 atomic_store work here? https://en.cppreference.com/w/c/atomic/atomic_store)
…when creating a pthread. Fixes emscripten-core#25026.
d9e3154 to
b051e2c
Compare
|
|
||
| // Assign the pthread_t object over immediately, so that by the time pthread_create_js() | ||
| // is dispatched to a pthread and the pthread main runs, the value will be visible to | ||
| // the thread to examine. |
There was a problem hiding this comment.
Can we move this to line 250?
…mscripten-core#25209) Fix race condition in assigning the pthread_t object in main thread, when creating a pthread. Fixes emscripten-core#25026. Also reduce the stress test case for `test_stress_pthread_proxying` so that it runs faster and reproduces more often.
Fix race condition in assigning the pthread_t object in main thread, when creating a pthread. Fixes #25026.
Also reduce the stress test case for
test_stress_pthread_proxyingso that it runs faster and reproduces more often.