Skip to content

Commit f3fef05

Browse files
authored
Signal before unlocking proxy context mutex in emscripten_proxy_finish (#26582)
When finishing a proxied call, the following race condition can happen: thread 1 in [emscripten_proxy_finish](https://github.com/emscripten-core/emscripten/blob/main/system/lib/pthread/proxying.c#L337): ``` pthread_mutex_lock(&ctx->sync.mutex); ctx->sync.state = DONE; remove_active_ctx(ctx); pthread_mutex_unlock(&ctx->sync.mutex); --> thread is preempted or suspends here <--- pthread_cond_signal(&ctx->sync.cond); ``` thread 2 in emscripten_proxy_sync_with_ctx: (ctx is on this thread's stack) ``` pthread_mutex_lock(&ctx.sync.mutex); <-- locks after unlock above while (ctx.sync.state == PENDING) { <--- reads sync.state == DONE pthread_cond_wait(&ctx.sync.cond, &ctx.sync.mutex); <-- doesn't run } pthread_mutex_unlock(&ctx.sync.mutex); int ret = ctx.sync.state == DONE; em_proxying_ctx_deinit(&ctx); <--- frees ctx and returns ``` Then thread 1 tries to run pthread_cond_signal on the freed ctx. This same logic applies to cancel_ctx which is also called on the target thread.
1 parent 9d9b267 commit f3fef05

File tree

3 files changed

+10
-6
lines changed

3 files changed

+10
-6
lines changed

system/lib/pthread/proxying.c

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -339,8 +339,11 @@ void emscripten_proxy_finish(em_proxying_ctx* ctx) {
339339
pthread_mutex_lock(&ctx->sync.mutex);
340340
ctx->sync.state = DONE;
341341
remove_active_ctx(ctx);
342-
pthread_mutex_unlock(&ctx->sync.mutex);
342+
// Signal must come before unlock to avoid emscripten_proxy_sync_with ctx
343+
// seeing the state as DONE and freeing the ctx before we call unlock.
344+
// See https://github.com/emscripten-core/emscripten/pull/26582
343345
pthread_cond_signal(&ctx->sync.cond);
346+
pthread_mutex_unlock(&ctx->sync.mutex);
344347
} else {
345348
// Schedule the callback on the caller thread. If the caller thread has
346349
// already died or dies before the callback is executed, then at least make
@@ -365,8 +368,9 @@ static void cancel_ctx(void* arg) {
365368
if (ctx->kind == SYNC) {
366369
pthread_mutex_lock(&ctx->sync.mutex);
367370
ctx->sync.state = CANCELED;
368-
pthread_mutex_unlock(&ctx->sync.mutex);
371+
// Signal must be first, see comment in emscripten_proxy_finish.
369372
pthread_cond_signal(&ctx->sync.cond);
373+
pthread_mutex_unlock(&ctx->sync.mutex);
370374
} else {
371375
if (ctx->cb.cancel == NULL ||
372376
!do_proxy(ctx->cb.queue,

test/codesize/test_codesize_minimal_pthreads.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@
22
"a.out.js": 7363,
33
"a.out.js.gz": 3604,
44
"a.out.nodebug.wasm": 19046,
5-
"a.out.nodebug.wasm.gz": 8822,
5+
"a.out.nodebug.wasm.gz": 8821,
66
"total": 26409,
7-
"total_gz": 12426,
7+
"total_gz": 12425,
88
"sent": [
99
"a (memory)",
1010
"b (exit)",

test/codesize/test_codesize_minimal_pthreads_memgrowth.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@
22
"a.out.js": 7765,
33
"a.out.js.gz": 3810,
44
"a.out.nodebug.wasm": 19047,
5-
"a.out.nodebug.wasm.gz": 8823,
5+
"a.out.nodebug.wasm.gz": 8822,
66
"total": 26812,
7-
"total_gz": 12633,
7+
"total_gz": 12632,
88
"sent": [
99
"a (memory)",
1010
"b (exit)",

0 commit comments

Comments
 (0)