Repository navigation
Conversation
AnyTaskJob::run_task (the work-pool callback for async node:crypto jobs, Bun.secrets, and Bun.zstd*) held a raw backref to the scheduling VirtualMachine and dereferenced it on the pool thread to read vm.global and vm.event_loop_shared() when posting its completion. When that VM was a worker that terminate() freed (WebWorker::shutdown dealloc's the VM storage) while a long scrypt/pbkdf2 was still running in BoringSSL, both reads were a cross-thread heap-use-after-free. Release builds segfaulted; ASAN reported the UAF on a Bun Pool thread at event_loop_shared. Fix: a per-VM Arc'd RwLock<bool> gate that each AnyTaskJob clones at create time, plus a snapshot of global/event_loop so run_task never reads a VM field. WebWorker::shutdown closes the gate (write lock) right next to the existing ScriptExecutionContext/JSCTaskScheduler fences, before draining the concurrent queue and freeing the VM: the write lock waits out any in-flight enqueue reader so its push is visible to the drain, and completions landing after close() observe it and drop their task instead of touching freed memory. Rejected jobs leak their ctx (it holds Strong JSC handles into the dead VM's heap), matching the fate of tasks already queued on a terminated worker's never-drained queue.
|
Warning Review limit reached
Next review available in: 14 minutes 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 (4)
Comment |
|
Updated 12:53 PM PT - Jul 22nd, 2026
❌ @robobun, your commit 7b7b146 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 35154That installs a local version of the PR into your bun-35154 --bun |
|
Reproduced with the script in the test (6 scrypt/pbkdf2 lanes in a worker, then 12f4032 extends the gate to cover CI (build 77982): the new test is green on every lane including aarch64 release and x64-asan. Remaining red is unrelated to this diff:
Ready for review. |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re the duplicate flag: #32071 is already called out in the PR body. #34154 is the broader VM-pin redesign (47 files, currently merge-conflicting) that covers the full producer class including |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this is cross-thread VM-lifetime / memory-safety code (RwLock fence between work-pool completions and worker VM teardown, with an intentional-leak path) and one slice of the broader #32071 design — worth a human sign-off.
What was reviewed:
- Fence ordering:
close()sits aftermarkTerminating/markShuttingDownand before the concurrent-queue drain / step-5vm.destroy();has_terminatedis set only indestroy(), so the debug assertion inenqueue_task_concurrentcannot fire under the read lock. run_taskno longer reads any VM field on the pool thread;ctx.run(global)still receives a raw pointer that may dangle post-terminate, matching the existing trait contract that impls never dereference it.- Rejected-path cleanup: the heap
ConcurrentTasknode is freed and the gateArcisdrop_in_place'd; the documentedctx/pollleak mirrors tasks stranded on a terminated worker's undrained queue. any_task_gateinit viaaddr_of_mut!().write(zero-validOptionslot) andtake()indestroy(); main-VMglobal_exitnever closes the gate, which is the pre-existing scope gap deferred to #32071.
Extended reasoning...
Overview
Adds a per-VM Arc<RwLock<bool>> gate (AnyTaskGate) so AnyTaskJob work-pool completions can detect that their owning worker VM was freed by terminate() and drop instead of pushing into a freed EventLoop. Touches VirtualMachine (new field + init/destroy wiring + accessor), any_task_job.rs (gate type, snapshot of global/event_loop at create time, rewritten run_task), web_worker.rs (single close() call in shutdown), and a new regression test.
Security risks
None user-facing. This is a UAF fix; the closed-gate path intentionally leaks the job's ctx (Strong JSC handles into a dead heap) and KeepAlive, which is a bounded per-terminate leak equivalent to the existing fate of tasks stranded on a terminated worker's concurrent queue.
Level of scrutiny
High. Cross-thread lifetime + lock-ordering reasoning around VM teardown is exactly the category REVIEW.md calls out as most-blocked. The RwLock-as-fence pattern is sound as written (write lock in close() waits out in-flight readers so their push is visible to the subsequent drain; later readers observe closed), and it mirrors the adjacent ScriptExecutionContext::markTerminating shape. But the choice of an Arc<RwLock> gate here vs. #32071's VM-handle registry, and the intentional-leak decision, are design calls a maintainer should ratify.
Other factors
enqueue_task_concurrenttakes&selfand its debug-onlyvm_ref().has_terminatedread is ordered safe (has_terminatedis set indestroy(), which is step 5, afterclose()).bun_threading::RwLock<T>has the parking_lot-shapedread()/write()guards the code assumes.- The gate is never closed for the main VM (only
WebWorker::shutdowncallsclose());destroy()just drops the Arc. That's the pre-existing gap the PR explicitly scopes out to #32071, not a regression. - Test follows harness conventions (subprocess,
bunEnv, concurrent pipe drain, combined-object assertion, awaits the "up" message before terminate) and the PR reports fail-before/pass-after under ASAN.
Several AnyTaskJobCtx::run impls touch the owning VM's JSC heap on the pool thread: Scrypt writes its derived key into a JSC ArrayBuffer (the backing Scrypt::result borrows), and both Scrypt and PBKDF2 read their password/salt from StringOrBuffer-wrapped JS buffers. With only the enqueue fenced, a terminate() that won the race freed the JSC heap (teardownJSCVM) while a pool thread was still writing the scrypt output into it; the next worker to reuse that memory saw the corruption as a garbage ArrayAllocationProfile in llint_op_new_array (aarch64 release lanes on CI). Hold the gate's read lock across ctx.run() as well as the enqueue, so close() in WebWorker::shutdown waits out the whole in-flight work body before JSC teardown. A job the pool picks up after close() skips its body entirely. With the VM proven live under the lock, run_task can read vm.global / vm.event_loop_shared() directly again, so the snapshot fields are dropped.
…out of LSan VirtualMachine::global_exit() under BUN_DESTRUCT_VM_ON_EXIT mirrors WebWorker::shutdown()'s fence-then-drain sequence and reaches Zig__GlobalObject__destructOnExit (frees the JSC heap) and destroy(), so the same AnyTask gate fence belongs here too; without it an async scrypt running at process.exit() would write its output into a freed ArrayBuffer. Placed right after JSCTaskScheduler__markShuttingDown, matching the worker path. The new test's subprocess is opted out of LSan: jobs that race into concurrent_tasks before close() end up requeued into EventLoop.tasks at deinit, whose buffer becomes unreachable when the worker VM is raw-dealloc'd. The leak is bounded per terminated worker and is the same accounting as tasks already stranded on a terminated worker's queue.
There was a problem hiding this comment.
Thanks for addressing the global_exit() mirror in 7b7b146 — that closes the sibling path. I didn't find further correctness issues, but this is cross-thread VM-lifetime code with an intentional-leak trade-off and an open question of whether to land this slice vs. revive #34154, so deferring to a maintainer.
What was reviewed:
AnyTaskGateread/write-lock ordering vs. theshutdown()step sequence —close()is placed beforerelease_queued_tasks_for_shutdownandteardownJSCVM, so in-flight readers finish and their enqueue is visible to the drain.- The closed-gate leak path:
drop_in_placeon the job'sgatefield releases theArcclone;ctx/pollare leaked as documented (touching either would UAF the dead VM). Matches the fate of already-queued concurrent tasks. close()'s write lock will block workershutdown()(and thusterminate()resolution) until every in-flight KDF finishes — for the test'sN=1<<14scrypt that's tens of ms, but user-controllable params could stretch it. Worth a maintainer eye on whether that latency is acceptable vs. Node's drain semantics.
Extended reasoning...
Overview
Adds a per-VM Arc<RwLock<bool>> fence (AnyTaskGate) so work-pool AnyTaskJob bodies (ctx.run() + concurrent-queue enqueue) either complete entirely before the owning VM is torn down, or are skipped. Touches src/jsc/any_task_job.rs (new gate type + run_task rework), src/jsc/VirtualMachine.rs (new field + accessor, global_exit() close, destroy() cleanup), src/jsc/web_worker.rs (one close() call in shutdown()), and a new regression test.
Security risks
None user-facing; this is an internal lifetime fix. The change removes a heap-use-after-free (pool thread writing scrypt output into a freed JSC ArrayBuffer / reading a freed EventLoop).
Level of scrutiny
High. Per REVIEW.md this sits squarely in "Native code: memory safety (the most-blocked category)" — cross-thread refcounts, RwLock ordering against a documented multi-step teardown sequence, an intentional leak on the closed-gate path with an LSan opt-out in the test, and unsafe raw-pointer field access on the pool thread. The placement of close() relative to markTerminating/markShuttingDown/release_queued_tasks_for_shutdown/teardownJSCVM is load-bearing and mirrors two existing fences, but the write-lock-blocks-until-KDF-finishes behaviour is a new latency characteristic for terminate() that a human should sign off on.
Other factors
- My earlier finding (missing
close()in theglobal_exit()mirror underBUN_DESTRUCT_VM_ON_EXIT) was addressed in 7b7b146 and the inline thread is resolved. - The author explicitly flags this as competing with #34154 (broader 47-file redesign) and offers to close if that PR is revived — a maintainer needs to make that call.
- Test has fail-before/fail-after evidence and covers both scrypt and pbkdf2; LSan opt-out is documented and bounded per terminated worker.
bun_threading::RwLockis the in-tree primitive (notstd::sync), matching repo convention.
|
Generalised into #35767 using the enqueue-by-identifier route (same serialization point as |
|
test/js/node/test/parallel/test-crypto-prime.js went red on main in builds 84904 (alpine x64, segfault) and 85363 (debian x64-asan, heap-use-after-free) with the same The test's worker block (line 313) calls |
|
Rebased onto current main as #36817 (this one had gone conflicting) with a Buffer-input test that exercises the |
|
Closing as superseded by #37075 (9d519e8), which replaced Verified on a debug + ASAN build of current main (f426a8e): the test from this PR and the one from #36817 both pass, two runs each, with no sanitizer output. Main also carries direct coverage for pbkdf2/scrypt jobs being refused after |
2 participants
What
worker.terminate()while an asynccrypto.scrypt()/crypto.pbkdf2()(or any otherAnyTaskJob: HKDF, prime gen, key-pair gen, sign, DH,Bun.secrets,Bun.zstd*) is running on the work pool freed the worker'sVirtualMachineand its JSC heap while the pool thread was still touching both:AnyTaskJob::run_taskreadvm.globalandvm.event_loop_shared()off a raw backref when posting its completionctx.run()itself reads inputs from JSC-backedArrayBuffers (StringOrBufferpassword/salt) and, forScrypt, writes its derived key into a JSCArrayBufferallocated ininit()WebWorker::shutdownfrees the JSC heap (teardownJSCVM) and then the VM storage, so a pool thread still inside BoringSSL's scrypt wrote its output into freed memory. The next worker to reuse that allocation saw the corruption as a garbageArrayAllocationProfileinllint_op_new_array(aarch64 release lanes), or the completion enqueue read the freedEventLoopdirectly:Fix
A per-VM
Arc<RwLock<bool>>gate.AnyTaskJob::createclones it;run_taskrunsctx.run()and theenqueue_task_concurrentpush under a read lock.WebWorker::shutdowncloses the gate (write lock) right next to the existingScriptExecutionContext::markTerminating/JSCTaskScheduler::markShuttingDownfences, before draining the concurrent queue, before JSC teardown, and before deallocating the VM. The write lock waits out every in-flight reader so the work body finishes and its push is visible to the drain; a job the pool picks up afterclose()observes it and skips its body entirely (matching Node's behaviour of draining in-flight libuv work on env teardown).Skipped jobs intentionally leak their
ctx(it holdsStrongJSC handles into the dead VM's heap, andpoll.unrefwould touch the freed event loop). This matches the fate of tasks already queued on a terminated worker's never-drained concurrent queue. The gateArcitself is released on both paths so it does not accumulate.Scope
This is the focused
AnyTaskJobslice of #32071 / #34154 (which cover the full cross-thread producer class). The other producers (FetchTasklet,WorkTask,ConcurrentPromiseTask,AsyncFSTask, napi, watchers) still have the analogous defect and stay with those PRs; this change keeps the diff small so the crypto segfault can land independently. TheAnyTaskGateintroduced here is reusable by those call sites if the wider change is further delayed.Verification
New test in
test/js/web/workers/worker-terminate-lifetime.test.ts:USE_SYSTEM_BUN=1): segfault at0xFFFFFFFF00000000, "multiple threads are crashing"bun bd, ASAN): passesExisting
test/js/node/crypto/{pbkdf2,scrypt,hkdf-callback-null}.test.tsandnode-crypto.test.js(202 tests, covers theextern_crypto_job!set) all pass.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/workers/worker-terminate-lifetime.test.ts