webcrypto: fence ConcurrentCppTask's post-body unref against worker.terminate() freeing the VM - #35758
webcrypto: fence ConcurrentCppTask's post-body unref against worker.terminate() freeing the VM#35758robobun wants to merge 4 commits into
Conversation
…erminate() freeing the VM ConcurrentCppTask wraps every async crypto.subtle op that runs on the work pool (deriveBits/sign/digest/etc. via PhonyWorkQueue::dispatch). After the C++ body ran, run_owned dereferenced the raw bunVM pointer captured by EventLoopTaskNoContext to call event_loop_shared().unref_concurrently(). When the creating VM was a worker freed by terminate() while the crypto op was still running, that read the freed VirtualMachine allocation (ASAN heap-use-after-free in event_loop_shared; stock bun SIGSEGV). The body itself was already safe: it captures the ScriptExecutionContext identifier and posts the result via postTaskTo(), which serializes with markTerminating() on the contexts-map lock. Only the trailing unref was unfenced. EventLoopTaskNoContext now also captures the context identifier, and the pool-thread unref goes through a new ScriptExecutionContext__unrefEventLoopConcurrently(id) that resolves the context under the same map lock and no-ops if it is gone or terminating. WebWorker::shutdown already calls markTerminating() (under that lock) before any VM state is freed, so the unref either completes before shutdown begins or is dropped.
|
Warning Review limit reached
Next review available in: 7 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 (2)
WalkthroughChangesThe PR adds context-identifier FFI plumbing, replaces VM-based concurrent unref with termination-aware context lookup, and adds an ASAN regression test covering PBKDF2 tasks during worker termination. Concurrent task cleanup
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:14 PM PT - Jul 25th, 2026
❌ @robobun, your commit 814978a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35758That installs a local version of the PR into your bun-35758 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs. The fence mirrors postTaskTo()'s existing contexts-map-lock pattern and WebWorker::shutdown does call markTerminating() (web_worker.rs:1299) before any VM state is freed, so the either-unref-under-lock-or-drop argument holds. That said, this is a cross-thread lifetime fix in the worker VM teardown path, so it's worth a human look.
What was reviewed:
- Confirmed
markTerminating()takesallScriptExecutionContextsMapLockand is called before the VM box is freed, so!isTerminating()under the lock ⇒context->vm()/clientData->bunVMare live. - Checked
unref_concurrently()is an atomicfetch_sub+wakeup();postTaskTo()already callspostTaskConcurrentlyunder this same lock, so no new lock ordering is introduced. context_identifier()is read beforeEventLoopTaskNoContext::rundeletes the task; identifiers are monotonic (++lastUniqueIdentifier), so no ABA on a freed worker's id.
Extended reasoning...
Overview
Fixes a cross-thread heap-use-after-free: ConcurrentCppTask::run_owned (the work-pool wrapper for async crypto.subtle ops) previously dereferenced the raw bunVM pointer captured by EventLoopTaskNoContext to call unref_concurrently() after the crypto body finished on a pool thread. If the creating VM was a worker freed by terminate() while the op ran, that read freed memory. The fix captures the ScriptExecutionContextIdentifier at task construction and routes the pool-thread unref through a new ScriptExecutionContext__unrefEventLoopConcurrently(id) that resolves the context under allScriptExecutionContextsMapLock and no-ops if gone/terminating — the same fence postTaskTo() already uses. Touches src/jsc/CppTask.rs, EventLoopTaskNoContext.{h,cpp}, ScriptExecutionContext.cpp, plus an ASAN-gated regression test.
Security risks
None user-facing. This is a memory-safety hardening: it removes a UAF reachable from worker.terminate() racing in-flight WebCrypto. No new attack surface.
Level of scrutiny
High. Cross-thread lifetime reasoning around worker VM teardown is exactly the "most-blocked category" in this repo, and the correctness argument depends on lock ordering (markTerminating() under the contexts-map lock happening strictly before the VM box is freed in WebWorker::shutdown). I verified that ordering holds (web_worker.rs:1299 sits well before the VM dealloc), that the unref runs entirely under the lock so context->vm() → clientData->bunVM → event_loop_shared() are all live when !isTerminating(), and that no new lock inversion is introduced (postTaskTo() already calls postTaskConcurrently/wakeup under this lock). But this class of fix has enough subtlety — and enough sibling PRs (#35154/#35155/#35156/#35158) — that a maintainer who owns the worker teardown sequence should sign off.
Other factors
The schedule-time ref_concurrently() staying direct is justified (called on the creating VM's JS thread from PhonyWorkQueue::dispatch). The context_id is read before run deletes the C++ task. Identifiers come from an atomic counter and are never recycled, so a freed worker's id can't be reused by a new context. The test is ASAN-gated and follows the file's existing conventions (rounds, timeout, combined {stdout, stderr, exitCode} assertion), though the Bun.sleep(60 + ...) is timing-based by necessity for a race repro. The PR description notes the test wasn't run locally ("Platform-specific test(s) that do not run on this machine"), so CI is the actual proof.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I checked the FFI ABI (ScriptExecutionContextIdentifier is uint32_t, matching the Rust-side u32), and confirmed Bun__eventLoop__incrementRefConcurrently is a pure atomic counter op — so holding allScriptExecutionContextsMapLock across it can't deadlock against the worker thread blocking in markTerminating(). The fence itself looks right: the lock is held through the VM deref, and markTerminating() takes the same lock before shutdown frees anything.
Extended reasoning...
This is a cross-thread memory-safety fix in the JSC/worker teardown path — the highest-scrutiny category per REVIEW.md — so deferring to a human regardless. The one inline finding is a style nit only. I additionally verified: (1) the C++ ScriptExecutionContextIdentifier is using ... = uint32_t so the Rust u32 extern signature is ABI-correct; (2) Bun__eventLoop__incrementRefConcurrently (JSCScheduler.rs:40) just reads event_loop_shared() and does an atomic inc/dec, so calling it under allScriptExecutionContextsMapLock cannot deadlock with the worker thread waiting on the same lock in markTerminating(); (3) the lock is held for the entire body of ScriptExecutionContext__unrefEventLoopConcurrently, so the VM cannot be freed between the isTerminating() check and the deref. Also noting the comment-cop bot has flagged several of the new comments for length — the author will likely want to trim those.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/js/web/workers/worker-terminate-lifetime.test.ts`:
- Around line 146-157: Update the worker source and parent synchronization
around the readiness message so each lane completes key setup and submits its
long-running deriveBits operation before posting "up"; replace the fixed
Bun.sleep delay with a readiness promise that rejects on worker error or
premature exit, while preserving termination after all crypto work has been
submitted.
🪄 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: 93eb038d-36a1-42c0-9f19-6cb0d103c569
📒 Files selected for processing (5)
src/jsc/CppTask.rssrc/jsc/bindings/EventLoopTaskNoContext.cppsrc/jsc/bindings/EventLoopTaskNoContext.hsrc/jsc/bindings/ScriptExecutionContext.cpptest/js/web/workers/worker-terminate-lifetime.test.ts
|
Overlaps with #35767, which applies the same |
There was a problem hiding this comment.
No issues found, but this is cross-thread VM-lifetime code (worker teardown × pool-thread completion) and overlaps with #35767's broader fence — worth a human sign-off on landing order and the lock-held unrefEventLoop() path.
What was reviewed:
- Verified
ScriptExecutionContextIdentifierisuint32_t, matching the Rustu32FFI signature. - Traced
ScriptExecutionContext__markTerminatinginweb_worker.rs:1299— runs beforeteardownJSCVMand the VMdealloc, so the map-lock fence holds. - Confirmed
context_identifier()is read beforerun()deletes theEventLoopTaskNoContext. - Prior nit (use
context->unrefEventLoop()helper) and CodeRabbit's test-sync feedback were both addressed in 39b9276 / 814978a.
Extended reasoning...
Overview
Fixes a heap-use-after-free where ConcurrentCppTask::run_owned (the work-pool wrapper for async WebCrypto ops) dereferenced the raw bunVM pointer captured by EventLoopTaskNoContext to call unref_concurrently() after the crypto body ran on a pool thread. If the creating VM was a worker freed by terminate() mid-op, that read freed memory. The fix captures the ScriptExecutionContextIdentifier (a u32) instead and routes the pool-thread unref through a new ScriptExecutionContext__unrefEventLoopConcurrently(id) that resolves the context under allScriptExecutionContextsMapLock and no-ops if the context is gone or isTerminating() — the same fence postTaskTo() already uses.
Touches src/jsc/CppTask.rs, src/jsc/bindings/EventLoopTaskNoContext.{h,cpp}, src/jsc/bindings/ScriptExecutionContext.cpp, and adds an ASAN-gated regression test to test/js/web/workers/worker-terminate-lifetime.test.ts.
Security risks
None new. This closes a UAF (process crash / potential memory corruption) reachable from untrusted worker code via crypto.subtle + worker.terminate(). The fix does not add new attack surface; it narrows an existing one.
Level of scrutiny
High. This is exactly the REVIEW.md "most-blocked category": cross-thread memory safety at the JSC VM lifetime boundary. The reasoning depends on lock ordering (markTerminating() under the contexts-map lock happens-before VM teardown at web_worker.rs:1299 → teardownJSCVM → dealloc), and on unrefEventLoop() being safe to call while holding that lock (it's an atomic decrement via Bun__eventLoop__incrementRefConcurrently, same as postTaskTo's postTaskConcurrently under the same lock). I traced these and they hold, but a maintainer familiar with the sibling PRs (#35154/#35155/#35156/#35158) and the overlapping #35767 should confirm the landing order and that this slice doesn't conflict with the broader fence.
Other factors
- The fix follows the established
postTaskTo()fence pattern rather than inventing a new mechanism. - The schedule-time
ref_concurrently()inConcurrentCppTask__createAndRuncorrectly stays direct — it runs on the creating VM's JS thread. - The test is ASAN-only, follows the file's existing conventions (spawned subprocess,
{stdout, stderr, exitCode}combined assertion,rounds/timeoutscaling), and the retainedBun.sleepis commented with why no observable signal exists (pool-thread entry into the PBKDF2 body). - All prior review threads (my
unrefEventLoop()helper nit, CodeRabbit's test-sync feedback, comment-cop verbosity flags) are resolved. - The author flagged overlap with #35767; whichever lands second needs to reconcile the shared
ScriptExecutionContext.cpp/EventLoopTaskNoContext.hadditions — a human should decide that.
|
CI status across builds #81125 / #81309 / #81535: the Every lane that did run is green: on #81309, all linux-x64 / linux-aarch64 / linux-aarch64-musl / windows-aarch64 builds passed and all 71 test shards that ran passed with no test failures. The x64-asan lane (where the new test runs) was also queued behind a stuck The diff is ready; it needs the queue to drain or a manual retry once agents are available. |
|
Closing: this is fixed on current main by #37075. ConcurrentCppTask::run_owned now clones a refcounted VmHandle before running the C++ body and does the trailing unref through that handle (src/jsc/CppTask.rs), and VM teardown waits for counted off-thread work before freeing the VM, so the pool thread no longer touches a freed VirtualMachine after worker.terminate(). The ASAN test this PR adds to test/js/web/workers/worker-terminate-lifetime.test.ts (terminate() while crypto.subtle async ops are in flight) passes unmodified against an ASAN debug build of main at 04148c8, three runs in a row. |
2 participants
Problem
worker.terminate()with asynccrypto.subtleops in flight (deriveBitsPBKDF2,sign,digest, etc.) is a cross-thread heap-use-after-free that kills the whole process:Stock release bun:
Segmentation fault (core dumped), 2/3 on the repro.Cause
ConcurrentCppTaskwraps everycrypto.subtleop that runs on the work pool (viaPhonyWorkQueue::dispatch, the only caller ofConcurrentCppTask__createAndRun). After the C++ body finishes on the pool thread,run_owneddereferenced the rawbunVMpointer captured byEventLoopTaskNoContextto callevent_loop_shared().unref_concurrently(). When the creating VM was a worker freed byterminate()while the crypto op was still running, that read the freedVirtualMachineallocation.The crypto body itself was already safe:
dispatchAlgorithmOperationcaptures theScriptExecutionContextIdentifierand posts the result viapostTaskTo(), which serializes withmarkTerminating()on the contexts-map lock. Only the trailing Rust-side unref was unfenced.Fix
EventLoopTaskNoContextnow also captures the context identifier, and the pool-thread unref goes through a newScriptExecutionContext__unrefEventLoopConcurrently(id)that resolves the context under the same contexts-map lock and no-ops if it is gone or terminating.WebWorker::shutdownalready callsmarkTerminating()(under that lock) before any VM state is freed, so the unref either completes before shutdown begins or is dropped (the counter of a freed event loop needs no balancing).The schedule-time
ref_concurrently()inConcurrentCppTask__createAndRunstays direct: it runs on the creating VM's JS thread.Verification
New ASAN-gated test in
test/js/web/workers/worker-terminate-lifetime.test.ts: terminates a worker with four lanes of PBKDF2/SHA-512 200k-iterationderiveBitsin flight.heap-use-after-free ... event_loop_sharedSibling to #35154 / #35155 / #35156 / #35158 (same worker-teardown UAF class, different completion site).
no test proof · iteration 3 · 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