worker: fence in-flight AnyTaskJob work against terminate() freeing the VM - #36817
worker: fence in-flight AnyTaskJob work against terminate() freeing the VM#36817robobun wants to merge 3 commits into
Conversation
…he VM worker.terminate() while an async crypto.pbkdf2()/scrypt() (or any other AnyTaskJob: HKDF, prime/keypair gen, sign, DH, Bun.secrets, Bun.zstd*) is running on the shared work pool freed the worker VM and its JSC heap out from under the pool thread. AnyTaskJob::run_task both (a) handed the caller's ArrayBuffer-backed password/salt to BoringSSL (freed by Heap::lastChanceToFinalize, which ignores protect()) and (b) dereferenced the VM box to post the completion. Release builds segfaulted the whole process; ASAN reported the UAF on a Bun Pool thread inside PKCS5_PBKDF2_HMAC / EVP_PBE_scrypt or VirtualMachine::event_loop_shared. Add a per-VM Arc<RwLock<bool>> gate. AnyTaskJob::create clones it; run_task runs ctx.run() and the enqueue_task_concurrent push under a read lock. WebWorker::shutdown (and global_exit for the destruct-on-exit path) close the gate before the concurrent-queue drain, 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 after close() observes it and skips its body entirely. Matches Node's behaviour of draining in-flight libuv work on env teardown. Skipped jobs intentionally leak their ctx (it holds Strong/JSPromiseStrong handles into the dead VM's heap, and poll.unref would touch the freed event loop); bounded per terminated worker, same fate as tasks already queued on a terminated worker's never-drained concurrent queue. The gate Arc itself is released on both paths so it does not accumulate.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughChangesAnyTask shutdown fencing
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
Also worth cross-linking (not fixed here): #33936 asks for exactly this fence shape — "have 🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 3:31 AM PT - Aug 3rd, 2026
✅ @robobun, your commit 50704e49553ad4607f2d1f3952c97898f322e592 passed in 🧪 To try this PR locally: bunx bun-pr 36817That installs a local version of the PR into your bun-36817 --bun |
Match the ready() pattern the sibling tests use so a worker startup failure surfaces the actual error instead of an opaque exitCode 13.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found; the earlier readiness-wait nit was addressed in a66675a. Deferring to human review — this is cross-thread memory-safety code in the VM/worker-shutdown path with design trade-offs a maintainer should sign off on (new RwLock fence in the shutdown ordering, terminate() now blocks on the longest in-flight KDF, intentional ctx/poll leak on the closed-gate path with LSan opt-out in the test). Four comment-cop flags on the new doc comments are also still open.
What was reviewed:
- Gate ordering:
close()sits aftermarkTerminating/markShuttingDownand before the concurrent-queue drain /teardownJSCVMin bothWebWorker::shutdownandglobal_exit, so an in-flight reader's enqueue is visible to the drain. run_taskclones theArcbefore taking the read lock, so the gate outlives the&mut *thisreborrow inside the closure; on the closed path onlygateisdrop_in_place'd and the box is leaked (no double-drop).any_task_gatefield:Optionforalloc_zeroedvalidity, written ininit(),take()'d indestroy();any_task_gate()accessor is only reached between the two.
Extended reasoning...
Overview
Adds a per-VM Arc<RwLock<bool>> fence (AnyTaskGate) so pool-thread AnyTaskJob bodies (node:crypto KDFs/HKDF/primes/keypairs/sign/DH, Bun.secrets, Bun.zstd*) either complete entirely before worker.terminate() frees the VM/JSC heap, or observe the closed gate and skip. Touches VirtualMachine (new field + init/destroy + global_exit), any_task_job.rs (gate type + run_task rewrite), web_worker.rs (shutdown() fence), and adds a subprocess regression test.
Security risks
None user-facing. The change hardens against a process-wide crash/UAF; it does not touch auth, input parsing, or trust boundaries. The only new attack-surface consideration is that terminate() now waits on in-flight work-pool jobs, so a worker that armed a very expensive scrypt could delay its own teardown — but that's the worker's own code, and matches Node's libuv-drain behaviour on env teardown.
Level of scrutiny
High. This is exactly the category REVIEW.md calls out as most-blocked: cross-thread lifetime, new synchronization primitive, ordering against JSC heap teardown, an intentional bounded leak, and a behavioural change to terminate() latency. It's also one slice of a tracked multi-producer effort (#32073 / #35767 / #34154); the fence shape here (Arc<RwLock> vs. the generational VmHandle token proposed in #32073) is a design choice the other producers will inherit if they reuse AnyTaskGate. A maintainer should confirm they're happy with that direction.
Other factors
- My prior inline nit (readiness promise not wired to error/exit) was applied in a66675a and is resolved.
- Four comment-cop bot flags remain unresolved on the new multi-line doc/SAFETY comments — they read to me as legitimate cross-thread invariant documentation rather than workaround justification, but a human should make that call.
- The test opts the subprocess out of LSan (
detect_leaks=0) to accommodate the by-design leak of stranded jobs; that's consistent with the PR's stated rationale but is the kind of safety-net opt-out REVIEW.md asks reviewers to scrutinise. - Verification in the PR description is thorough (fails-before on both system bun and unpatched debug build, passes-after, sibling crypto/zstd suites green,
rust:check-allclean).
There was a problem hiding this comment.
Thanks for wiring error/exit into the readiness promise (a66675a). No new issues found, but this is cross-thread lifetime fencing on the VM shutdown path with an intentional-leak tradeoff and competing designs in flight (#32073 / #35767), so it warrants a maintainer look.
What was reviewed:
run_gatedread-lock ordering vsclose()write-lock — the enqueue is inside the guard, so it's visible to the post-close()drain in bothWebWorker::shutdownandglobal_exit.- Gate-closed path:
drop_in_placeon(*this).gatereleases the job's Arc clone; the outergatelocal's Arc is dropped just before, so no double-drop and no Arc leak. any_task_gateinit/take covers all VM lifecycle exits (initwrites it,global_exitcloses it,destroytakes it; worker path closes inshutdownbefore the concurrent-queue drain).
Extended reasoning...
Overview
Adds a per-VM Arc<RwLock<bool>> gate (AnyTaskGate) so work-pool AnyTaskJob bodies and their completion enqueue run under a read lock, and worker/process shutdown takes the write lock (waiting out in-flight readers) before JSC teardown and the VM dealloc. Touches VirtualMachine init/destroy/global_exit, any_task_job.rs (new gate type + run_task rewrite), a one-line close() call in WebWorker::shutdown, and a new subprocess regression test.
Security risks
None in the injection/auth sense. This is memory-safety code: the risk surface is the cross-thread ordering itself — a mis-placed close() (after the drain instead of before) or a read-lock scope that doesn't cover the enqueue would leave the original UAF window open. Both look correct: close() precedes release_queued_tasks_for_shutdown in both call sites, and enqueue_task_concurrent is inside run_gated's closure.
Level of scrutiny
High. This is exactly the category REVIEW.md calls out as most-blocked: cross-thread lifetime, refcounts, intentional leaks with SAFETY annotations, and new unsafe raw-pointer manipulation in run_task. The intentional leak of ctx/poll on the gate-closed path is a design tradeoff (bounded per terminated worker, same fate as the never-drained concurrent queue) that a maintainer should sign off on — the PR description itself frames this as one approach among several (#35154 rebase; #35767/#34154 track the sibling producers; #32073 proposes a generation-token alternative).
Other factors
- My prior feedback (test readiness-wait error/exit wiring) was applied in a66675a.
- The comment-cop bot's paragraph-length flags were addressed in 50704e4; remaining comments are doc/SAFETY on a sync primitive, which the file's existing style matches.
close()blocking on the longest in-flight KDF meansterminate()latency is now bounded by user-controlled crypto params — the PR notes this matches Node's libuv-drain behavior, but it's a user-observable change worth a maintainer nod.- CI build #88084 was still running at last timeline update; no green signal yet.
- The test disables LSan for the subprocess (documented, matches the intentional-leak design), and uses the file-local
rounds/timeoutscaling.
|
Closing as superseded by #37075 (9d519e8), which replaced Verified on a debug + ASAN build of current main (f426a8e): the test from this PR (Buffer-backed pbkdf2/scrypt inputs) and the one from #35154 both pass, two runs each, with no sanitizer output. Main also carries direct coverage for pbkdf2/scrypt jobs being refused after |
2 participants
Problem
worker.terminate()while an asynccrypto.pbkdf2()/crypto.scrypt()(or any otherAnyTaskJob: HKDF, prime/keypair gen, sign, DH,Bun.secrets,Bun.zstd*) is running on the shared work pool frees the worker VM and its JSC heap out from under the pool thread.AnyTaskJob::run_taskbothHeap::lastChanceToFinalizefrees regardless ofprotect(), andvm.event_loop_shared()on the rawBackRef<VirtualMachine>).Release builds segfault the whole process; debug+ASAN reports the UAF on a "Bun Pool" thread:
The ArrayBuffer-read face shows as
PKCS5_PBKDF2_HMAC/EVP_PBE_scryptreading freed bytes inside BoringSSL, but only when the salt length is not a multiple of the SHA-256 block size: the block function is uninstrumented assembly, so ASAN only observes the <64-byte memcpy tail. Workerprocess.exit()and an uncaught throw take the same shutdown path.Fix
A per-VM
Arc<RwLock<bool>>gate.AnyTaskJob::createclones it;run_taskrunsctx.run()and theenqueue_task_concurrentpush under a read lock.WebWorker::shutdown(andglobal_exitfor the destruct-on-exit path) close the gate right next to the existingmarkTerminating/markShuttingDownfences, before the concurrent-queue drain, 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.Skipped jobs intentionally leak their
ctx(it holdsStrong/JSPromiseStronghandles into the dead VM's heap, andpoll.unrefwould touch the freed event loop); bounded per terminated worker, same fate as tasks already queued on a terminated worker's never-drained concurrent queue. The gateArcitself is released on both paths so it does not accumulate.close()blocking on the longest in-flight KDF matches Node's behaviour of draining in-flight libuv work on env teardown.Scope
This is #35154 rebased onto current main with a Buffer-input test for the ArrayBuffer-read face. The other cross-thread producers (
FetchTasklet,WorkTask,ConcurrentPromiseTask,AsyncFSTask,PasswordJob, napi, watchers) still have the analogous defect and stay with #35767 / #34154; theAnyTaskGatehere is reusable by those call sites.Verification
New test in
test/js/web/workers/worker-terminate-lifetime.test.ts: a subprocess arms 2x pbkdf2 + 2x scrypt with a 4 MiB Buffer salt in each ofroundsworkers and terminates once armed.USE_SYSTEM_BUN=1: child segfaults at0xFFFFFFFF00000000(rc 139, round 6-7)bun bdwithout the src/ change: ASAN heap-use-after-free atAnyTaskJob<Pbkdf2Ctx>::run_task(round 0)bun bdwith the fix: 4/4 rounds clean, ~10s under ASANtest/js/node/crypto/{pbkdf2,scrypt}.test.ts,node-crypto.test.js(203),zstd.test.ts(83) all passbun run rust:check-all: 10/10 targets okThe pre-existing
dns.lookup()test in the same file fails on current main (4104-byte leak) with or without this change.no test proof · iteration 0 · 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
Towards #32073