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
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.
There was a problem hiding this comment.
Yeah, I guess emscripten_proxy_is_canceled would be prone to TOCTOU races. We need something that can atomically check if the caller is canceled and if not, prevent it from being canceled until released.
There was a problem hiding this comment.
Ok I've pushed up an experimental exploration of this with a emscripten_proxy_release_arg(ctx) and emscripten_proxy_acquire_arg(ctx) and atomic per-call context state.
|
Can we use __pthread_setcancelstate to suspend cancelation while the caller is waiting on proxied work? @guybedford what kind of workloads are you running that depend on thread cancellation? i.e. is this a fix for a real issue you are hitting? |
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.
30a3fc4 to
ecc3a7e
Compare
Sync proxying ctxs are now allocated from a per-thread free list and coordinate through a single atomic lifecycle word that the caller futex-waits on directly, removing the per-call heap allocation and mutex/cond operations. emscripten_proxy_release_arg and emscripten_proxy_acquire_arg are exposed as public API implemented as transitions on that word, and a caller canceled before its task starts now exits immediately with the task dropped.
ecc3a7e to
fddfea0
Compare
Collapse the five-phase lifecycle into three ownership flags (LOANED, DONE+OK, ORPHANED): task start and emscripten_proxy_acquire_arg become the same loan-taking transition, unifying the dropped-task and orphaned paths. Also removes the pthread cond/mutex machinery from minimal pthread builds entirely, coming out ~1KB smaller than main.
Unfortunately not, if there is genuinely stalled work, the wait could be delayed indefinitely. This allows the worker to be fully released even in that case.
This is not actually a problem I have personally, I just posted this as a quick AI-generated fix when I became aware of the gap, which then turned out to be harder than I expected! I think the fix now works in its current state using atomics to properly check the state, whether it's the absolute best approach is another question. |
Is this really an issue though. Do you think you will get folks complaining "I can't cancel a thread that is blocked forever waiting on another thread"? Maybe its reasonable to wait for the current operation to complete and then cancel? Pthread cancellation is very rare/dangerous/obscure thing, and I'm not sure we need to jump through too many hoops to make these edge cases work perfectly. So I'd be tempted to wait until you have a real user saying "Hey why can't I cancel this pthread" and in the mean time just delay cancelation. Maybe we can use the existing PTHREAD_CANCEL_MASKED thing that pthread_cond_timedwait uses? |
Yeah I would suggest leaving this PR open / stalled, until the matter comes up with stronger motivation. |
Mask cancellation around the wait as pthread_cond_timedwait does, so a cancel surfaces as ECANCELED and the ctx handover runs as straight-line code before exiting, rather than unwinding out of the wait through a cleanup handler.
I was able to refactor the cancellation delivery to use this mechanism. Note that the atomic ctx state and arg handover is still required though. |
3 participants
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