bundler: post Bun.build completions by ScriptExecutionContext id to survive worker.terminate() - #35158
bundler: post Bun.build completions by ScriptExecutionContext id to survive worker.terminate()#35158robobun wants to merge 7 commits into
Conversation
…urvive worker.terminate() worker.terminate() mid-Bun.build() crashed the whole process: the bundle thread's complete_on_bundle_thread() dereferenced a BackRef<EventLoop> into a VM the worker thread had already dealloc'd (heap-use-after-free under ASAN, SIGSEGV on release). A second queued build then dereferenced the worker's freed env loader in Transpiler::init. Route both the completion post and the CompletionDispatch vtable enqueue through ScriptExecutionContext::postConcurrentTask, which holds the contexts-map lock across the lookup + isTerminating() check + enqueue and so serializes with WebWorker::shutdown's existing markTerminating() the same way postTaskTo() already does for C++ callers. Queued builds whose owning context has begun shutdown are skipped before create_and_configure_transpiler touches worker-owned state.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
Comment |
|
Reproduced the reported UAF on canary (SIGSEGV 3/5) and under debug+ASAN (5/5) with the fixture in this PR's new test. With the fix, the full Scope after review (9a0318f):
CI: |
|
Updated 3:48 PM PT - Jul 22nd, 2026
❌ @robobun, your commit 9a0318f has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35158That installs a local version of the PR into your bun-35158 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the duplicate flags: #32071 is stale (conflicts with main, clippy failing) and uses an address-keyed registry that the review flagged for ABA on VM-address reuse; the #34154 is Jarred's broader This PR is scoped to the bundler case only (6 files). If #34154 lands first I'll rebase this down to just the test, or close it if #34154's test already covers the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…fix test assertions
The is_owner_alive() pre-check was a TOCTOU that narrowed but did not
close the env-loader race: the contexts-map lock is released before
Transpiler::init runs, and a running build's resolver holds the raw
loader pointer for its whole duration. Clone the env map into the
completion task at creation time (on the JS thread, where the VM's
loader is guaranteed live) so the bundle thread never dereferences
worker-lifetime memory. The pre-check stays as an early-out so a build
queued by a dead worker is skipped instead of producing a result that
complete_on_bundle_thread would drop anyway.
Remove the now-dead jsc_event_loop field (both consumers rerouted to
context_id.post_concurrent_task), its stale doc comment, and the
event_loop parameter it was fed from.
Test: replace the not.toContain panic-string checks with a combined
{stdout, stderr, exitCode} object assertion, and disable LSan leak
detection for the spawned subprocess (pre-existing worker-termination
leaks on main, node:fs Binding / WebWorker box, would abort it under
CI's detect_leaks=1; the UAF under test still aborts with a
heap-use-after-free report regardless).
Also: free result/log/env on the bundle thread when the post fails, so
only the small box + dead-VM JSC handle remain; allow
clippy::not_unsafe_ptr_arg_deref on the HOST_EXPORT enqueue thunk.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/api/js_bundle_completion_task.rs:770-784— Whenpost_concurrent_taskreturns false here, the droppedConcurrentTaskItemmay be a pluginonLoad/onResolvedispatch (viaenqueue_on_js_loop_for_plugins, bundle_v2.rs:1498-1505), not just the final completion — itsgraph.pending_itemsincrement is never decremented, sowait_for_parse(predicatepending_items == 0) parks the process-wideBundleThreadsingleton forever insidegenerate_in_new_thread, and every subsequentBun.build()from any thread (including main) hangs. Before this PR the same path UAF-crashed; this converts it to a silent permanent process-wide bundler hang for the plugins case, which the new test (no plugins) does not exercise. The false branch needs to actively abort the build (e.g. flagis_done()/ route to the same owner-dead short-circuit as the pre-start check), not just reclaim the heap task.Extended reasoning...
What the bug is
COMPLETION_VTABLE.enqueue_task_concurrentis not only the final-completion post — it is the JS-loop hop for pluginonLoad/onResolvedispatch mid-build.BundleV2::enqueue_on_js_loop_for_plugins(bundle_v2.rs:1498-1506) routesResolve::dispatch(bundle_v2.rs:1128),Load::dispatch(bundle_v2.rs:1259), andDeferredBatchTask::schedule(DeferredBatchTask.rs:57) throughcompletion.enqueue_task_concurrent(task)→ this vtable thunk. Each such dispatch has a matchinggraph.pending_itemsincrement (bundle_v2.rs:2873); the counter is only decremented when the plugin's response posts back to the bundle thread's mini loop viaon_load_async/on_resolve_async→decrement_scan_counter(bundle_v2.rs:2881-2883).With this PR, when
post_concurrent_taskreturns false (worker terminated mid-bundle), the thunk drops theConcurrentTaskItemand returns.run_on_js_threadnever runs, the plugin never fires, no response is ever posted back, andpending_itemsnever reaches 0.The specific code path that triggers it
init_and_runconstructsAnyEventLoop::default()=Mini(Box<MiniEventLoop>).run_from_js_in_new_threadcallswait_for_parse(bundle_v2.rs:2013), which loopsAnyEventLoop::tick_rawwithis_done()as the sole predicate;is_done()(bundle_v2.rs:1994-2010) returns true only whengraph.pending_items == 0.tick_raw's Mini arm callsMiniEventLoop::tick_once, which — once other WorkPool parse tasks drain and the queue is empty — blocks in the uws loop'stick()waiting for a wakeup that would only come from the plugin response'smini.enqueue_task_concurrent. That wakeup never arrives, so the bundle thread parks forever insidegenerate_in_new_thread.Why existing code doesn't prevent it
BundleThreadis a process-wide singleton (BundleThread.rsmod singleton,OnceLock-backed).thread_mainpops one completion at a time and runsgenerate_in_new_threadsynchronously. Once that call is stuck inwait_for_parse, the loop never returns toqueue.pop(), so every subsequentBun.build()from any thread — including the main thread — queues ontoBundleThread::queueand is never popped. Theis_owner_alive()pre-check added in this PR only guards builds that haven't started yet; it does nothing for a build already insidewait_for_parsewhen the worker terminates. Thecomplete_on_bundle_threadfalse-branch handles the final post correctly, but that code is unreachable —generate_in_new_threadnever returns to call it.Step-by-step proof
- Worker calls
Bun.build({ plugins: [...] }). Task is enqueued; bundle thread pops it,is_owner_alive()→ true, entersgenerate_in_new_thread→init_and_run→run_from_js_in_new_thread. - Scan phase encounters a module matching a plugin filter.
Load::dispatchincrementsgraph.pending_itemsand callsenqueue_on_js_loop_for_plugins→COMPLETION_VTABLE.enqueue_task_concurrent(c, task). - Meanwhile the parent calls
worker.terminate().WebWorker::shutdowncallsmarkTerminating()underallScriptExecutionContextsMapLock. ScriptExecutionContext__postConcurrentTasktakes the same lock, seescontext->isTerminating(), returnsfalse. The vtable thunk doesdrop(heap::take(task))and returns.- No response ever posts back to the mini loop.
pending_itemsstays ≥ 1.wait_for_parseloops onis_done()= false; once the mini loop's task queue drains,tick_onceblocks in(*loop_ptr()).tick()with nothing to wake it. - The main thread later calls
Bun.build(...).singleton::enqueuepushes ontoBundleThread::queueandwaker.wake()s — butthread_mainis stuck two frames deep insidegenerate_in_new_threadand never reachesqueue.pop(). The main-thread promise never settles.
Impact
Before this PR the same path UAF-crashed at
jsc_event_loop.enqueue_task_concurrent(the PR's own first ASAN trace). This PR converts that into a silent, permanent, process-wide bundler hang for the plugins case — arguably worse than the crash for diagnosability, since there is no stack trace and the symptom ("all myBun.buildcalls hang") appears far from the cause ("a worker with plugins was terminated once, minutes ago"). The new test uses no plugins, so it does not exercise this path. This is precisely REVIEW.md "Every error/abort/timeout path actively completes the operation. Settle every pending promise slot (an unsettled promise pins objects and hangs callers forever)" and "Cover the variant matrix, not just the repro" (the plugin variant is a sibling entry point sharing the same fix).How to fix
Reclaiming the
ConcurrentTaskItemis necessary but not sufficient — the false branch must also signal the mini loop that the build is dead. Options:- Set an "owner dead" flag on
BundleV2(reachable viafrom_completion_handle(c)→ the completion's stashedtranspiler: *mut BundleV2) thatis_done()also checks, and post a wakeup to the mini loop sotick_onceunblocks; havewait_for_parse's caller bail with an error when the flag is set. - Or route this case to the same short-circuit as the pre-start
is_owner_alive()check: mark the completion result asErrand post a task back to the mini loop (which is bundle-thread-owned and always alive) that decrementspending_itemsfor each dropped dispatch, sowait_for_parsereturns andthread_mainproceeds tocomplete_on_bundle_thread(which will itself observe the dead context and drop the post).
Either way, the invariant is: every path out of
enqueue_task_concurrentmust eventually letis_done()return true.Not a duplicate of the existing BundleThread.rs:240 comment — that concerns the
is_owner_alive()pre-check TOCTOU onenv_loader(a UAF duringTranspiler::init/resolver, before or outside the plugin round-trip); this is a distinct hang in the mid-build plugin dispatch path via the vtable thunk. - Worker calls
… hang COMPLETION_VTABLE.enqueue_task_concurrent carries mid-build plugin onLoad/onResolve dispatches (via enqueue_on_js_loop_for_plugins), not just the final completion. Dropping one when post_concurrent_task returned false left graph.pending_items unbalanced, so wait_for_parse parked the process-wide BundleThread singleton forever and every subsequent Bun.build() from any thread hung. Revert that path to the pre-PR direct enqueue so plugin builds behave exactly as on main; complete_on_bundle_thread (the reported no-plugins crash) keeps the context-id post.
|
On the plugin-dispatch hang: confirmed. |
There was a problem hiding this comment.
The earlier 🔴 env-loader TOCTOU is closed by the task-owned snapshot in 43ee822 — Transpiler::init and resolver.env_loader now consume self.env, which is the task's own Loader::init(cloned map), so the mid-bundle NODE_PATH deref no longer touches worker-lifetime memory. is_owner_alive() is now just an early-out, not load-bearing for safety. Remaining findings this pass are nits.
Extended reasoning...
Confirming resolution of the prior blocking finding after 43ee822/b6722c5f. The env-loader lifetime is now task-owned end-to-end (construction → create_and_configure_transpiler → configure_bundler's resolver.env_loader = NonNull::new(transpiler.env...) → do_compilation's to_executable), and the dead-owner branch in complete_on_bundle_thread correctly drops result/log/env/env_map in place. Not restating the three inline nits. This remains a cross-thread memory-safety change with a documented residual (plugin-dispatch path unchanged) and a competing maintainer approach in #34154, so deferring to human review.
…ire worker error Set did_load_process on the task-owned loader so run_env_loader's load_process() early-returns instead of re-walking OS environ (which would clobber JS-set values and alloc per-entry on every Bun.build). Destroy the plugin on the clone_with_allocator error arm so the now-fallible window between receiving plugins and storing it in the box doesn't leak it on OOM. Wire the test fixture's worker 'error' event to reject so a future eval failure surfaces immediately instead of timing out.
The HTMLBundle::Route caller passes a plugin borrowed from the server's ServePluginsState::Loaded(Box<Plugin>); destroying it in the shared error arm would tombstone + unprotect the server's live plugin. Release at the acquisition site instead: JSBundler.rs owns its plugin and destroys it in .map_err; HTMLBundle.rs does nothing on error (its borrow is still owned by the server).
There was a problem hiding this comment.
All prior review findings have been addressed (env snapshot with did_load_process = true, plugin OOM cleanup moved to the owning Bun.build caller, worker error wired to reject, dead-field cleanup). No new issues this round, but this is cross-thread lifetime / UAF-fix work with a couple of deliberate design tradeoffs — the plugin-dispatch vtable keeps the pre-existing direct enqueue (and its UAF on termination) to avoid hanging wait_for_parse, and the completion-task box + JSPromiseStrong/KeepAlive are intentionally leaked when the target context is dead — so a human should sign off on those.
What was reviewed:
ScriptExecutionContext__postConcurrentTasklock shape mirrorspostTaskTo(enqueue underallScriptExecutionContextsMapLock, same path already callsenqueue_task_concurrentunder that lock — no new deadlock).is_owner_alive()TOCTOU is now only a work-skip optimization; the env-loader UAF it originally guarded is closed by the task-owned snapshot, and the bundle thread touches no other worker-owned state inconfigure_bundler/init_and_run.deinitnull-checksenvso the dead-context branch's early free doesn't double-free;HTMLBundle::Route's borrowed plugin is no longer destroyed on the OOM path.
Extended reasoning...
Overview
Fixes a process-wide crash when worker.terminate() races an in-flight Bun.build() inside the worker. The bundle thread previously posted the completion via a raw BackRef<EventLoop> into the worker's freed VM; a second UAF hit the worker's freed bun_dotenv::Loader in Transpiler::init. The fix reroutes complete_on_bundle_thread through a stable ScriptExecutionContextIdentifier posted under the C++ contexts-map lock (same serialization as postTaskTo/markTerminating), snapshots the env map into a task-owned Loader at schedule time, and adds an is_owner_alive() skip in BundleThread::thread_main. New FFI: ScriptExecutionContext__postConcurrentTask, ScriptExecutionContext__isAlive, ScriptExecutionContextIdentifier__forGlobalObject, Bun__EventLoop__enqueueConcurrentTask. Nine files touched across bun_bundler, bun_jsc, bun_runtime, C++ bindings, and one new test.
Security risks
None user-facing. This is internal cross-thread lifetime management; no untrusted-input parsing, auth, or crypto paths are touched.
Level of scrutiny
High. This is exactly the category REVIEW.md flags as most-blocked: cross-thread ownership, UAF fixes, ref-count balance across terminal paths, and a new lock-gated FFI surface. The change is well-reasoned and the ASAN evidence is solid, but it also encodes non-obvious tradeoffs a maintainer should ratify:
COMPLETION_VTABLE.enqueue_task_concurrentdeliberately keeps the directjsc_event_loopenqueue (pre-existing UAF on termination) because dropping a pluginonLoad/onResolvepost would strandgraph.pending_itemsand hang the process-wide bundle thread. That's documented in-line but is a "known residual UAF" a human should accept.- On the dead-context path, the
JSBundleCompletionTaskbox (with itsJSPromiseStrong,KeepAlive,pluginshandle,BackRef<JSGlobalObject>) is leaked because those point into the dead VM and can't be released off the JS thread. The large payloads (result,log,env/env_map) are reclaimed. Bounded, but a design call. - Per-
Bun.buildenv-map clone is a small new allocation cost on every build.
Other factors
Three prior review rounds; every finding was addressed in follow-up commits (43ee822, b6722c5, 17be429, 9a0318f). The new subprocess test asserts a combined {stdout, stderr, exitCode} object, wires the worker error event to reject, scales rounds/modules by build type, and disables detect_leaks with a stated reason. rust:check-all reported 10/10; the file-level suite passes locally per the PR body. The postConcurrentTask path holds the same lock across the same enqueue_task_concurrent call that postTaskTo → postTaskConcurrently → queueTaskConcurrently already does, so no new lock-ordering is introduced.
|
Generalised into #35767 (one chokepoint for every cross-thread poster, same |
|
Closing: this is fixed on current main by the Worker teardown rewrite in #37075. |
2 participants
Problem
worker.terminate()whileBun.build()is in flight inside the worker crashes the whole process. The bundler runs on its own thread andJSBundleCompletionTask::complete_on_bundle_threadposts the completion back into the worker'sEventLoopvia a rawBackRef<EventLoop>, butWebWorker::shutdownhas alreadydealloc'd the VM (which contains thatEventLoop).process.exit()inside the worker and an uncaught throw are equal triggers.Reproduced on release canary (SIGSEGV 3/5 runs) and under debug+ASAN (UAF 5/5) with the fixture in the new test: a worker that spins
Bun.buildin a loop and is terminated after a short jittered delay.Fixing the completion post uncovered a second UAF one layer down: a build that was queued on the bundle thread but not yet started gets popped after the worker is gone, and
create_and_configure_transpiler→Transpiler::initdereferences the worker's freedbun_dotenv::Loader(captured as a raw pointer inself.env):Cause
JSBundleCompletionTaskborrows worker-lifetime state (jsc_event_loop: BackRef<EventLoop>,env: *mut Loader,global_this) on the assumption that the owning JS thread outlives the task. That holds for the main thread but not for a worker: nothing inWebWorker::shutdownparks or waits for the bundle thread before freeing the VM and env loader, so the bundle thread's only pointers into the target VM become dangling the momentshutdown()reachesstd::alloc::dealloc.ScriptExecutionContext::postTaskTo/markTerminating()already implement the correct lock-gated-flag-then-drain serialization for C++ cross-thread posters (WebCrypto, MessagePort, Worker.dispatchExit), andWebWorker::shutdownalready callsmarkTerminating()before it drains the concurrent queue and frees the VM. The bundle thread just wasn't using it.Fix
JSBundleCompletionTasknow captures the originatingScriptExecutionContextIdentifier(a stableu32) atBun.buildcall time.complete_on_bundle_threadand theCompletionDispatchvtableenqueue_task_concurrentpost via a newScriptExecutionContext__postConcurrentTask(id, task): underallScriptExecutionContextsMapLock, look up the context, checkisTerminating(), and only then call back intoEventLoop::enqueue_task_concurrent. This is the same shape aspostTaskTobut for a pre-allocated RustConcurrentTaskItem. If the context is gone or terminating, theConcurrentTaskItemis reclaimed on the bundle thread and the completion task itself is leaked (itsdeinittouches JS-thread-ownedJSPromiseStrong/Plugin/KeepAlivestate that no longer exists).BundleThread::thread_mainchecksis_owner_alive()(newCompletionStructtrait method, backed byScriptExecutionContext__isAlive) before starting a popped build, so a build queued by a now-dead worker skipscreate_and_configure_transpilerinstead of dereferencing the freed env loader.ScriptExecutionContextIdentifier::{post_concurrent_task, is_alive}andJSGlobalObject::script_execution_context_identifier()Rust bindings.Verification
New test
terminating a worker mid-Bun.build() does not crash the processintest/bundler/bun-build-api.test.tsspawns a subprocess that repeatedly creates a worker running two concurrentBun.buildlanes over a multi-hundred-module graph and terminates it mid-bundle.main: release SIGSEGV at address0x0within two rounds; debug+ASAN heap-use-after-free deterministically on round 0.bun-build-api.test.tspasses (50 pass, 1 pre-existing skip, 1 todo).bun run rust:check-all: 10/10 targets ok.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts