worker: post every cross-thread completion by ScriptExecutionContext id so terminate() cannot UAF the freed VM - #35767
worker: post every cross-thread completion by ScriptExecutionContext id so terminate() cannot UAF the freed VM#35767robobun wants to merge 7 commits into
Conversation
worker.terminate() frees the VirtualMachine box while work already handed to a process-global thread (WorkPool, HTTP thread, bundle thread) is still in flight; the later completion posted back through a captured BackRef<EventLoop> / &VirtualMachine, which is a pointer into the freed box. Every still-reproducing cross-thread UAF in this class converges on EventLoop::enqueue_task_concurrent, and several callers 'guard' with an off-thread vm.is_shutting_down() read that is itself a read of freed memory. Off-thread jobs now carry the originating ScriptExecutionContextIdentifier (a u32, cannot dangle) and post through ScriptExecutionContextIdentifier::post_concurrent_task, which looks the context up and enqueues under allScriptExecutionContextsMapLock, the same lock markTerminating() (already called in WebWorker::shutdown before the VM dealloc) takes to set the terminating flag. Either the poster's critical section ran first (task enqueued; shutdown's drain reclaims it) or markTerminating ran first (poster gets false and runs its abandon path without touching the VM). Converted: the three generic helpers (WorkTask, ConcurrentPromiseTask, AnyTaskJob, covering Bun.file/Bun.write, Transpiler.transform, Glob.scan, pbkdf2/scrypt/generateKeyPair/zstd/Secrets), plus the direct callers FetchTasklet, PasswordJob, NativeZlib/Brotli/Zstd, AsyncFSTask/NewAsyncCpTask/AsyncReaddirRecursiveTask, S3HttpSimpleTask/S3HttpDownloadStreamingTask, Archive AsyncTask, JSBundleCompletionTask, TranspilerJob, and ConcurrentCppTask's unref_concurrently (WebCrypto). Design doc: docs/ROOT-B-SHUTDOWN-FENCE.md. Verify harness: repro/rootB-verify/verify.mjs (100x arm-every-source + terminate; stock canary SIGSEGVs on teardown 1, debug+ASAN heap-UAF on teardown 1-3; 10/10 clean after).
WalkthroughChangesThe PR introduces Worker shutdown fence
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Addressed in 0cdab21:
|
|
Updated 9:14 PM PT - Jul 25th, 2026
❌ @robobun, your commit 96e26ed has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35767That installs a local version of the PR into your bun-35767 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…, root test scratch under harness tempDir COMPLETION_VTABLE.enqueue_task_concurrent (the bundler's off-thread onLoad/onResolve plugin dispatch) was still derefing jsc_event_loop; route it through context_id.post_concurrent_task like complete_on_bundle_thread. The jsc_event_loop field (and the event_loop parameter on create_and_schedule_completion_task) are now dead and removed. Worker test scratch dir rooted under body.mjs's directory (the harness tempDir), so 'using dir' reclaims it instead of leaking into $TMPDIR. repro/verify.mjs now owns one scratch root and removes it on exit. Test's Bun.build grows a plugin so the COMPLETION_VTABLE path is armed.
|
…m_callback, drop dead event_loop fields - node_zlib_binding: wrap do_work() in is_alive() (writes into the pinned JS ArrayBuffer), same best-effort skip as AnyTaskJob::run_task. - ConcurrentTask::destroy_from_callback: from_callback allocates two boxes (outer ConcurrentTask + inner ManagedTask); the abandon paths at FetchTasklet deref_from_thread/on_write_request_data_drain and NewAsyncCpTask now reclaim both. - WorkTask / ConcurrentPromiseTask: event_loop field was write-only after on_finish moved to context_id; removed with its imports.
|
a9a1475 addresses the round-3 nits:
|
…erred in design doc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@repro/rootB-verify/verify.mjs`:
- Line 14: Validate ROOTB_ITER after converting it in the ITERATIONS
initialization so it must be a finite positive safe integer; reject zero,
negatives, NaN, fractions, and Infinity before the teardown loop runs. Preserve
the existing default of 100 when the environment variable is unset, and fail
clearly for invalid values.
In `@test/js/web/workers/worker-terminate-lifetime.test.ts`:
- Around line 194-203: Update the scratch path setup around the module URL in
the worker test to convert import.meta.url with fileURLToPath before passing it
to path.dirname. Add the required node:url import and preserve the existing
scratch directory and cleanup behavior.
🪄 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: 3724e752-b2b6-46d2-90b8-d4eceaf090d9
📒 Files selected for processing (29)
docs/ROOT-B-SHUTDOWN-FENCE.mdrepro/rootB-verify/verify.mjsrepro/rootB-verify/worker-body.mjssrc/event_loop/ConcurrentTask.rssrc/jsc/ConcurrentPromiseTask.rssrc/jsc/CppTask.rssrc/jsc/JSGlobalObject.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/WorkTask.rssrc/jsc/any_task_job.rssrc/jsc/bindings/EventLoopTaskNoContext.cppsrc/jsc/bindings/EventLoopTaskNoContext.hsrc/jsc/bindings/ScriptExecutionContext.cppsrc/jsc/virtual_machine_exports.rssrc/runtime/api/Archive.rssrc/runtime/api/JSBundler.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/node/node_fs.rssrc/runtime/node/node_zlib_binding.rssrc/runtime/node/zlib/NativeBrotli.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/node/zlib/NativeZstd.rssrc/runtime/server/HTMLBundle.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/s3/download_stream.rssrc/runtime/webcore/s3/simple_request.rstest/js/web/workers/worker-terminate-lifetime.test.ts
💤 Files with no reviewable changes (1)
- src/runtime/api/JSBundler.rs
| // lets make sure that we always call deinit from main thread | ||
| // `from_callback` heap-allocates a fresh `ConcurrentTaskItem`; the queue | ||
| // takes ownership of it. | ||
| Self::enqueue_concurrent( | ||
| self_.javascript_vm, | ||
| ConcurrentTask::from_callback(this, FetchTasklet::deinit_callback), | ||
| ); | ||
| let node = ConcurrentTask::from_callback(this, FetchTasklet::deinit_callback); | ||
| if !Self::enqueue_concurrent(self_.context_id, node) { | ||
| // SAFETY: ownership not transferred; `node` is a `from_callback` allocation. | ||
| unsafe { ConcurrentTask::destroy_from_callback(node) }; | ||
| // SAFETY: last ref; see the `!is_alive()` branch above. | ||
| unsafe { FetchTasklet::dealloc_for_shutdown(this) }; | ||
| } |
There was a problem hiding this comment.
🟡 The worker-terminate abandon path here (both the !is_alive() branch at line 400 and the new !enqueue_concurrent() fallback at line 412) still routes to dealloc_for_shutdown, which parks the tasklet in the process-global SHUTDOWN_RECLAIMS list — drained only by the main VM's global_exit() — so at process exit deinit() releases the tasklet's Strong/Weak/JSPromiseStrong handles into the long-freed worker HandleSet, and the boxes accumulate unboundedly across worker create/terminate cycles until then. Pre-existing (the old is_shutting_down() gate reached the same sink, and its flag read was itself a UAF), so not a regression — but it violates this PR's own abandon-path contract ("must not touch Strong/Weak/JSPromiseStrong"). For worker contexts, leak the box outright (matching WorkTask/ConcurrentPromiseTask/AnyTaskJob/PasswordJob) and reserve dealloc_for_shutdown for the main-context process-exit case its own doc-comment describes.
Extended reasoning...
What the bug is
FetchTasklet::deref_from_thread's abandon path — both the !self_.context_id.is_alive() branch (line 394→400) and the new !Self::enqueue_concurrent(...) race-window fallback (line 408→412) — calls dealloc_for_shutdown(this). That function (lines 527–532) parks the tasklet via http::defer_shutdown_reclaim(this, FetchTasklet::deinit_erased) into the process-global SHUTDOWN_RECLAIMS list (HTTPThread.rs:1404, 1411–1415). SHUTDOWN_RECLAIMS is drained only inside shutdown_for_exit() (HTTPThread.rs:1469–1473), whose sole runtime caller is VirtualMachine::global_exit() on the main VM at process exit. WebWorker::shutdown never calls it — and cannot, since the HTTP thread is process-global and other VMs still use it.
The mechanism's own doc-comment (lines 519–523: "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") is written entirely for the main-VM process-exit case — it assumes the tasklet's JSC handles belong to the same VM whose global_exit() is running and whose HandleSet is still live. That invariant does not hold for a tasklet whose JSC heap belongs to a worker that terminated long ago.
The code path
The abandon closure in the HTTP callback (line ~2477) reaches this via 2× deref_from_thread(task) when is_done. The last of those hits ref_count==0, sees !context_id.is_alive() (or races into !enqueue_concurrent()), and calls dealloc_for_shutdown. Same path from on_write_request_data_drain's abandon → deref_from_thread.
Why existing code doesn't prevent it
dealloc_for_shutdown correctly avoids touching JSC handles immediately on the HTTP thread. But it defers a full deinit() — clear_data() calls self.response.clear() (jsc::Weak, line 473), readable_stream_ref.deinit(), abort_reason.deinit() / check_server_identity.deinit() (StrongOptional), clear_abort_signal(), and then drop(boxed) runs Drop for promise: JSPromiseStrong. Every one of those releases a slot in the worker's JSC HandleSet/WeakSet, freed by teardownJSCVM when that worker terminated. This is exactly what the PR's own docs/ROOT-B-SHUTDOWN-FENCE.md §Abandon-path forbids: "must not touch Strong/Weak/JSPromiseStrong … the HandleSet is freed" — just deferred to process exit rather than avoided.
Impact
- Deferred UAF at process exit: main JS thread runs
deinit()on a box whose JSC handles point into a freed worker HandleSet. Low practical severity (process is dying), but ASAN-visible. - Unbounded accumulation: each terminated worker with an in-flight fetch whose HTTP-thread callback lands post-
markTerminating()parks one ~1KB+ box inSHUTDOWN_RECLAIMSfor the rest of the process lifetime — accumulates across repeated worker create/terminate in a long-running server, contrary to the design doc's "bounded: one per terminated worker per in-flight op" (which assumes the leak dies with the worker, not accumulates process-wide).
Step-by-step proof
- Worker starts
fetch("http://127.0.0.1:.../");FetchTasklet::get()heap-allocates the tasklet withcontext_id = <worker id>,abort_reason/check_server_identity/response/promiseall handles into the worker's JSC heap; ref_count = 2; HTTP thread has the request in flight. - Parent calls
worker.terminate(). Worker JS thread: step 2bmarkTerminating()(setsisTerminatingunder the contexts-map lock) → 2d drain → step 3teardownJSCVM(frees the worker's HandleSet/WeakSet) → step 5dealloc(vm_ptr). - HTTP thread receives the final result and enters
callback.task_ref.context_id.is_alive()returnsfalse(isTerminating set) →abandon(task_ref)→ 2×deref_from_thread(task). The second call hits ref_count==0 →!self_.context_id.is_alive()→dealloc_for_shutdown(this)→SHUTDOWN_RECLAIMS.push({this, deinit_erased}). - Process runs on. Server repeats steps 1–3 across many worker create/terminate cycles; each parks another tasklet in
SHUTDOWN_RECLAIMS. - Eventually the main VM calls
global_exit()→bun_http::shutdown_for_exit()→for r in SHUTDOWN_RECLAIMS { (r.drop_fn)(r.ctx) }. On the main JS thread,FetchTasklet::deinit_erased→deinit(this)→clear_data()→abort_reason.deinit()etc. reach into the worker HandleSet freed at step 2/3 — heap-use-after-free.
Pre-existing?
Yes. Before this PR the guard was self_.javascript_vm.is_shutting_down(). During worker-shutdown steps 2a–4 that flag read true, so worker tasklets reached dealloc_for_shutdown(this) then too (identical call at the old line 400); after step 5 the flag read was itself a UAF into the freed VM box. So the leak-into-SHUTDOWN_RECLAIMS + deferred-process-exit-UAF was already there. This PR fixes the immediate UAF-read and is a strict improvement, but keeps routing to the same broken sink and adds a second deterministic entry (line 412). Per REVIEW.md "Fix the whole class in the same PR", flagging because the PR touches these exact lines and its own abandon-path contract explicitly forbids what dealloc_for_shutdown does for worker contexts.
Fix
For worker contexts, do what every other converted site does: leak the box outright (matching WorkTask/ConcurrentPromiseTask/AnyTaskJob), or free the pure-Rust fields (scheduled_response_buffer, response_buffer, request_headers, http, url_proxy_buffer) and mem::forget the JSC-handle fields — mirroring PasswordJob. Reserve dealloc_for_shutdown for the main-VM process-exit case it was designed for, e.g. gate on self_.context_id == <main context id> or on self_.javascript_vm being the main VM (that pointer read is safe here since FetchTasklet is a separate heap box).
There was a problem hiding this comment.
Agreed this is the wrong sink for worker tasklets, but it is the pre-existing sink this branch has always taken (the old is_shutting_down() gate reached the same dealloc_for_shutdown call, and that flag read was itself a UAF). This PR changed only the gate, not the sink. The proper fix is to make dealloc_for_shutdown (or a worker-specific sibling) free the Rust payload and mem::forget the JSC handles instead of parking for a global_exit() that belongs to a different VM; that touches the FetchTasklet deinit machinery rather than the enqueue fence, so I would rather land it separately. Leaving unresolved for the maintainer to weigh in.
|
CI builds 81365 and 81483 both failed with every The diff builds and passes its regression test locally under debug+ASAN (20/20 clean teardowns where stock canary SIGSEGVs at teardown ~9). Ready for a re-run once the queue clears. |
|
Superseded by the Worker rewrite in #37075, which landed on main and closed #32073. It solves the same problem this PR generalised (off-thread completions posting back into a VM that terminate() may have freed) at the same chokepoint: every cross-thread completion (thread pool, HTTP thread, bundle thread, napi, JSC helper threads) now posts through a per-VM handle that teardown closes, and a late completion is refused and released by its producer. Verified on current main (165dc9f, debug+ASAN build): this PR's test, "terminate() while cross-thread WorkPool completions are in flight does not UAF on enqueue", passes 4 out of 4 runs, and the standalone harness from repro/rootB-verify reports 30 out of 30 teardowns clean. Closing. |
2 participants
Problem
worker.terminate()frees the worker'sVirtualMachinebox while work already handed to a process-global thread (WorkPool, HTTP thread, bundle thread) is still in flight. The later completion posts back through aBackRef<EventLoop>/&'static VirtualMachine/*const JSGlobalObjectcaptured at schedule time, every one of which is a pointer into the freed box or the freed JSC heap.All reproduced cross-thread UAFs in this class converge on
EventLoop::enqueue_task_concurrent(src/jsc/event_loop.rs:997). A check inside the funnel cannot help:&selfthere is already inside the freed box. Several callers also "guard" with an off-threadvm.is_shutting_down()read, which is itself a read of freed memory.Fix
Off-thread jobs now carry the originating
ScriptExecutionContextIdentifier(au32; cannot dangle) instead of a raw event-loop/VM pointer, and post throughbacked by the same
allScriptExecutionContextsMapLock+isTerminating()gate thatScriptExecutionContext::postTaskToalready uses for C++ posters.WebWorker::shutdownalready callsmarkTerminating()(which takes that lock) before it drains the concurrent queue and frees the VM, so every Rust poster now serializes into one of two cases: either its whole critical section ran first (task enqueued; the subsequentrelease_queued_tasks_for_shutdown()drain observes it), ormarkTerminating()ran first (poster getsfalseand runs its abandon path without touching the VM).Converted:
WorkTask<C>,ConcurrentPromiseTask<C>,AnyTaskJob<C>(coversBun.file/Bun.write,Transpiler.transform,Glob.scan,crypto.pbkdf2/scrypt/generateKeyPair,Bun.zstdCompress,Bun.secrets, image pipeline).ConcurrentCppTask(crypto.subtle): the trailingunref_concurrently()now goes through the id under the same lock.FetchTasklet,PasswordJob,NativeZlib/NativeBrotli/NativeZstd,AsyncFSTask/NewAsyncCpTask/AsyncReaddirRecursiveTask,S3HttpSimpleTask/S3HttpDownloadStreamingTask,Archive::AsyncTask,JSBundleCompletionTask,TranspilerJob.Abandon path: when
post_concurrent_taskreturnsfalse, free the freshly-allocatedConcurrentTasknode (ownership was not transferred) and any pure-Rust payload; leak the job box when it holdsStrong/JSPromiseStronghandles into the dead JSC heap (bounded: one per terminated worker per in-flight op).Design doc:
docs/ROOT-B-SHUTDOWN-FENCE.md.Verification
New test
terminate() while cross-thread WorkPool completions are in flight does not UAF on enqueueintest/js/web/workers/worker-terminate-lifetime.test.ts: a subprocess repeatedly creates a worker that arms one in-flight op of every converted source (Bun.write/Bun.file,fs.promises.*,crypto.pbkdf2/scrypt/generateKeyPair,Bun.zstdCompress,Bun.password.hash,zlib.deflate/gzip,Bun.Transpiler.transform,Glob.scan,crypto.subtle.digest,Bun.build,fetch) and is terminated once armed.0x8on teardown ~9 (release);heap-use-after-freeon teardown 1-3 (debug+ASAN).Standalone harness
repro/rootB-verify/verify.mjsarms the same set x100 for the manual gate (ROOT-B VERIFY: PASS (10 teardowns)under ASAN here; default 100 iterations).Relationship to open PRs
Generalises the per-callsite patches in #35158 (Bun.build), #35154 (AnyTaskJob) and #35156 (Bun.password) into one chokepoint using the identifier-route design. Uses the same C++
ScriptExecutionContext__postConcurrentTaskshape as #35158.Explicitly out of scope (not enqueue-shaped; need their own fixes): nested-worker child-init reading a freed parent VM,
node:quicfinalizer ordering,RedisClient::finalize,Bun.SQLhandle crashes during terminating-VM JS execution, and thenapi_async_work/fs.watch/ shell-task /AsyncModule-wake posters and the Windows-onlyWriteFileWindows/CopyFileWindowsAsyncMkdirpcompletions (same shape, follow-up).no test proof · iteration 1 · 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
Fixes #32073
Towards #33936
Towards #15964