Skip to content

Commit ef94d08

Browse files
authored
[WASM_WORKERS] Fix UB in pthread_mutex with debug+hybrid builds (#26673)
This fix only applies to the hybrid Wasm Workers + pthread build mode. We can't currently rely on calling `pthread_self()` from a Wasm Worker, even the hybrid mode. We are working on fixing that in #26631. However, we do have a `CURRENT_THREAD_ID` macro that works on both Wasm Workers and pthreads. As it happens, pthread locks under musl do not normally depend on `pthread_self` except for "recursive" which need to access the `pthread_self()->tid` field to track the current owner. This means that I broke the debug + hybrid mode locks in #24607 because I added the recursion check to all locks in debug builds. While I have added a repro case for this its not directly testing the issue, it just heppens to crash due to the current undefined behavior when doing `pthread_self()->tid` from a Wasm Worker. Fixes: #26619
1 parent 7e2483c commit ef94d08

File tree

7 files changed

+47
-8
lines changed

7 files changed

+47
-8
lines changed

system/lib/libc/musl/src/thread/pthread_mutex_consistent.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ int pthread_mutex_consistent(pthread_mutex_t *m)
77
int own = old & 0x3fffffff;
88
if (!(m->_m_type & 4) || !own || !(old & 0x40000000))
99
return EINVAL;
10-
if (own != __pthread_self()->tid)
10+
if (own != CURRENT_THREAD_ID)
1111
return EPERM;
1212
a_and(&m->_m_lock, ~0x40000000);
1313
return 0;

system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,12 +87,12 @@ int __pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec
8787
if (!own && (!r || (type&4)))
8888
continue;
8989
if ((type&3) == PTHREAD_MUTEX_ERRORCHECK
90-
&& own == __pthread_self()->tid)
90+
&& own == CURRENT_THREAD_ID)
9191
return EDEADLK;
9292
#if defined(__EMSCRIPTEN__) && !defined(NDEBUG)
9393
// Extra check for deadlock in debug builds, but only if we would block
9494
// forever (at == NULL).
95-
assert(at || own != __pthread_self()->tid && "pthread mutex deadlock detected");
95+
assert(at || own != CURRENT_THREAD_ID && "pthread mutex deadlock detected");
9696
#endif
9797

9898
a_inc(&m->_m_waiters);

system/lib/libc/musl/src/thread/pthread_mutex_trylock.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ int __pthread_mutex_trylock_owner(pthread_mutex_t *m)
55
int old, own;
66
int type = m->_m_type;
77
pthread_t self = __pthread_self();
8-
int tid = self->tid;
8+
int tid = CURRENT_THREAD_ID;
99
volatile void *next;
1010

1111
old = m->_m_lock;

test/codesize/test_codesize_minimal_pthreads.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
{
22
"a.out.js": 7367,
33
"a.out.js.gz": 3603,
4-
"a.out.nodebug.wasm": 19003,
4+
"a.out.nodebug.wasm": 19005,
55
"a.out.nodebug.wasm.gz": 8786,
6-
"total": 26370,
6+
"total": 26372,
77
"total_gz": 12389,
88
"sent": [
99
"a (memory)",

test/codesize/test_codesize_minimal_pthreads_memgrowth.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
{
22
"a.out.js": 7769,
33
"a.out.js.gz": 3809,
4-
"a.out.nodebug.wasm": 19004,
4+
"a.out.nodebug.wasm": 19006,
55
"a.out.nodebug.wasm.gz": 8787,
6-
"total": 26773,
6+
"total": 26775,
77
"total_gz": 12596,
88
"sent": [
99
"a (memory)",

test/test_stress.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,3 +113,6 @@ def test_stress_pthread_proxying(self):
113113

114114
js_file = self.build('pthread/test_pthread_proxying_reduced_stress_test_case.c')
115115
self.parallel_stress_test_js_file(js_file, not_expected='pthread_self() == unknown', expected='pthread_self() == worker2', assert_returncode=0)
116+
117+
def test_stress_pthread_malloc(self):
118+
self.do_runf('test_stress_pthread_malloc.c', 'done\n', cflags=['-pthread', '-sWASM_WORKERS'])

test/test_stress_pthread_malloc.c

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
#include <emscripten.h>
2+
#include <emscripten/wasm_worker.h>
3+
#include <emscripten/console.h>
4+
#include <stdlib.h>
5+
#include <stdio.h>
6+
7+
_Atomic int running_threads = 0;
8+
9+
static void malloc_loop() {
10+
// Busy loop here until both threads are up running
11+
running_threads += 1;
12+
while (running_threads != 2) {}
13+
14+
for (int i = 0; i < 1000000; ++i) {
15+
free(malloc(1));
16+
}
17+
}
18+
19+
static void worker_callback(void) {
20+
emscripten_outf("worker_callback");
21+
malloc_loop();
22+
}
23+
24+
static void main_callback(void* arg) {
25+
emscripten_outf("main_callback");
26+
malloc_loop();
27+
emscripten_outf("done");
28+
emscripten_terminate_all_wasm_workers();
29+
}
30+
31+
int main() {
32+
emscripten_outf("main");
33+
emscripten_wasm_worker_post_function_v(emscripten_malloc_wasm_worker(1024 * 1024), worker_callback);
34+
emscripten_async_call(main_callback, NULL, 0);
35+
return 0;
36+
}

0 commit comments

Comments
 (0)