Fix use-after-free when a worker is terminated with a fetch in flight - #31692
Fix use-after-free when a worker is terminated with a fetch in flight#31692robobun wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a refcounted ConcurrentEnqueueGate and wires it into VirtualMachine, WebWorker shutdown, and FetchTasklet to serialize cross-thread VM access during teardown; includes docs updates and a regression test exercising worker termination during streaming fetch. ChangesWorker VM Teardown Gating for Cross-Thread Safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the flagged possible duplicates:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if !gate.enter() { | ||
| ConcurrentEnqueueGate::deref(gate_ptr); | ||
| // SAFETY: last ref; exclusive access. See the shutdown comment | ||
| // below — same reclaim path. | ||
| unsafe { FetchTasklet::dealloc_for_shutdown(this) }; | ||
| return; |
There was a problem hiding this comment.
🔴 The new gate-closed branch routes every dead-worker tasklet's last-ref drop to dealloc_for_shutdown, which parks the box in the process-global SHUTDOWN_RECLAIMS list — but that list is drained only by the main VM's global_exit() (and only under BUN_DESTRUCT_VM_ON_EXIT). So a long-running parent that repeatedly terminates fetch-using workers leaks every parked FetchTasklet (buffers, Box<AsyncHTTP>, headers, gate ref) until process exit, and when the drain does run, deinit → clear_data() tears down JSC Strong/Weak handles registered in a worker JSC VM that was freed long ago by WebWorker__teardownJSCVM. The gate-closed path needs its own reclaim that frees Rust-side allocations directly without parking and without touching the dead JSC handles.
Extended reasoning...
What the bug is
The PR's new gate-closed branches in deref_from_thread (FetchTasklet.rs:431-436) and in callback's vm_gone && is_done path (FetchTasklet.rs:2396-2404 → deref_from_thread ×2) deterministically reach dealloc_for_shutdown(this) on the 1→0 ref transition. dealloc_for_shutdown (FetchTasklet.rs:582-587) calls http::defer_shutdown_reclaim(this, deinit_erased), which pushes onto the process-global static SHUTDOWN_RECLAIMS Vec (HTTPThread.rs:1399, 1406-1410). That Vec is drained only inside shutdown_for_exit() (HTTPThread.rs:1464-1468), which is called only from VirtualMachine::global_exit() on the main thread (VirtualMachine.rs:1627), and only when should_destruct_main_thread_on_exit() is true (BUN_DESTRUCT_VM_ON_EXIT). The PR description states "in-flight requests of dead workers finish on the HTTP thread and reclaim through the existing dealloc_for_shutdown path" as if that were a freeing path; it is actually a defer-to-process-exit path designed for main-VM shutdown.
Step-by-step proof
- Parent process creates a Worker that calls
fetch(); the body starts streaming.FetchTasklet::get()runs on the worker's JS thread, taking a gate ref and settingjavascript_vm = &worker_vm. ref_count = 2 (JS-side + HTTP-side). - Parent calls
worker.terminate().WebWorker::shutdownruns on the worker thread:gate.close()→release_queued_tasks_for_shutdown()→ step 3WebWorker__teardownJSCVM(global)destroys the worker's JSC VM (its HandleSet/WeakSet are freed) → step 5std::alloc::dealloc(vm_ptr, ...)frees theVirtualMachineallocation. - The HTTP thread keeps delivering progress for the still-open socket. When the request finally completes (
is_done = true),callbackat FetchTasklet.rs:2372 evaluatesvm_gone = !gate.enter()→true. It enters the shutdown branch, freesscheduled_response_buffer, unlocks, then callsderef_from_thread(task)twice (lines 2402-2404). - The second
deref_from_threadsees the 1→0 transition. At line 431 it evaluates!gate.enter()→true→dealloc_for_shutdown(this)→defer_shutdown_reclaimpushes{ctx: this, drop_fn: deinit_erased}ontoSHUTDOWN_RECLAIMS. - The parent process keeps running. Nothing ever drains
SHUTDOWN_RECLAIMS. The parkedFetchTaskletbox — with itsOption<Box<AsyncHTTP>>,request_headers: Headers,url_proxy_buffer,response_buffer/scheduled_response_buffercapacity, theConcurrentEnqueueGateref, and (depending on what was populated before terminate)native_response,hostname, etc. — sits in the Vec forever. Repeat steps 1-4 N times → N leaked tasklets. Unbounded growth in a long-running parent. - Eventually the parent process exits. If
BUN_DESTRUCT_VM_ON_EXITis unset,global_exitnever reachesshutdown_for_exit()and the Vec is simply leaked at_exit— harmless at that point but the steady-state leak in step 5 already happened. If it is set,shutdown_for_exit()iteratesSHUTDOWN_RECLAIMSand callsdeinit_erased→deinit→clear_data()on each parked worker tasklet on the main thread.clear_data()(FetchTasklet.rs:523, 526, 530, 539-541) executesself.response.clear()(ajsc::Weakregistered in the dead worker's WeakSet),Response::unref(native_response)(a JSC cell in the dead worker's GC heap),readable_stream_ref.deinit()/abort_reason.deinit()/check_server_identity.deinit()/clear_abort_signal()(JSCStronghandles in the dead worker's HandleSet). All of those structures were freed in step 2 — heap-use-after-free.
Why existing code doesn't prevent it
dealloc_for_shutdown's own doc (FetchTasklet.rs:565-578) spells out the contract: "the drain runs from global_exit() after the HTTP thread has parked but before destructOnExit, so deinit() there can release every handle on the right thread". That contract holds for tasklets owned by the main VM (whose destructOnExit runs after the drain), but is violated for worker tasklets — their JSC VM is destroyed in WebWorker::shutdown step 3 long before the main-VM drain, and the drain runs on the wrong thread anyway. The pre-PR is_shutting_down() arm had the same design assumption, but reaching it for a dead worker required first reading is_shutting_down from freed memory (the very UAF this PR fixes), so it was effectively unreachable. This PR makes the dealloc_for_shutdown route the deterministic outcome via the new !gate.enter() branch and explicitly relies on it as the cleanup mechanism.
The new regression test doesn't catch either effect: the subprocess exits right after the loop (so the steady-state leak isn't observed), and bunEnv doesn't set BUN_DESTRUCT_VM_ON_EXIT (so shutdown_for_exit() never drains and the parked boxes are simply abandoned at _exit).
Impact
- Memory leak (always): every worker terminated with an in-flight fetch leaks one
FetchTaskletplus its owned allocations until process exit. For a server that uses workers as a sandbox/timeout mechanism around outbound HTTP, this is unbounded RSS growth. - Exit-time UAF (under
BUN_DESTRUCT_VM_ON_EXIT): writes into the freed JSC HandleSet/WeakSet/GC heap of a long-dead worker, on the wrong thread.
Fix
The gate-closed (vm_gone) last-ref path should not park in SHUTDOWN_RECLAIMS. Instead, reclaim directly on the HTTP thread: free the Rust-side allocations (url_proxy_buffer, hostname, request_headers, http: Box<AsyncHTTP>, metadata, scheduled_response_buffer, request_body_streaming_buffer deref, etc.), drop the gate ref, and heap::destroy the box — without touching any JSC Strong/Weak/native_response field (those handles became invalid the moment the worker's JSC VM was torn down; leaking the handle slot is correct because the HandleSet/WeakSet that owned it no longer exists). One option is a clear_data_rust_only() that skips lines 523-541's JSC operations, plus a ManuallyDrop/mem::forget around the JSC-handle fields so drop(boxed) doesn't run their Drop impls. Keep dealloc_for_shutdown for the main-VM is_shutting_down() arm (gate open, VM alive), where its existing contract still holds.
There was a problem hiding this comment.
Fixed in d2817f0. You're right that the gate-closed branch made the park-at-exit path the deterministic outcome — and worse, ASAN CI runs with BUN_DESTRUCT_VM_ON_EXIT=1, so the exit drain's dead-heap deinit would have been reachable there.
What changed:
- New
FetchTasklet::reclaim_for_dead_vm: frees all Rust-side allocations immediately on whichever thread drops the last ref (Box<AsyncHTTP>, headers, URL/hostname buffers, metadata, response buffers, cert info, blob-store ref / sendfile fd, stream-buffer ref, box storage) and leaks only the JSC-registered handles viaManuallyDrop(their HandleSet/WeakSet died with the worker heap; the raw-ptr refs —sink/signal/native_response— are single-threaded refcounts we must not touch off-thread). - One subtlety beyond your sketch: the leaked
responseWeak keepson_response_finalizeregistered against the tasklet box until the worker's JSC heap is destroyed, so the box can't be freed while the heap is still unwinding. Handled by movingConcurrentEnqueueGate::close()to afterWebWorker__teardownJSCVM(gate closed now implies "heap gone ⇒ no finalizers"), and by parking a last-ref drop that lands while the gate is still open (is_shutting_down,!is_main_thread) on the gate itself —close()'s drain reclaims those right after heap teardown. dealloc_for_shutdownis now reachable only for main-VM tasklets, where its documented contract holds.
Verified: with BUN_DEBUG_FetchTasklet=1, the regression fixture shows reclaimForDeadVm ×8 for 8 terminated workers and zero deallocForShutdown parks; the test now runs with BUN_DESTRUCT_VM_ON_EXIT=1 explicitly (exit 0 under ASAN), so both the steady-state leak and the exit-drain UAF are covered.
There was a problem hiding this comment.
Status change: the reclaim_for_dead_vm machinery from d2817f0 is deferred to a follow-up and this PR is back to the park-at-exit disposition (plus the doc fixes and the BUN_DESTRUCT_VM_ON_EXIT=1 test hardening). Reason, with evidence:
Both CI builds containing the reclaim machinery segfault at process exit on Windows 2019 in tests that execute none of it:
http/serve.test.ts: crashes after all 195 tests pass, on x64 and x64-baseline, on both the new base (build 59827) and the old base (59844) — 4 crashes, identical 14-frame stack shape. The file spawns no workers, and the v1→v2 patch diff shows every non-worker path byte-identical.grpc-js/test-client.test.ts(node:http2 — JS-thread only, no VM backrefs off-thread) andbake/dev/production.test.ts(bun build --app, a 105 ms SSG subprocess with no fetch and no workers) also crash at exit, once or twice each.- The previous head (e58e25b — same gate, no reclaim machinery) was 16/16 green on the same Windows lanes, including
serve.test.tsat the same shard position, and no other recent build in the fleet shows this crash signature (checked every failed Windows-2019 job across the last 100 failed builds). - Linux ASAN cannot reproduce any of it (serve/bake/grpc all clean on the reclaim build).
I can't symbolize the Windows traces from this environment (artifact downloads and bun.report are blocked), so I can't explain the interaction — only demonstrate the correlation. Rather than block the Sentry crash fix on it, the reclaim hardening needs a follow-up with a Windows repro loop.
On the residual behavior this PR ships: dead-worker tasklets park in SHUTDOWN_RECLAIMS (the pre-PR disposition, now reached deterministically instead of after a UAF read). That's a bounded allocation per terminated-mid-fetch worker, reachable from a static (LSan-quiet). The exit-drain deinit of a dead worker's JSC handles under BUN_DESTRUCT_VM_ON_EXIT=1 touches bmalloc-backed memory — verified locally that ASAN does not flag it and the hardened regression test (which now sets that env var explicitly) passes; it's pre-existing and in scope for the same follow-up.
e58e25b to
d2817f0
Compare
|
CI note: build 59827 (previous head d2817f0) failed 9 Windows-2019 shards. Two of the failures are real segfaults —
To bisect, this push is the identical tree rebased back onto Decoded traces (image-relative, from the bun.report URLs in the job logs):
|
01ed6bc to
2dba9da
Compare
Windows CI failure — complete evidence, needs Windows-side symbolicationThree CI rounds later, here is the full picture. The Linux/macOS/ASAN matrix is green in every round; the regression test proves the Sentry-crash fix both ways. What remains is a Windows-2019-only segfault at process exit that I cannot diagnose further from this (Linux, egress-restricted) environment. Build matrix
Meanwhile every other PR build in the same time windows passes these lanes 16/16 (checked ~30 builds finishing 05:24–07:02 UTC, plus every failed Windows job across the last 100 failed builds — zero occurrences of this signature anywhere else). The crashAlways bun.report links (I cannot reach bun.report or the S3 artifacts from this environment to symbolize):
What this rules in/out
What's neededSomeone with a Windows machine (or bun.report access) to open the trace links above — the 14-frame main-thread exit stack will name the subsystem immediately — and/or loop |
eb101cf to
5ac198f
Compare
|
CI triage for build 59875 (head 5ac198f, doc-only delta): the only non-Windows failure is Windows lanes for this head were still running at triage time; expectations per the evidence comment above (the exit-segfault needs Windows-side symbolication — trace links there). No further CI re-rolls from my side. |
|
Final CI summary for this head (
The diff is green across the matrix; the remaining red is darwin agent capacity. I've used my one CI re-roll, so this is ready for a maintainer — either a targeted retry of the expired darwin jobs or review/merge as-is. |
|
Addendum to the CI summary: the previously-expired darwin lanes got agents and ran. darwin-14-x64 failed on exactly three files, each retried 4× with the same result — That's the complete picture: the diff is green across the matrix; the only red is darwin container-service flake. I've spent my one CI re-roll, so over to a maintainer for a targeted darwin-job retry or merge. |
The HTTP client thread's FetchTasklet callbacks dereference a raw VirtualMachine backref (the is_shutting_down read, the concurrent-task enqueue, and the loop wakeup), but WebWorker::shutdown frees the worker's VirtualMachine allocation with no synchronization against those callbacks. A progress update landing after the dealloc read freed memory and pushed onto the freed concurrent queue — a segfault on the queue-head atomic swap. Zig has the same logical race but frees worker VMs by destroying a private mimalloc heap, which typically leaves the pages mapped and masks it; the Rust port frees through the global allocator, so the race is a hard UAF. Fix: a refcounted ConcurrentEnqueueGate (mutex + vm_alive flag) shared between the VM and every FetchTasklet, so it outlives both sides. The HTTP thread brackets every VM access with enter()/exit(); worker shutdown close()s the gate — synchronizing with any in-flight gated section — then drains the concurrent queue (releasing parked tasklet refs while JSC is still alive) before invalidating and freeing the VM. A closed gate is handled exactly like is_shutting_down, without touching the VM.
The worker-shutdown caller of release_queued_tasks_for_shutdown runs with the HTTP daemon still live (the closed ConcurrentEnqueueGate, not a parked daemon, is what makes the drain complete), so the per-tag release fns own only the queued entry's counted ref — exclusivity exists only on the 1→0 transition. Update the two callee docs and the callback gate-exit note, and run the worker-terminate regression fixture with BUN_DESTRUCT_VM_ON_EXIT=1 so the process-exit reclaim paths are exercised on every lane (ASAN CI already sets it).
…actually exercises
ecc9a07 to
5eeacc9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/js/web/workers/worker-terminate-lifetime.test.ts`:
- Around line 124-126: The inFlight Promise currently only resolves on
worker.onmessage which lets the test hang if the worker errors or closes; update
the promise creation for inFlight so it resolves on worker.onmessage and rejects
on worker.onerror and worker.onclose (or via addEventListener for 'error' and
'close') and also handle abort/termination events where applicable; reference
the inFlight variable and the worker.onmessage / worker.onerror / worker.onclose
handlers to wire failure paths to reject instead of relying on timeouts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d24c79b8-9f91-441b-b492-fc8f642d84dd
📒 Files selected for processing (6)
src/jsc/VirtualMachine.rssrc/jsc/event_loop.rssrc/jsc/web_worker.rssrc/runtime/dispatch.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/js/web/workers/worker-terminate-lifetime.test.ts
|
CI triage for build 61218 (head 7c583fb, comment-only delta): the only failures so far are I've already spent this PR's one CI re-roll, so no retrigger from my side; the failing test needs an upstream fixture pin or a fleet-level fix, not anything in this PR. |
|
Superseded by #32071, which generalizes this gate to a process-global registry of live VM/event-loop addresses and converts every cross-thread producer (fetch, S3, work pool tasks, node:fs, zlib, napi async work / threadsafe functions / finalizers, watchers, the waiter thread, bundler completion), not just One thing this PR does that #32071 does not: draining the worker's own concurrent queue at shutdown to release already-queued task refs (a leak fix, not a crash fix). Noted in #32071's description as a possible follow-up. |
1 participant
Crash
Sentry BUN-3BR3 (aarch64 siblings BUN-3DMH / BUN-3DRK): ~20 crashes in 14 days, all Linux (x64 + aarch64), canaries
49c97de6b…7069b3092. Segfault on the HTTP client thread:Repro
Terminate a worker while a
fetch()it started is still streaming. The regression test (test/js/web/workers/worker-terminate-lifetime.test.ts) does this in a loop; on an unfixed ASAN build it crashes deterministically:Cause
FetchTaskletholds a rawjavascript_vm: &'static VirtualMachinebackref that the HTTP client thread dereferences on every result callback — theis_shutting_downread, the push ontoEventLoop.concurrent_tasks, and the uws loop wakeup.WebWorker::shutdownfrees the worker'sVirtualMachineallocation (std::alloc::dealloc) with no synchronization against those callbacks, so a progress update landing after teardown reads freed memory and swaps the freed queue head. (worker.terminate()/ in-workerprocess.exit()bypass the event-loop keepalive thatpoll_refprovides for natural exit.)The Zig original has the same logical race but frees worker VMs via
mi_heap_destroyof a private heap, which typically leaves the pages mapped and masks it; the Rust port frees through the global allocator, so the race is a hard UAF. Linux-only in telemetry is just teardown timing.Fix
A small refcounted
ConcurrentEnqueueGate(mutex +vm_aliveflag,src/jsc/event_loop.rs) shared between the VM (one ref, created inVirtualMachine::init) and everyFetchTasklet(one ref, taken inget(), released with the tasklet), so the gate strictly outlives both sides:callback,on_write_request_data_drain,deref_from_thread) withenter()/exit(). A closed gate is handled exactly likeis_shutting_down, without touching the VM.WebWorker::shutdownclose()s the gate —close()takes the same mutex, so it synchronizes with any in-flight gated enqueue and every laterenter()fails — then drains the concurrent queue (releasing queued tasklet refs while JSC is still alive) beforedestroy()/dealloc. Since no gated producer can enqueue afterclose(), the drain observes every push that won the race.close()never blocks shutdown beyond one in-flight enqueue (nanoseconds). Dead-worker tasklets whose last ref drops on the HTTP thread reclaim through the existingdealloc_for_shutdownpark list — the pre-PR disposition, now reached without the UAF read. Replacing that park with immediate reclamation was attempted (d2817f0) and reverted: both CI builds containing it segfault at process exit on Windows in tests that execute none of it (serve.test.ts ×4 across two bases and both binaries, grpc-js, bake SSG), while this exact tree was 16/16 green on the same lanes; details in the review thread. That hardening (a bounded per-terminate parked allocation) moves to a follow-up with a Windows repro loop.Verification
bun bd(ASAN heap-use-after-free on the HTTP Client thread) and passes with the fix; it runs its fixture withBUN_DESTRUCT_VM_ON_EXIT=1so the process-exit reclaim paths are exercised on every lane.worker-terminate-lifetime.test.ts4/4;fetch-abort-stream-body+fetch-abort-queued+body-stream: 9088 pass / 0 fail;worker.test.ts,fetch.test.ts,fetch-leak.test.tsfailures identical with and without the fix (container env).cargo check+cargo clippyclean onbun_jsc/bun_runtime.