Make sync proxied waits safe cancellation points - #27613
Conversation
49eb53e to
5f4bcc6
Compare
|
If you rebase it should fix the browser test failures. |
5f4bcc6 to
b877aa5
Compare
b877aa5 to
4875696
Compare
A thread blocked in emscripten_proxy_sync[_with_ctx] already woke on pthread_cancel (the cond wait's futex honors it), but unwound with the em_proxying_ctx and the caller-owned argument still on its stack while the target thread kept referencing both, and for PROXY_SYNC_ASYNC JS imports a later promise resolution wrote the result into the freed stack. Any blocking proxied wait could hit this, e.g. poll()/epoll_wait() with an infinite timeout. The sync ctx is now heap allocated and refcounted between caller and target. A cancellation cleanup handler on the caller waits until the target has released the argument (immediately after a JS import has been dispatched; on completion for generic callers), marks the ctx orphaned and drops its reference, so a late completion on the target is harmless. The PROXY_SYNC JS path routes through the same guarded result write.
bf2dfbc to
31a5fd9
Compare
| pthread_mutex_lock(&ctx->sync.mutex); | ||
| ctx->sync.arg_released = true; | ||
| pthread_cond_signal(&ctx->sync.cond); | ||
| pthread_mutex_unlock(&ctx->sync.mutex); |
There was a problem hiding this comment.
I don't love that we're accessing the ctx here. Ideally run_js_func_with_ctx would be no more privileged than any other function passed to emscripten_proxy_sync_with_ctx and would treat the ctx as opaque.
I think we need to expose public emscripten_proxy_release_arg(ctx) and emscripten_proxy_is_canceled(ctx) (or similar) functions that this function or any other async or long-running user-defined work functions can call.
There was a problem hiding this comment.
Yeah this one's tough - I don't really like the design either. But I think the suggestion might be racy (I just tried to implement it and an alternative as well).
Will put some more thought to if there's a better way here, and follow-up next week.
This fixes a use-after-free when a thread blocked in
emscripten_proxy_sync[_with_ctx]is canceled, and with it makes those waits proper POSIX cancellation points.The wait already woke on
pthread_cancel(the cond wait's futex honors it), but the canceled thread then unwound with theem_proxying_ctxand its caller-owned argument still on its stack while the target thread kept referencing both; forPROXY_SYNC_ASYNCJS imports a later promise resolution wrote the result into the freed stack. Any blocking proxied wait could hit this, e.g.poll()/epoll_wait()with an infinite timeout.pthread_cleanup_pushhandler on the caller waits until the target has released the argument - immediately after a JS import has been dispatched, or on completion for generic callers - then marks the ctx orphaned and drops its reference. The thread exits withPTHREAD_CANCELED; a lateemscripten_proxy_finishon the target is harmless.PROXY_SYNCJS path now goes through the same guarded result write asPROXY_SYNC_ASYNC.maybeExitpassed an unsetEXITSTATUSto_emscripten_thread_exitfor a pthread that unwound to the event loop rather than returning a result. Harmless on wasm32, butBigInt(undefined)throws underMEMORY64, which the new test hits since its target thread parks in the event loop.Tests:
test_pthread_proxying_canceled_caller(cancel while the target still runs the work, and while it holds an unfinished ctx) andtest_epoll_cancel(threads canceled inepoll_wait(-1)andpoll(-1), then the orphaned listeners fired by a real datagram). Both fail on main.test_codesize_minimal_pthreadsgrows by 94 bytes (cleanup push/pop andpthread_cond_timedwaitnow linked).Made with AI assistance under my review