Fix use-after-free when worker.terminate() races in-flight fetch/work-pool completions - #34154
Fix use-after-free when worker.terminate() races in-flight fetch/work-pool completions#34154Jarred-Sumner wants to merge 2 commits into
Conversation
|
Updated 12:28 PM PT - Jul 22nd, 2026
❌ @autofix-ci[bot], your commit c3e5318 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 34154That installs a local version of the PR into your bun-34154 --bun |
|
get some rest, Jarred, it's 3 AM :) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThis change adds gated cross-thread VM handles, migrates asynchronous completion paths from raw VM or event-loop references, updates fetch and S3 shutdown handling, maps cancelled DNS completions to rejected promises, and adds worker termination regression tests. Cross-thread VM shutdown safety
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 6 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jsc/RuntimeTranspilerStore.rs (1)
518-529: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftDo not leave worker-VM teardown relying on this gate. The FIXME shows
terminate()can free the VM and hive slot mid-flight, so this path still has a use-after-free risk. Add the drain/join protocol here or prove worker VMs can never queueTranspilerJobs.🤖 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/jsc/RuntimeTranspilerStore.rs` around lines 518 - 529, Fix the worker-VM teardown race in the queueing path around RuntimeTranspilerStore and event_loop: add the required drain/join synchronization so terminate() cannot free the VM, hive slot, or transpiler store while the job is being queued and scheduled. Alternatively, establish and enforce a guarantee that worker VMs never queue TranspilerJob instances; remove the FIXME only once the use-after-free risk is eliminated.src/runtime/node/node_fs.rs (1)
2176-2210: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winAsyncReaddirRecursiveTask should carry a VMHandle
finish_concurrently()still does an unsafebun_vm_concurrently()dereference from work-pool threads, so worker shutdown can race this path and free the VM/event loop before the last recursive-readdir subtask finishes. Addvm: VMHandlehere and enqueue the completion throughvm.enqueue_task_concurrent(...)likeAsyncFSTask.🤖 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/node/node_fs.rs` around lines 2176 - 2210, Add a VMHandle field to AsyncReaddirRecursiveTask, initialize it when creating the task, and update finish_concurrently() to enqueue completion via vm.enqueue_task_concurrent(...) instead of directly dereferencing bun_vm_concurrently(). Match AsyncFSTask’s VM lifetime and completion-enqueue pattern so the VM remains alive until all recursive-readdir subtasks finish.
🤖 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 `@src/jsc/VirtualMachine.rs`:
- Around line 995-1017: In cross_thread_handle, replace the handle_gate Arc’s
method-style clone with the explicit Arc::clone form while preserving the
existing expect and VMHandle construction.
In `@src/jsc/vm_handle.rs`:
- Around line 27-28: Add a separate adjacent SAFETY comment immediately before
the unsafe Sync implementation in the VMHandle declarations, while retaining the
existing safety comment for Send. Ensure both unsafe impls have their own
explicit safety justification.
In `@src/runtime/webcore/fetch/FetchTasklet.rs`:
- Around line 402-418: Ensure worker shutdown drains FetchTasklets parked by
defer_shutdown_reclaim(), not only the process-exit path in
HTTPThread::shutdown_for_exit(). Update the
WebWorker::shutdown()/VirtualMachine::destroy() shutdown flow to invoke the
existing drain at the appropriate point, while preserving global_exit() behavior
and avoiding duplicate or unsafe reclamation.
In `@src/threading/ShutdownGate.rs`:
- Around line 1-65: Add a multi-threaded stress test for the ShutdownGate state
machine, repeatedly racing many concurrent enter/leave operations against
close_and_wait. Verify successful entrants always leave, entrants after closure
are rejected, close_and_wait completes only after all guests exit, and the gate
remains closed afterward.
In `@test/js/web/workers/worker-terminate-lifetime.test.ts`:
- Around line 134-137: Update both regression test assertion blocks in
worker-terminate-lifetime.test.ts to assert stderr first and unconditionally
before stdout or exitCode. Preserve the existing stdout and exitCode assertions,
and ensure any ASAN/UAF output on stderr fails the tests even when the process
prints "done\n" and exits successfully.
---
Outside diff comments:
In `@src/jsc/RuntimeTranspilerStore.rs`:
- Around line 518-529: Fix the worker-VM teardown race in the queueing path
around RuntimeTranspilerStore and event_loop: add the required drain/join
synchronization so terminate() cannot free the VM, hive slot, or transpiler
store while the job is being queued and scheduled. Alternatively, establish and
enforce a guarantee that worker VMs never queue TranspilerJob instances; remove
the FIXME only once the use-after-free risk is eliminated.
In `@src/runtime/node/node_fs.rs`:
- Around line 2176-2210: Add a VMHandle field to AsyncReaddirRecursiveTask,
initialize it when creating the task, and update finish_concurrently() to
enqueue completion via vm.enqueue_task_concurrent(...) instead of directly
dereferencing bun_vm_concurrently(). Match AsyncFSTask’s VM lifetime and
completion-enqueue pattern so the VM remains alive until all recursive-readdir
subtasks finish.
🪄 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: 326c18b8-28c9-4226-bcab-a8faee7ff33f
📒 Files selected for processing (23)
src/jsc/ConcurrentPromiseTask.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/VirtualMachine.rssrc/jsc/WorkTask.rssrc/jsc/lib.rssrc/jsc/vm_handle.rssrc/jsc/web_worker.rssrc/runtime/api/Archive.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/napi/napi_body.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/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/s3/download_stream.rssrc/runtime/webcore/s3/multipart.rssrc/runtime/webcore/s3/simple_request.rssrc/threading/ShutdownGate.rssrc/threading/lib.rstest/js/web/workers/worker-terminate-lifetime.test.ts
💤 Files with no reviewable changes (1)
- src/runtime/webcore/s3/multipart.rs
ea006b6 to
ef9135e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/RuntimeTranspilerStore.rs (1)
516-530: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftConfirmed:
TranspilerJobis not yet migrated toVMHandleand remains UAF-exposed on workerterminate().
self.vmis a raw*mut VirtualMachinedereferenced unconditionally throughoutrun()/dispatch_to_main_thread()(not just here) — if a worker'sterminate()frees the VM while this job is mid-flight on a WorkPool thread, this is the same use-after-free class the rest of this PR fixes. The FIXME correctly notes aVMHandlegate alone isn't sufficient here (the job's hive slot itself lives invm.transpiler_store, so the fix needs a drain/join at worker shutdown, not just a pinned pointer).Given this matches the PR's own "documented follow-up" scope, would you like me to open a tracking issue for migrating
TranspilerJob(drain/jointranspiler_store.queuebefore the VM is freed), or help draft an initial approach?🤖 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/jsc/RuntimeTranspilerStore.rs` around lines 516 - 530, Track the documented follow-up for TranspilerJob rather than treating the VMHandle gate as sufficient: migrate the raw self.vm usage across run() and dispatch_to_main_thread() to a safe VMHandle-based lifetime model, and add worker-shutdown drain/join coordination for transpiler_store.queue before terminate() frees the VM. Preserve the existing event-loop dispatch behavior while ensuring queued jobs cannot access a freed VM or hive slot.
🤖 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 `@src/jsc/vm_handle.rs`:
- Around line 42-51: Update VMHandle::with so the gate count is released on
every exit path, including when the caller-supplied closure f panics. After a
successful gate.enter(), introduce a scope-bound drop guard whose cleanup calls
self.gate.leave(), then invoke f without a separate unconditional leave;
preserve the existing None and Some(result) behavior.
In `@src/runtime/napi/napi_body.rs`:
- Around line 125-129: Remove the function-local has_pending_exception checks
from napi_get_prototype and the additionally affected path, relying on the
centralized NAPI_PREAMBLE pending-exception gate instead. Verify those entry
points pass through NAPI_PREAMBLE; only retain a local check if that centralized
gate does not cover the path and the sanctioned interim behavior requires it.
---
Outside diff comments:
In `@src/jsc/RuntimeTranspilerStore.rs`:
- Around line 516-530: Track the documented follow-up for TranspilerJob rather
than treating the VMHandle gate as sufficient: migrate the raw self.vm usage
across run() and dispatch_to_main_thread() to a safe VMHandle-based lifetime
model, and add worker-shutdown drain/join coordination for
transpiler_store.queue before terminate() frees the VM. Preserve the existing
event-loop dispatch behavior while ensuring queued jobs cannot access a freed VM
or hive slot.
🪄 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: 66cc8c2a-7fdf-46cc-9e77-238fef048e01
📒 Files selected for processing (23)
src/jsc/ConcurrentPromiseTask.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/VirtualMachine.rssrc/jsc/WorkTask.rssrc/jsc/lib.rssrc/jsc/vm_handle.rssrc/jsc/web_worker.rssrc/runtime/api/Archive.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/napi/napi_body.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/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/s3/download_stream.rssrc/runtime/webcore/s3/multipart.rssrc/runtime/webcore/s3/simple_request.rssrc/threading/ShutdownGate.rssrc/threading/lib.rstest/js/web/workers/worker-terminate-lifetime.test.ts
💤 Files with no reviewable changes (1)
- src/runtime/webcore/s3/multipart.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/RuntimeTranspilerStore.rs (1)
516-530: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftConfirmed:
TranspilerJobis not yet migrated toVMHandleand remains UAF-exposed on workerterminate().
self.vmis a raw*mut VirtualMachinedereferenced unconditionally throughoutrun()/dispatch_to_main_thread()(not just here) — if a worker'sterminate()frees the VM while this job is mid-flight on a WorkPool thread, this is the same use-after-free class the rest of this PR fixes. The FIXME correctly notes aVMHandlegate alone isn't sufficient here (the job's hive slot itself lives invm.transpiler_store, so the fix needs a drain/join at worker shutdown, not just a pinned pointer).Given this matches the PR's own "documented follow-up" scope, would you like me to open a tracking issue for migrating
TranspilerJob(drain/jointranspiler_store.queuebefore the VM is freed), or help draft an initial approach?🤖 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/jsc/RuntimeTranspilerStore.rs` around lines 516 - 530, Track the documented follow-up for TranspilerJob rather than treating the VMHandle gate as sufficient: migrate the raw self.vm usage across run() and dispatch_to_main_thread() to a safe VMHandle-based lifetime model, and add worker-shutdown drain/join coordination for transpiler_store.queue before terminate() frees the VM. Preserve the existing event-loop dispatch behavior while ensuring queued jobs cannot access a freed VM or hive slot.
🤖 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 `@src/jsc/vm_handle.rs`:
- Around line 42-51: Update VMHandle::with so the gate count is released on
every exit path, including when the caller-supplied closure f panics. After a
successful gate.enter(), introduce a scope-bound drop guard whose cleanup calls
self.gate.leave(), then invoke f without a separate unconditional leave;
preserve the existing None and Some(result) behavior.
In `@src/runtime/napi/napi_body.rs`:
- Around line 125-129: Remove the function-local has_pending_exception checks
from napi_get_prototype and the additionally affected path, relying on the
centralized NAPI_PREAMBLE pending-exception gate instead. Verify those entry
points pass through NAPI_PREAMBLE; only retain a local check if that centralized
gate does not cover the path and the sanctioned interim behavior requires it.
---
Outside diff comments:
In `@src/jsc/RuntimeTranspilerStore.rs`:
- Around line 516-530: Track the documented follow-up for TranspilerJob rather
than treating the VMHandle gate as sufficient: migrate the raw self.vm usage
across run() and dispatch_to_main_thread() to a safe VMHandle-based lifetime
model, and add worker-shutdown drain/join coordination for
transpiler_store.queue before terminate() frees the VM. Preserve the existing
event-loop dispatch behavior while ensuring queued jobs cannot access a freed VM
or hive slot.
🪄 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: 66cc8c2a-7fdf-46cc-9e77-238fef048e01
📒 Files selected for processing (23)
src/jsc/ConcurrentPromiseTask.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/VirtualMachine.rssrc/jsc/WorkTask.rssrc/jsc/lib.rssrc/jsc/vm_handle.rssrc/jsc/web_worker.rssrc/runtime/api/Archive.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/napi/napi_body.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/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/s3/download_stream.rssrc/runtime/webcore/s3/multipart.rssrc/runtime/webcore/s3/simple_request.rssrc/threading/ShutdownGate.rssrc/threading/lib.rstest/js/web/workers/worker-terminate-lifetime.test.ts
💤 Files with no reviewable changes (1)
- src/runtime/webcore/s3/multipart.rs
🛑 Comments failed to post (2)
src/jsc/vm_handle.rs (1)
42-51: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
VMHandle::with()leaks the gate's guest count on panic, risking a shutdown hang.
self.gate.leave()only runs iffreturns normally. Iffpanics, the guest count is never decremented, so a subsequentclose_and_wait()(worker termination) blocks forever waiting for a count that will never reach zero. As per path instructions: "Reference counts must balance on every terminal path: success, error, cancellation, and finalize." Sincefis a caller-supplied closure (not proven panic-free), the count should be balanced with a drop guard instead of a plain post-call decrement.🔒️ Proposed fix using a drop guard
pub fn with<R>(&self, f: impl FnOnce(&VirtualMachine) -> R) -> Option<R> { if !self.gate.enter() { return None; } + // Ensures `leave()` runs even if `f` panics, keeping the guest count + // balanced on every terminal path (including panic/unwind). + struct LeaveGuard<'a>(&'a ShutdownGate); + impl Drop for LeaveGuard<'_> { + fn drop(&mut self) { + self.0.leave(); + } + } + let _guard = LeaveGuard(&self.gate); // SAFETY: gate held open — `close_and_wait()` in worker shutdown // blocks until we `leave()`, so the allocation outlives this call. let result = f(unsafe { self.vm.as_ref() }); - self.gate.leave(); Some(result) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.pub fn with<R>(&self, f: impl FnOnce(&VirtualMachine) -> R) -> Option<R> { if !self.gate.enter() { return None; } struct LeaveGuard<'a>(&'a ShutdownGate); impl<'a> Drop for LeaveGuard<'a> { fn drop(&mut self) { self.0.leave(); } } let _guard = LeaveGuard(&self.gate); // SAFETY: gate held open — `close_and_wait()` in worker shutdown // blocks until we `leave()`, so the allocation outlives this call. let result = f(unsafe { self.vm.as_ref() }); Some(result) }🤖 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/jsc/vm_handle.rs` around lines 42 - 51, Update VMHandle::with so the gate count is released on every exit path, including when the caller-supplied closure f panics. After a successful gate.enter(), introduce a scope-bound drop guard whose cleanup calls self.gate.leave(), then invoke f without a separate unconditional leave; preserve the existing None and Some(result) behavior.Source: Path instructions
src/runtime/napi/napi_body.rs (1)
125-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Ad hoc pending-exception check re-introduced in
napi_get_prototype.A prior learning on this exact function recommended relying on the centralized
NAPI_PREAMBLEgate rather than adding function-localhas_pending_exception()checks. This change adds exactly that pattern back. If the centralized gate now covers this path, the local check is redundant; if not, please confirm this is the sanctioned interim approach rather than a re-introduced anti-pattern.Based on learnings, "avoid adding ad-hoc pending-exception checks inside individual N-API entry-point functions... Instead, rely on the centralized NAPI_PREAMBLE pending-exception gate."
Also applies to: 882-896
🤖 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/napi/napi_body.rs` around lines 125 - 129, Remove the function-local has_pending_exception checks from napi_get_prototype and the additionally affected path, relying on the centralized NAPI_PREAMBLE pending-exception gate instead. Verify those entry points pass through NAPI_PREAMBLE; only retain a local check if that centralized gate does not cover the path and the sanctioned interim behavior requires it.Source: Learnings
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 `@src/runtime/dns_jsc/dns.rs`:
- Around line 2116-2129: Update the cancelled early-return branch in
DNSLookup::on_complete to call resolver.request_completed() before
Self::destroy(this), matching the non-empty completion path and sibling
cancellation handlers while preserving the existing rejection 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: 1d3cbd2f-b7c8-4efc-b4d1-a183961d8c55
📒 Files selected for processing (1)
src/runtime/dns_jsc/dns.rs
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/jsc/web_worker.rs:1289-1290— A workerFetchTaskletwhose intrusiveconcurrent_taskwas already enqueued beforeclose_cross_thread_gate()and is then drained byrelease_queued_tasks_for_shutdown()never has its transfer aborted:__bun_release_task_at_shutdown's FetchTasklet arm (dispatch.rs:1169-1173) onlyderef()s and never resetshas_schedule_callback, so subsequent HTTP-thread callbacks fail the CAS at line 2447, take the early return at 2454-2459, and never reach the|| enqueued.is_none()→schedule_shutdownabort this PR adds at line 2481. Body bytes accumulate inscheduled_response_bufferuntil the server closes. The one-line fix — alsohas_schedule_callback.store(false, Relaxed)in that arm — lets the next callback win the CAS, getNonefromenqueue_concurrent, and hit the abort this PR already adds. (The dispatch.rs comment "HTTP daemon is already parked" is also now false for the worker-shutdown caller and should be updated.)Extended reasoning...
What the bug is
This is distinct from the two existing
dealloc_for_shutdownfindings on this PR (which coverenqueue_concurrentreturningNoneorSome(false)at the moment of enqueue). This covers the third case: the tasklet's intrusiveconcurrent_taskwas successfully enqueued (Some(true)) beforeclose_cross_thread_gate()ran, and is then drained byrelease_queued_tasks_for_shutdown()at web_worker.rs:1290.__bun_release_task_at_shutdown's FetchTasklet arm at dispatch.rs:1169-1173 does exactly one thing:task_tag::FetchTasklet => { // SAFETY: `task.ptr` is the live heap `FetchTasklet`; HTTP daemon is // already parked so we hold the sole reference. FetchTasklet::deref(task.ptr.cast::<FetchTasklet>()); true }
It does not reset
has_schedule_callbackand does not abort the transfer. Its SAFETY comment ("HTTP daemon is already parked") is true for the main-VMglobal_exit()path (which callsshutdown_for_exit()before the drain) but false for the worker-shutdown caller this PR now sequences at web_worker.rs:1289-1290 — worker shutdown does not park the shared HTTP daemon.The normal dispatch path —
on_progress_updateat FetchTasklet.rs:815 — is the only writer ofhas_schedule_callback = false. When the shutdown-drain arm runs instead, that flag stays stuck attrue.Step-by-step proof
- Worker starts
fetch()on a streaming response. HTTP thread fires a mid-stream callback (is_done = false): appends body bytes at 2431-2435, wins thehas_schedule_callbackCASfalse→trueat 2447, callsenqueue_concurrent→gate.enter()succeeds → the intrusiveconcurrent_taskis pushed →Some(true). Refcount stays 2. worker.terminate()→WebWorker::shutdown():vm.close_cross_thread_gate()at web_worker.rs:1289 (waits out any in-flight guest, so the enqueue in step 1 is complete and the task is in the queue), thenrelease_queued_tasks_for_shutdown()at :1290.drop_concurrent_cpp_tasks()forwards the FetchTasklet entry fromconcurrent_tasksintoself.tasks;__bun_release_task_at_shutdownruns the FetchTasklet arm →FetchTasklet::deref()(refcount 2→1).has_schedule_callbackis stilltrue.- Worker VM freed (step 5 of
shutdown()). - HTTP thread receives the next chunk.
callback()locks the tasklet's mutex, appends the chunk toscheduled_response_bufferat 2431-2435, then the CAS at 2447 fails withErr(true)(flag stilltruefrom step 1, never reset). It takes the early return at 2453-2459 — never reachingenqueue_concurrentat 2465, so it never observes the closed gate (None) and never reaches theschedule_shutdownabort at 2481-2484 that this PR added specifically for the dead-worker case. - Repeat for every subsequent chunk. The transfer runs until the server closes; body bytes accumulate in the leaked tasklet's
scheduled_response_buffer. For the PR's own new fetch test (server drips 1 KB/ms forever), each terminated worker's fetches whose task was queued at the moment of gate-close keep streaming into leaked buffers untilprocess.exit(0).
Why existing code doesn't prevent it
- The PR's new abort at FetchTasklet.rs:2481 (
|| enqueued.is_none()→schedule_shutdown) is only reachable after winning the CAS at 2447; a stuck-trueflag makes it unreachable. close_cross_thread_gate()only refuses new guests; it does not touch tasks already in the queue.- The CAS-fail early-return path (lines 2340-2459) touches only tasklet-owned fields (
mutex,http,result,response_buffer,scheduled_response_buffer,has_schedule_callback) — neverjavascript_vm— so the closed gate is never observed on this path.
Impact and severity
This is nit severity because the mid-stream accumulation itself is pre-existing — the CAS-fail early-return path did not touch the VM before this PR either, so a stuck-
trueflag after a shutdown-drain already produced this exact behavior. Only the eventualis_done=true→deref_from_thread→is_shutting_down()read UAF'd before, and this PR fixes that. So the PR is a strict improvement over baseline (leak instead of crash) for this ordering.However, it's worth mentioning because: (a) the PR description explicitly claims "Fetches … in flight are aborted on their next HTTP-thread event … once the gate is closed", and this ordering defeats that claim; (b) this PR adds
close_cross_thread_gate()immediately before the drain and rewrites the abort branch at 2481, so it's squarely in-scope per "fix the whole class in the same PR"; (c) the dispatch.rs:1166-1171 comment ("HTTP daemon is already parked so we hold the sole reference") is now false for the worker-shutdown caller.Fix
Have the FetchTasklet arm of
__bun_release_task_at_shutdownalso do(*task.ptr.cast::<FetchTasklet>()).has_schedule_callback.store(false, Ordering::Relaxed)before thederef(). Then the next HTTP-thread callback wins the CAS, callsenqueue_concurrent→None(gate closed), and hits theschedule_shutdownabort at line 2481 that this PR already adds. This is a one-line change that reuses the abort machinery this PR built. The comment at dispatch.rs:1166-1171 should also be updated to note that the HTTP daemon is not parked on the worker-shutdown path. - Worker starts
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 `@src/threading/ShutdownGate.rs`:
- Around line 68-109: Update the close_and_wait_drains_racing_guests test to
deterministically hold at least one guest inside before starting closure, then
invoke close_and_wait concurrently from two waiter threads and release the
in-flight guest so both waiters must drain and return. Retain the post-close
assertions and ensure the workload exercises rejected entries by asserting
rejected is greater than zero.
🪄 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: f847e941-f3a8-4f0e-8087-ff61e5be62d8
📒 Files selected for processing (7)
src/jsc/web_worker.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/dns_jsc/dns.rssrc/runtime/node/node_fs.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/s3/client.rssrc/threading/ShutdownGate.rs
💤 Files with no reviewable changes (2)
- src/runtime/crypto/PasswordObject.rs
- src/runtime/webcore/s3/client.rs
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This PR is terrible code. Redo it.
ce6b717 to
6598046
Compare
|
Pushed b42566d — rebased on current main and an idiomatic-Rust pass:
Suite 9/9, 15-scenario LSan matrix clean, clippy clean, check-all 10/10 on the pushed head. |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/jsc/web_worker.rs:1283— The first line of this comment block was accidentally dedented to column 0 (// Stop cross-thread posters first: markTerminating() serializes) while its continuation lines at :1284-1290 keep their 12-space indent, visually breaking the block inside a deeply-nested scope.rustfmtdoes not reflow//comment indentation, so this won't be auto-fixed — restore the 12-space indent.Extended reasoning...
What changed
The diff at
src/jsc/web_worker.rs:1283shows:- // Stop cross-thread posters first: markTerminating() serializes +// Stop cross-thread posters first: markTerminating() serializes // with postTaskTo() on the contexts-map lock, so after this call // every task another thread has already enqueued is visible to the // drain below and no new one can land. teardownJSCVM() will call
Line 1283 lost its 12-space leading indent and now sits at column 0. The continuation lines (:1284-1290) keep their original 12-space indentation. This code lives inside
impl WebWorker's terminate path, several nesting levels deep — the surrounding statements (rare.close_all_socket_groups(...)above andScriptExecutionContext__markTerminating(...)below) are both at 12-space indent.Why it happened
This PR inserts a new
abort_pending_transfers/close_and_waitblock immediately after this comment (theScriptExecutionContext__markTerminatingcall is followed by the new abort-registry walk and gate close). The dedent is a stray whitespace edit from that insertion — nothing in the semantic change touches this comment line's content.Why it won't self-heal
rustfmtdoes not reflow//line-comment indentation inside function bodies; it preserves whatever indentation the author wrote. So neithercargo fmtnor CI's format check will restore this — it will persist in the tree until manually fixed.Impact
Formatting only — no runtime effect. The comment block is visually broken (first line flush-left, next seven lines indented), which reads as if the first line is a stray file-level note rather than the head of the block explaining why
markTerminating()precedes the drain.Step-by-step proof
- Before this PR, line 1283 reads
// Stop cross-thread posters first: markTerminating() serializes— 12 leading spaces, matching lines 1284-1290 and the surrounding statements. - The diff hunk shows exactly one change to this line:
- // Stop ...→+// Stop .... No content change; only the 12 leading spaces are removed. - Lines 1284-1290 are untouched (no
+/-) and retain 12-space indent. - The next code line,
ScriptExecutionContext__markTerminating(vm.global());, is at 12-space indent, confirming the enclosing block depth. rustfmttreats//comments as opaque tokens on their own lines and does not adjust their leading whitespace, sobun run rust:fmt/cargo fmtwill not restore the indent.
Fix
Restore the 12-space indent on line 1283:
// Stop cross-thread posters first: markTerminating() serializes - Before this PR, line 1283 reads
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Clean up the code. This is still slop.
worker.terminate() freed the VirtualMachine while work-pool / HTTP-thread completions still held raw pointers into it; the late enqueue into the freed event loop's concurrent queue corrupted whatever reused the slab (heap-use-after-free on Bun Pool, StructureID asserts, or an unwrap panic in AsyncFSTask::run_from_js_thread depending on timing). Pin producers: VirtualMachine owns an Arc<ShutdownGate>; vm.pin() returns an RAII GateGuest. Every cross-thread producer (node:fs ops, recursive cp/readdir, dns, zlib/brotli/zstd writes, Bun.password, pbkdf2 and the generic AnyTaskJob, napi async work, Archive, Bun.build, stat-watcher, runtime transpiler, fetch, S3) takes a pin at creation on the JS thread and drops it on the completing thread after the completion enqueue. Terminate aborts registered in-flight transfers (fetch/S3) so their pins drop promptly, closes the gate and waits for all pins, then reclaims queued completions per-tag with JSC still alive, then tears down. Main-VM exit closes without waiting (its box is never freed). AsyncFSTask::run_from_js_thread bails early when the Strong is empty or the VM is stopping, so the drain-race panic cannot abort the process. Rebased onto 47597ab and addresses the review on #34154: InitialStatTask pinned, FetchTasklet::release_at_shutdown pin-drop ordering, duplicate clippy allow, stale event_loop/stat-watcher doc comments. The drain-time leak residuals (AsyncCpTask/TranspilerJob/RequestContext) stay in Known residuals. Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
8ece895 to
e9c14e1
Compare
|
Rebased onto 47597ab as a single commit (e9c14e1) and addressed the open review threads. What changed vs. the previous push:
Verification:
Drain-time leak findings (AsyncCpTask / TranspilerJob / RequestContext S3 ctx / StatWatcher restat) stay in Known residuals; they are bounded per-terminate leaks, not crashes. Backup branch at |
| async () => { | ||
| using scratch = tempDir("worker-terminate-fs", { | ||
| big: Buffer.alloc(4 << 20, 0x5a).toString("binary"), | ||
| }); | ||
| await using proc = Bun.spawn({ |
There was a problem hiding this comment.
🟡 The tempDir("worker-terminate-fs", { big: Buffer.alloc(4 << 20, ...) }) at :168-170 runs for all 6 workPoolJobs entries, but only the "node:fs readFile/writeFile/copyFile" entry reads process.env.FS_SCRATCH — the other five never touch it, so that's 5 unneeded 4MB alloc→toString('binary')→write→cleanup cycles per suite run. Trivially avoidable by gating on job.includes("FS_SCRATCH") (or moving the readFile/writeFile/copyFile case out of the loop); small in absolute terms vs. each test's worker-churn cost, so not blocking.
Extended reasoning...
What the inefficiency is
The for (const [name, job] of workPoolJobs) loop at worker-terminate-lifetime.test.ts:165 wraps every test body in:
using scratch = tempDir("worker-terminate-fs", {
big: Buffer.alloc(4 << 20, 0x5a).toString("binary"),
});and passes FS_SCRATCH: String(scratch) in env unconditionally. But workPoolJobs has 6 entries, and only the last one — "node:fs readFile/writeFile/copyFile" (:157-163) — reads process.env.FS_SCRATCH in its worker source. The other five entries (Bun.password, node:zlib, node:fs stat, node:dns, node:crypto) never touch the scratch dir or the env var.
Why nothing else needs it
Reading each workPoolJobs source string:
Bun.password:Bun.password.hash('hunter2', ...)— no filesystem access.node:zlib:gzip(Buffer.alloc(1 << 16, 7), swallow)— in-memory buffer.node:fs:stat(process.execPath)— reads the bun binary, notFS_SCRATCH.node:dns:lookup('localhost')— no filesystem access.node:crypto:pbkdf2(...)— in-memory.node:fs readFile/writeFile/copyFile:const f = process.env.FS_SCRATCH; fsp.readFile(f + '/big')...— the only consumer.
So 5 of 6 iterations create a temp dir, allocate a 4MB Buffer, convert it via toString('binary') (a per-byte Latin1 decode into a JS string in the debug/ASAN test-runner process), write it to disk, and clean it up on scope exit — all for nothing.
Impact (small)
The absolute cost is modest: ~5 × (4MB Buffer.alloc + toString('binary') + mkdtemp + 4MB write + rm), on the order of tens of ms total. Each of these tests already spawns a subprocess that churns perRound (12 or 32) worker VMs serially with await Bun.sleep(i % 4) between them and a 20-60s timeout, so the wasted setup is ≪1% of wall-clock. The debug+ASAN slowdown does not proportionally amplify disk I/O; it's the Buffer.alloc/toString in the ASAN-instrumented test-runner process that's the marginally slower part. REVIEW.md's "Keep tests fast (~1s per test)" / "a new file over ~10s on the default lane gets scrutinized" applies in spirit, but this is not what's making the file slow.
Step-by-step proof
- worker-terminate-lifetime.test.ts:150-164 —
workPoolJobsarray with 6[name, source]tuples. - :165 —
for (const [name, job] of workPoolJobs) {— iterates all 6. - :168-170 — inside the test body (so it runs once per test, 6× total):
using scratch = tempDir("worker-terminate-fs", { big: Buffer.alloc(4 << 20, 0x5a).toString("binary") }). - :194 —
env: { ...bunEnv, FS_SCRATCH: String(scratch) }— passed to every spawned child. - Grep the 6
sourcestrings forFS_SCRATCH: only the entry at :157-163 ("node:fs readFile/writeFile/copyFile") contains it. The other 5 sources never read the env var, so the scratch dir is created and torn down without effect for those iterations.
Fix
Gate the tempDir on the one entry that needs it, e.g.:
using scratch = job.includes("FS_SCRATCH")
? tempDir("worker-terminate-fs", { big: Buffer.alloc(4 << 20, 0x5a).toString("binary") })
: { path: "", [Symbol.dispose]() {} };or hoist the readFile/writeFile/copyFile case into its own test(...) outside the loop. Either is a few-line change with no behavior impact on the other five tests.
| // Reclaimed unrun by the terminate drain: the box owns no refs (the | ||
| // scheduler is kept alive by the StatWatcher's RefPtr) — plain free. | ||
| fn release_unrun(holder: *mut Holder) { | ||
| // SAFETY: queue-owned box popped by the drain; sole owner. | ||
| drop(unsafe { bun_core::heap::take(holder) }); | ||
| } | ||
| // SAFETY: `holder_ptr` was just `heap::alloc`'d and is exclusively | ||
| // owned here until `update_timer` (or the drain) reclaims it. |
There was a problem hiding this comment.
🟡 🟡 The pre-existing 3-line SAFETY comment at :330-332 ("holder_ptr was just heap::alloc'd … vm is the live per-thread VM (JSC_BORROW). addr_of_mut! so the field pointer inherits whole-Box provenance") was left in place when fn release_unrun (with its own doc at :333-334) was inserted between it and the unsafe {} block it documented, and a replacement SAFETY was added at :339-340. Lines 330-332 now sit directly above release_unrun and describe nothing there — release_unrun takes holder: *mut Holder, has no vm reference, and contains no addr_of_mut!. Same "new fn inserted between comment and target" class as the doc-misattachment batches already flagged in this PR (BundleThread.rs, jsc_hooks.rs, VirtualMachine.rs), at a distinct site not covered by any of them. Comment-only; delete :330-332.
Extended reasoning...
What was left behind
The diff to node_fs_stat_watcher.rs inserted a nested fn release_unrun between a pre-existing // SAFETY: block and the unsafe {} block that comment documented, then added a shortened replacement SAFETY comment above the unsafe {}. The result at HEAD (node_fs_stat_watcher.rs:330-341):
// SAFETY: `holder_ptr` was just `heap::alloc`'d and is exclusively owned here
// until `update_timer` reclaims it; `vm` is the live per-thread VM (JSC_BORROW).
// `addr_of_mut!` so the field pointer inherits whole-Box provenance.
// Reclaimed unrun by the terminate drain: the box owns no refs (the
// scheduler is kept alive by the StatWatcher's RefPtr) — plain free.
fn release_unrun(holder: *mut Holder) {
// SAFETY: queue-owned box popped by the drain; sole owner.
drop(unsafe { bun_core::heap::take(holder) });
}
// SAFETY: `holder_ptr` was just `heap::alloc`'d and is exclusively
// owned here until `update_timer` (or the drain) reclaims it.
unsafe {Lines 330-332 (the first // SAFETY: block) are now orphaned. They read as part of release_unrun's leading comment block (Rust groups adjacent line comments visually), but describe three things that do not exist in release_unrun: holder_ptr (release_unrun's parameter is holder: *mut Holder), vm (release_unrun never touches a VM reference), and addr_of_mut! (release_unrun's body is a single heap::take).
Why nothing else explains it
The diff itself shows the mechanism plainly: the pre-diff hunk had // SAFETY: holder_ptr … vm … addr_of_mut! immediately above unsafe { (*holder_ptr).task = AnyTask { … }; … }. The PR added release_unrun at :333-338 and a new // SAFETY: at :339-340, but did not delete the old comment above the insertion point. Lines 339-340 are a shortened rewrite of :330-331 ("holder_ptr was just heap::alloc'd and is exclusively owned here until update_timer (or the drain) reclaims it") — the vm/addr_of_mut! justifications were dropped even though the unsafe {} block at :341-350 still dereferences (*this).vm and uses core::ptr::addr_of_mut! at :348, but the duplication of the holder_ptr clause at both :330 and :339 is what proves :330-332 was not intentionally kept as extra documentation.
The PR description's Rebase notes claim "stale doc comments in event_loop.rs / node_fs_stat_watcher.rs updated" — this one was missed in that pass.
Why it's a distinct site
This PR's review timeline already carries doc-misattachment findings of the "new fn inserted between comment and target" class at BundleThread.rs, jsc_hooks.rs, VirtualMachine.rs, simple_request.rs, client.rs, Blob.rs, and event_loop.rs. This site — node_fs_stat_watcher.rs:330-332 — is not enumerated in any of those threads: the two prior comments on this file are on line 419 (the _vm_pin/restat cleanup leak) and the earlier (now-resolved) InitialStatTask unpinned finding, neither of which touches set_timer's SAFETY block.
Step-by-step proof
- node_fs_stat_watcher.rs:330-332 —
// SAFETY: holder_ptr was just heap::alloc'd … vm is the live per-thread VM (JSC_BORROW). addr_of_mut! so the field pointer inherits whole-Box provenance. - node_fs_stat_watcher.rs:333-338 —
// Reclaimed unrun by the terminate drain: … fn release_unrun(holder: *mut Holder) { … heap::take(holder) … }. Noholder_ptridentifier, novmaccess, noaddr_of_mut!. Lines 330-332 describe none of it. - node_fs_stat_watcher.rs:339-340 —
// SAFETY: holder_ptr was just heap::alloc'd and is exclusively owned here until update_timer (or the drain) reclaims it.— a rewrite of :330-331 with "or the drain" added, placed at the correct location aboveunsafe {at :341. - The diff confirms the ordering: the removed hunk had
// SAFETY: … addr_of_mut! …directly aboveunsafe { (*holder_ptr).task = AnyTask { ctx: …, callback: update_timer } }; the added hunk insertsrelease_unrunand the new SAFETY between them but keeps the old three lines as context.
Impact and fix
Comment-only — no runtime effect. A reader following release_unrun's comment block sees a SAFETY justification citing invariants that don't apply to the function beneath it, which REVIEW.md's comment guidance ("comments carry only durable non-obvious content") flags as noise. Fix: delete lines 330-332. Optionally, restore the dropped vm/addr_of_mut! clauses to :339-340 since the unsafe {} block at :341-350 still relies on both, but the orphan itself is the finding here.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
There is too much unsafe and type system workarounds in this PR. Can you make this more idiomatic Rust?
Read-direction coverage for the same pinArrayBuffer ref fix. The kernel's copy_to_user into the destination is invisible to ASAN, so the oracle is a direct address probe: the worker reports ptr(buf) before its fs.read/readv parks on an empty FIFO, the parent terminates it, and bun:ffi read.u8 at that address either sees the worker's fill byte (storage alive) or trips ASAN heap-use-after-free via GCIncomingRefCountedSet::lastChanceToFinalize (storage freed mid-read). Linux+ASAN only; the FIFO is never written so the pool thread stays in read(2) and the separate #34154 completion crash is not reached.
|
Connecting this PR to production crash telemetry: this fix covers what is currently the largest native crash family in Sentry for the 1.4.0 canaries. While investigating BUN-3PWA (
Roughly 700 events across the family, ongoing daily. Verification done today:
Landing this closes the whole family. |
3 participants
What
worker.terminate()freed the worker'sVirtualMachinewhile work-pool and HTTP-thread completions still held raw pointers into it. The lateenqueue_task_concurrentinto the freed event loop corrupted whatever reused the allocation. Three observable faces depending on timing:plus
Option::unwrap()panic inAsyncFSTask::run_from_js_thread(promise Strong read after the slot emptied), and JSCStructureID::decode/Heap m_collectionScopeasserts when the freed slab is reused.Design
VirtualMachineowns anArc<ShutdownGate>;vm.pin()returns an RAIIGateGuest. Every async producer takes a pin at creation on the JS thread and drops it on the completing thread after the completion enqueue. Covered producers:node:fsasync ops (including recursivecp/readdir), fetch, S3 (simple/streaming/multipart), dns, zlib/brotli/zstd writes,Bun.password, the genericAnyTaskJob(pbkdf2, scrypt,Bun.secrets, …), napi async work, Archive,Bun.build, stat-watcher (periodic + initial), runtime transpiler.TerminateAbortRegistry(fetch + every S3 request type) is walked first so pins drop promptly;close_and_wait()blocks until all pins drop; queued completions are reclaimed per-tag with JSC alive; then JSC teardown and box free. Main-VM exit closes without waiting (its box is never freed).AsyncFSTask::run_from_js_threadbails early when the promise Strong is empty or the VM is stopping, so the drain-race path cleans up instead of panicking the process.Known residuals (pre-existing, same race class, not addressed here)
Bun.$shell tasks, and napiThreadSafeFunctionstill enqueue cross-thread without a pin.AsyncCpTask/TranspilerJob/RequestContextS3 ctx is not wired; these leak a bounded box per in-flight op at terminate.Tests
test/js/web/workers/worker-terminate-lifetime.test.ts: fetch + 6 work-pool producer families (password, zlib, fs stat, fs readFile/writeFile/copyFile, dns, crypto), each terminating workers mid-op. On an unfixed debug+ASAN build:Bun.password,node:crypto, andnode:dnsabort with the heap-use-after-free above; with the fix, 11/11 pass.Raw repro (release build, no ASAN):
Segfaults at address
0x8after round 1 on a release build; completes all 12 rounds with the fix.rust:check-all: 10/10 target combos OK.Rebase notes (e9c14e1)
Rebased onto 47597ab as a single commit. Review findings from the previous round addressed:
InitialStatTasknow pinned;FetchTasklet::release_at_shutdownpin-drop ordering fixed (pin local, drop last);any_task_jobusesscript_execution_status()(the priorhas_termination_requestaccessor was removed in #35002); stale doc comments inevent_loop.rs/node_fs_stat_watcher.rsupdated; duplicate#[allow]insimple_request.rsremoved.generate_from_javascript/listen_callback/SourceType/bun_threading::Oncereintroductions dropped (dead on current main). Added aleaksan.suppentry for the pre-existing per-workernode_fs_binding::Bindingleak at terminate (fails on main independent of this diff; tracked separately).Supersedes #32071.