fetch: fence HTTP-thread FetchTasklet callbacks against worker VM dealloc - #36575
fetch: fence HTTP-thread FetchTasklet callbacks against worker VM dealloc#36575robobun wants to merge 7 commits into
Conversation
WalkthroughThe VM now uses an ChangesWorker shutdown and HTTP callback lifetime
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/fetch/FetchTasklet.rs (1)
399-418: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winHold the signal in a local before enqueueing the deinit task.
At this point
release()already returnedtrue, so the HTTP thread owns the last reference and hands ownership to the JS thread throughConcurrentTask::from_callback(this, FetchTasklet::deinit_callback). The JS thread can rundeinit_callbackas soon as the task is enqueued.deinit()callsbun_core::heap::take(this)and drops the box, including thevm_shutdown_signalfield. Line 417 then dereferencesself_.vm_shutdown_signalon freed memory.Two failures follow:
- Use-after-free of the
Arcfield (and of the wholeFetchTaskletallocation) on the HTTP thread.- If the drop wins the race,
end_vm_read()never runs against the live signal, soreadersstays above zero andwait_for_readers()inWebWorker::shutdownspins forever.Clone the
Arcinto a local while the tasklet is still alive, then enqueue, then end the read section through the local.Note the other two fenced sites are not affected:
on_write_request_data_draintakesref_()before enqueueing, andcallbackstill holds the HTTP-side ref pastend_vm_read().🐛 Proposed fix
// this is really unlikely to happen, but can happen // lets make sure that we always call deinit from main thread // `from_callback` heap-allocates a fresh `ConcurrentTaskItem`; the queue // takes ownership of it. + // The enqueued task may run and free `this` before we return, so keep + // our own handle on the signal to close the read section. + let signal = std::sync::Arc::clone(&self_.vm_shutdown_signal); + let vm = self_.javascript_vm; Self::enqueue_concurrent( - self_.javascript_vm, + vm, ConcurrentTask::from_callback(this, FetchTasklet::deinit_callback), ); - self_.vm_shutdown_signal.end_vm_read(); + signal.end_vm_read();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/webcore/fetch/FetchTasklet.rs` around lines 399 - 418, Clone self_.vm_shutdown_signal into a local Arc before calling ConcurrentTask::from_callback or enqueue_concurrent, while the FetchTasklet allocation is still alive. Enqueue the deinit task as before, then call end_vm_read() through the local clone so the signal remains valid even if deinit_callback drops the tasklet immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/runtime/webcore/fetch/FetchTasklet.rs`:
- Around line 399-418: Clone self_.vm_shutdown_signal into a local Arc before
calling ConcurrentTask::from_callback or enqueue_concurrent, while the
FetchTasklet allocation is still alive. Enqueue the deinit task as before, then
call end_vm_read() through the local clone so the signal remains valid even if
deinit_callback drops the tasklet immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ce49b9f-9d29-4047-8c34-a4b4978e9ccc
📒 Files selected for processing (4)
src/jsc/VirtualMachine.rssrc/jsc/web_worker.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/js/web/workers/worker-terminate-lifetime.test.ts
|
Addressed review so far: CodeRabbit (deref_from_thread, lines 399-418): fixed in 4477236. The enqueued comment-cop: trimmed in fb9d622; the check passes. The remaining flags are on the find-duplicate-prs-bot: this PR is the narrow fetch-only stopgap for the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The UAF I flagged in deref_from_thread is fixed in 4477236 — the signal Arc and javascript_vm are now snapshotted to locals before the enqueue, and end_vm_read() runs on the local clone. I found nothing further this round, but this is cross-thread lifetime code introducing a new synchronization primitive (Dekker-style reader fence + spin-wait + deliberate leak on the worker path) and one of six open PRs attacking the same bug class, so a maintainer should decide which approach lands.
What was reviewed:
deref_from_threadafter the 4477236 fix — locals hoisted before enqueue, no field read of*thispast the hand-off.callbackandon_write_request_data_drain—end_vm_read()readstask_ref/this_refwhile the HTTP-side ref (or the extraref_()) is still held, so the tasklet cannot be freed under it.- SeqCst ordering on
try_begin_vm_read/mark_shutting_down/wait_for_readers— the four-op Dekker fence holds;end_vm_read's release makes the enqueue visible torelease_queued_tasks_for_shutdown. dealloc_for_shutdownreadsis_main_thread()from the tasklet's own Arc field while the box is still live (ref_count == 0 but not yet freed).
Extended reasoning...
Overview
Adds an Arc<CrossThreadShutdownSignal> (atomic flag + reader count) to VirtualMachine, has each FetchTasklet clone it, and replaces the three HTTP-thread javascript_vm.is_shutting_down() reads with a try_begin_vm_read() / end_vm_read() bracket around enqueue_task_concurrent. WebWorker::shutdown marks the flag before on_exit() and spin-waits readers to zero right before release_queued_tasks_for_shutdown, so any concurrently-enqueued task is caught by the drain. dealloc_for_shutdown now leaks the tasklet on worker VMs (parking would deref freed JSC handles); large buffers are released by the caller first. An ASAN-gated subprocess test reproduces the original UAF.
Security risks
None user-facing; this is internal lifetime management. The failure mode being fixed is a process crash (DoS-ish for a server hosting workers), not data exposure or auth.
Level of scrutiny
High. This is hand-rolled cross-thread synchronization on a hot shutdown path in VirtualMachine / WebWorker / FetchTasklet — exactly the memory-safety category the review guide flags as most-blocked. It introduces a new primitive with a documented Dekker-style SeqCst fence, a busy spin-wait in worker teardown, and an intentional leak. The find-duplicate-prs bot lists five other open PRs (#30943, #31745, #32707, #34154, #35767) plus the general #32071 that this PR says will supersede it — which of these lands is a maintainer call, not something to auto-approve.
Other factors
My previous inline finding (reading self_.vm_shutdown_signal after the deinit enqueue in deref_from_thread) was addressed in 4477236. I re-checked the two other end_vm_read() sites: in callback the HTTP-side ref is held past end_vm_read() (deref runs after mutex.unlock()), and in on_write_request_data_drain an explicit Self::ref_() precedes the enqueue. The SeqCst placement on all four fence ops is correct for the stated invariant, and the end_vm_read fetch_sub's release semantics make the preceding enqueue visible to the worker thread's post-wait_for_readers drain. The comment-cop flags remaining are on the concurrency-contract doc comments, which the author has justified as invariant documentation rather than workaround narration; that's a style call for the human reviewer. Test follows the file's existing subprocess/ASAN-gated pattern and drains stdout/stderr/exited concurrently.
|
A second producer of this bug class was found in the same fuzz round: That producer is intentionally out of scope for this PR. The |
|
CI status for dfd74de (build #88110): all individual lane checks pass; only the aggregate is red. The new 4-door
Earlier build #86413 on the pre-rebase sha had the same shape (only red was the unrelated Ready for review. |
…lloc A worker's VirtualMachine is raw-dealloc'd by WebWorker::shutdown while the shared HTTP client thread can still deliver FetchTasklet callbacks for that worker's in-flight requests. Those callbacks dereferenced javascript_vm (a lifetime-erased &'static VirtualMachine) to read is_shutting_down and to enqueue_task_concurrent, which is a heap-use-after-free on the freed VM storage and crashes the whole process. Add a per-VM Arc<CrossThreadShutdownSignal> that tasklets clone at creation. The three HTTP-thread entry points (callback, deref_from_thread, on_write_request_data_drain) now read the shutdown flag from the Arc and hold a reader count across the VM dereference. WebWorker::shutdown marks the signal early and spins on the reader count before freeing the VM, so every in-flight callback either observes shutting_down and takes the early-return path or completes its enqueue before the dealloc. Tasklets orphaned by a dead worker VM are leaked rather than parked in the process-exit reclaim list, since the parked deinit() would touch JSC handles in freed storage; the large body buffers are released before the leak.
The enqueued deinit_callback may free the tasklet before this function returns, so reading self_.vm_shutdown_signal after the enqueue is a use-after-free and could leave the reader count unbalanced.
…row/reject) for the fetch UAF A worker can reach WebWorker::shutdown via four doors: parent terminate(), worker process.exit(), an uncaught throw, or an unhandled rejection. All four raw-dealloc the VirtualMachine while the shared HTTP thread may still be inside FetchTasklet::callback reading it; the fence in shutdown() is door-agnostic by construction. The test matrix runs the fetch-in-flight repro once per door so the fence is proven against each, not only terminate().
4477236 to
5fbe35b
Compare
|
Updated 4:51 AM PT - Aug 3rd, 2026
❌ @robobun, your commit dfd74de has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36575That installs a local version of the PR into your bun-36575 --bun |
|
Rebased onto main (074656d) and extended the test to cover all four worker-shutdown doors: parent ASAN on main without the fix: 4/4 doors fail with |
Matches the file's existing ready(w) pattern so a worker that fails before posting 'up' rejects with a useful message instead of hanging to the outer timeout. The error-swallowing handler for the throw/reject doors moves to after the ready-wait.
There was a problem hiding this comment.
No new issues found; both earlier findings (the deref_from_thread post-enqueue field read and the test's un-wired ready-wait) are addressed. This is cross-thread lifetime/memory-safety code with a new hand-rolled synchronization primitive, so it should get human eyes.
What was reviewed:
try_begin_vm_read/end_vm_readDekker ordering (inc-then-load vs store-then-wait, all SeqCst) and that the reader window in each of the three HTTP-thread sites is bounded sowait_for_readers()can't spin indefinitely.- All three
end_vm_read()call sites for tasklet-still-alive:deref_from_threadnow uses stack-clonedsignal;callbackholds the HTTP-side ref past it;on_write_request_data_draintakesSelf::ref_()before the enqueue. mark_shutting_down()placement (beforeon_exit()) vswait_for_readers()placement (immediately beforerelease_queued_tasks_for_shutdown), so any task a reader enqueued in between is drained.dealloc_for_shutdownmain-thread vs worker branch — main-thread path unchanged, worker path leaks the box after buffers were freed by the caller.
Extended reasoning...
Overview
The PR adds a per-VM Arc<CrossThreadShutdownSignal> (atomic flag + reader count) so the process-shared HTTP client thread can safely check whether a worker's VirtualMachine is still alive before dereferencing it. FetchTasklet clones the Arc at creation and brackets each of its three HTTP-thread VM dereferences (callback, deref_from_thread, on_write_request_data_drain) with try_begin_vm_read()/end_vm_read(). WebWorker::shutdown marks the flag before running exit handlers and spin-waits for readers to hit zero right before draining the concurrent task queue and raw-deallocing the VM. dealloc_for_shutdown now leaks the tasklet box for worker VMs (the parked deinit() would touch freed JSC handles) while keeping the existing defer_shutdown_reclaim path for the main thread. An ASAN-gated test exercises all four worker-shutdown doors.
Security risks
None identified. This is a crash/UAF fix in internal lifetime management; no user-facing input parsing, auth, or trust boundaries are touched.
Level of scrutiny
High. This is exactly the category REVIEW.md flags as most-blocked: cross-thread lifetime, hand-rolled atomics with a stated memory-ordering argument, a deliberate leak, and a spin-wait with no timeout on the worker-shutdown path. The try_begin_vm_read fence is a Dekker-style increment-then-check against store-then-wait; getting the ordering or the placement of wait_for_readers() relative to release_queued_tasks_for_shutdown wrong reintroduces the UAF or leaks enqueued tasks. The PR is also explicitly a stopgap that will be superseded by #34154/#35767, which is a design call a maintainer should ratify.
Other factors
- Two prior review findings on this PR were both addressed (the snapshot-before-enqueue in
deref_from_thread, and the test'sready(w)helper). - CI on the earlier revision was green on the ASAN lane for the new test; the one red was a pre-existing unrelated failure.
- The reader window at each HTTP-thread site is short and non-blocking (an MPSC push + wakeup), so
wait_for_readers()should not spin long — but it has no bound, which is worth a maintainer's judgement. - The double
mark_shutting_down()(once inWebWorker::shutdown, once viaon_exit()) is idempotent; main-thread behavior incallback's shutdown branch is preserved becausedealloc_for_shutdownstill routes throughdefer_shutdown_reclaimwhenis_main_thread.
Deferring to human review.
|
Superseded by #36983: the worker shutdown fence there spans the whole HTTP engagement of a FetchTasklet (queue() to the final is_done callback) via EventLoop.outstanding_offthread, adds an abort fan-out so terminate() does not wait out the transfer, and includes this PR's worker-path dealloc_for_shutdown leak fix and the four-door fetch test. Closing in favor of that PR. |
2 participants
Crash
A worker reaching
WebWorker::shutdownwithfetch()requests in flight crashes the whole process. Stock canary SIGSEGVs; ASAN builds reportheap-use-after-free8/8:All four shutdown doors trigger it: the parent calling
worker.terminate(), and the worker ending itself viaprocess.exit(), an uncaught throw, or an unhandled rejection.process.exit()in a worker isWebWorker::exit()=set_requested_terminate(), and both the entry-promise-rejected and uncaught-exception paths inspin()fall through to the sameshutdown(); the dealloc atweb_worker.rs:1383is the same in every case.Repro
Worker opens 10 lanes of back-to-back fetches against a local trickle-stream server and exits mid-stream (any door), repeat:
Cause
FetchTasklet.javascript_vmis a lifetime-erased&'static VirtualMachine. The HTTP client thread is process-shared and reads that reference incallback,deref_from_thread, andon_write_request_data_drainto checkis_shutting_down()and toenqueue_task_concurrent(). A worker'sVirtualMachine(with the event loop and concurrent-task queue embedded in it) is raw-dealloc'd byWebWorker::shutdown(), with no fence against those HTTP-thread readers. The main-threadglobal_exit()already has exactly that fence:bun_http::shutdown_for_exit()parks the HTTP daemon beforerelease_queued_tasks_for_shutdownand the VM free. Worker shutdown has no counterpart, andshutdown_for_exit()is a process-global one-shot so it cannot be reused per-worker.Fix
Give each VM an
Arc<CrossThreadShutdownSignal>(AtomicBoolflag + reader count) and have everyFetchTaskletclone it at creation. The three HTTP-thread entry points replacejavascript_vm.is_shutting_down()withtry_begin_vm_read(): the flag is read from the Arc (safe after dealloc), and ontruea reader count is held across theenqueue_task_concurrentso worker shutdown spins it out before freeing the VM. Worker shutdown marks the flag beforeon_exit()and waits for readers to drain right beforerelease_queued_tasks_for_shutdown, so any task a reader enqueued is caught by the drain. The fence sits inWebWorker::shutdownitself, so it covers every door.A tasklet whose last ref drops on the HTTP thread after a worker VM has shut down is leaked rather than parked in the process-exit
defer_shutdown_reclaimlist (the parkeddeinit()would dereference JSC handles in freed storage). The large body buffers are released before the leak; main-thread shutdown still uses the existing park-and-drain path.Relationship to #34154 / #35767
#34154 (and #35767) are the general fixes covering every cross-thread producer (fetch, S3, work pool, napi, fs watchers, the waiter thread). Both are currently conflicting. This PR is a focused stopgap for the fetch slice so the process-killing crash stops landing; either supersedes it when it lands.
Verification
test/js/web/workers/worker-terminate-lifetime.test.tsgains an ASAN-gateddescribethat runs the repro once per shutdown door (terminate(),process.exit(), uncaught throw, unhandled rejection) in a subprocess and asserts clean stderr.test/js/web/workers/worker.test.ts: 25 pass / 0 fail.test/js/node/worker_threads/worker_threads.test.ts: 91 pass / 0 fail.cargo clippy -p bun_jsc -p bun_runtime: clean.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/workers/worker-terminate-lifetime.test.ts