Carry a generation token with cross-thread VM handles - #32082
Merged
Jarred-Sumner merged 1 commit intoJun 12, 2026
Merged
Conversation
Collaborator
Author
|
Updated 6:44 PM PT - Jun 10th, 2026
❌ @robobun, your commit 6f3f989 has 7 failures in
🧪 To try this PR locally: bunx bun-pr 32082That installs a local version of the PR into your bun-32082 --bun |
The live-VM registry added for worker teardown keyed liveness by address
alone, so a new VM allocated at a dead VM's address could receive a stale
producer's completion. Stamp every registration with a process-unique
generation, hand producers a Copy VmHandle/LoopHandle { addr, generation }
captured at schedule time, and verify both under the registry lock in the
checked enqueue/ref/unref entry points.
Producers that stored *mut VirtualMachine / BackRef<EventLoop> for
completion delivery now store the handle: FetchTasklet, S3 tasks, WorkTask,
ConcurrentPromiseTask, AnyTaskJob, TranspilerJob, node:fs async tasks, zlib
native streams, napi async_work/TSFN, fs watchers, PasswordObject, Archive,
DevServer watcher events, bundler completion, and the EventLoopHandle Js
arm (waiter thread, shell tasks, bundler plugin dispatch). The
package-manager WakeHandler carries the generation next to its context
pointer.
Pointers captured by C++ (JSVMClientData::bunVM via JSCScheduler,
EventLoopTaskNoContext via CppTask) and the napi finalizer's env-derived VM
keep address-only checks through explicitly named _addr_only variants.
robobun
force-pushed
the
farm/6b65ff45/vm-handle-generation
branch
from
June 11, 2026 00:10
8373d7a to
6f3f989
Compare
Jarred-Sumner
merged commit Jun 12, 2026
0cf6132
into
farm/7117067b/worker-terminate-concurrent-queue-uaf
74 of 77 checks passed
This was referenced Jun 12, 2026
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jun 16, 2026
…ifiers eager (#32407) ## Crash Sentry BUN-2V1E: segfault inside `WTF::TypeCastTraits<JSVMClientData>::isType` reached from `Zig::GlobalObject::visitChildrenImpl` on a concurrent GC helper thread. 695 lifetime events (26 in the last 24h), 100% Windows x64, 1.2.17 through 1.3.14. 31% of events carry both `workers_spawned=True` and `workers_terminated=True` vs a ~3% baseline, pointing at worker-termination churn. Also seen intermittently in CI as the `broadcast-channel-worker-gc` flake (b03f1e6 is a rekick for it). ``` WTF::ParallelHelperPool::Thread::work JSC::Heap::runBeginPhase lambda JSC::SlotVisitor::drainFromShared JSC::SlotVisitor::drain JSC::SlotVisitor::visitChildren JSC::MethodTable::visitChildren Zig::GlobalObject::visitChildren Zig::GlobalObject::visitChildrenImpl WebCore::clientData(JSC::VM&) WTF::downcast<JSVMClientData> WTF::is<JSVMClientData> TypeCastTraits<JSVMClientData>::isType <-- SEGV ``` ## Cause `visitChildrenImpl` ran: ```cpp WebCore::clientData(thisObject->vm())->httpHeaderIdentifiers().visit<Visitor>(visitor); ``` Two problems on this line: **1. `thisObject->vm()` dereferences cell state on the marker thread.** `JSGlobalObject::vm()` returns `*m_vm` (a raw `VM* const` stored on the cell); `clientData()` then does `downcast<JSVMClientData>(vm.clientData)` whose `RELEASE_ASSERT(!source || is<Target>(*source))` calls the virtual `isWebCoreJSClientData()`. The neighbouring `visitGlobalObjectMember(unique_ptr)` overload already guards a window where the concurrent marker visits a `Zig::GlobalObject` picked up via conservative stack scan while its IsoSubspace slot is being recycled; in that same window `m_vm` can read stale bytes, resolving to a garbage `clientData` whose vtable load faults. `visitor.vm()` (= `m_heap.vm()`) is guaranteed alive for the duration of marking and does not depend on the visited cell at all; this is how JSC's own `visitChildren` implementations (`FunctionExecutable`, `JSWeakObjectRef`, `Structure`) fetch the VM on the marker thread. **2. `httpHeaderIdentifiers()` was an unlocked lazy `std::optional::emplace()`** called from both the mutator (`NodeHTTP.cpp` header assignment) and concurrent GC helper threads. With more than one `Zig::GlobalObject` in a VM (ShadowRealm, test-isolation swap, bake) distinct parallel marker helpers each visit a different global and all call `httpHeaderIdentifiers()` on the same `JSVMClientData`, so two threads can enter `emplace()` on the same storage. The `HTTPHeaderIdentifiers` constructor only runs ~90 `LazyProperty::initLater()` calls (each a single tagged-pointer store), so there is nothing worth deferring. ## Fix - `ZigGlobalObject.cpp`: fetch the VM via `visitor.vm()` instead of `thisObject->vm()`. - `BunClientData.{h,cpp}`: `m_httpHeaderIdentifiers` is now a plain eagerly-constructed member; `httpHeaderIdentifiers()` is an inline accessor. ## Verification The race window is too narrow to trip deterministically on Linux. An honest probe against the unfixed debug (ASAN) build, with `Malloc=1` + `BUN_JSC_collectContinuously=1` + `BUN_JSC_numberOfGCMarkers=8`: - 5 iterations of an 8-round × 6-worker BroadcastChannel create/terminate/GC stress: clean. - 8 iterations of a 100-round × 8-ShadowRealm (parallel-marker emplace) stress: clean. So there is no fail-before proof to hand the gate; the crash signature is Windows-specific and timing-dependent. The fix is nonetheless clearly correct on inspection: - `visitor.vm()` is the JSC convention for the marker thread and cannot read through the visited cell. - An unlocked `std::optional::emplace()` reachable from two threads is a data race in any memory model. A new stress test in `test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts` hammers the exact path (multiple globals per VM via ShadowRealm, worker churn, forced parallel markers, `Malloc=1` on non-Windows) so a future regression on Windows CI will show up where the signature has already been observed. ``` bun bd test test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts # 4 pass bun bd test test/js/node/http/node-http.test.ts -t headers # 5 pass (HTTPHeaderIdentifiers path) bun bd test test/js/node/http/numeric-header.test.ts # 1 pass ``` ## Related Checked #31990 / #32071 / #32082 (worker event-loop enqueue after terminate, Strong<> releases before VM teardown): none touch `visitChildrenImpl` or `vm.clientData` access from the marker thread. No open PR addresses this crash. The issue-matcher suggested four candidates; assessment against the actual stack: - #20641 (BUN-N2D): same `TypeCastTraits<JSVMClientData>::isType` frame but reached from `bunVMConcurrently` on the main event loop during libuv signal processing, not from a GC marker thread. Different code path; this PR does not touch it. - #20786 (BUN-PD8): same `isType` frame reached from `JSC::subspaceFor` inside `Request__create` on the HTTP server request path (main thread). Different code path; this PR does not touch it. - #27312: SIGILL (not SEGV) in `SlotVisitor::drain` on Linux during `bun test` cleanup. Adjacent area but a different fault signature; not claimed. - #31880: generic "multiple threads are crashing" under worker churn, no decoded stack. #32071 already declines to claim it for the same reason; not claimed here either. None are auto-closed by this PR. #20641 and #20786 suggest there may be other callers of `clientData()` that can see a bad `vm.clientData` on Windows; those are separate paths and out of scope here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #32073. Stacked on #32071 (base branch is that PR's branch; retargets to main when it lands).
Problem
The live-VM registry from #32071 keys liveness by address only. Its known residual, documented in the registry's doc comment: if a new VM is allocated at a dead worker VM's address, a stale producer that captured the old pointer passes the check and its completion is delivered to the new VM instead of being dropped. A registry-side counter alone cannot close this, because producers only bring an address to the lookup; the token has to be captured at schedule time and carried by the producer.
Fix
live_vm_registryis stamped with a process-unique generation (AtomicU64, starts at 1; 0 is the never-matches value of placeholder handles). The generation is also stamped intoVirtualMachine.live_generationand eachEventLoop.live_generationso handles can be built without the lock.enable_macro_modere-stamps the macro loop it recreates.Copyhandle types:VmHandle { addr, generation }(fromVirtualMachine::concurrent_handle()) andLoopHandle(fromEventLoop::concurrent_handle(), for producers that must deliver to the exact captured loop: regular vs macro, or the boxed spawnSync loop, which gets its own generation inregister_extra_loop).VirtualMachine::try_enqueue_task_concurrent,EventLoop::try_enqueue_task_concurrent,with_live_vm,is_shutting_down_or_freed,try_ref/unref_concurrently) take the handle and verify(addr, generation)under the registry lock. The lock-free main-VM fast paths compare the generation too; a stale pre-registration read only causes a spurious miss into the locked path.*mut VirtualMachine/BackRef<EventLoop>for completion delivery now stores the handle: FetchTasklet, S3 simple/list/download tasks, WorkTask, ConcurrentPromiseTask, AnyTaskJob, TranspilerJob dispatch, node:fs async tasks, zlib native streams (NativeZlib/NativeBrotli/NativeZstd), napi async_work and TSFN dispatch, FSWatcher/StatWatcher, PasswordObject, Archive async tasks, DevServer watcher events,Bun.buildcompletion, and blob copy_file/write_file on Windows.EventLoopHandle::Js/AnyEventLoop::Js(bun_event_loop) gain agenerationfield captured at construction, and theJsEventLoop::enqueue_task_concurrentdispatch carries it, covering the process waiter thread, shell tasks, and the bundler plugin/parse-task completions in one place.EventLoopHandle::from_tag_ptrre-reads the generation under its existing still-live contract.WakeHandlercarries the generation next to its context pointer so the auto-install wake (AsyncModule::on_wake_handler) can reassemble the handle.Structs whose VM backref is also used on the JS thread (FSWatcher, StatWatcher/scheduler, DevServer, TSFN) keep that pointer and add the handle for the one cross-thread access; single-purpose fields were converted outright.
GlobalJS::enqueue_task_concurrent_wait_pid(shell) is deleted: it had no callers and computed the VM from the global object at call time on non-JS threads, which cannot carry a schedule-time handle.Remaining address-only checks (explicitly named
*_addr_only)Pointers captured by C++ cannot carry a generation without C++-side plumbing, so
JSVMClientData::bunVM(JSCScheduler:Bun__queueJSCDeferredWorkTaskConcurrently,Bun__eventLoop__incrementRefConcurrently),EventLoopTaskNoContext(webcrypto CppTask ref/unref), and the napi finalizer's env-derived VM stay address-checked, which is exactly #32071's behavior for them. They are named*_addr_onlyso the residual is greppable. TSFN acceptance-after-death and napi env teardown remain tracked in #15964 / #30286.Verification
bun bd test test/js/web/workers/worker-terminate-lifetime.test.ts: 6/6 pass under the ASAN debug build, including Don't enqueue to a terminated worker's freed event loop from other threads #32071's fetch-in-flight ASAN test and two new tests:cargo check -p bun_binon linux-x64, windows-x64 (covers the copy_file/write_file changes), darwin-arm64; clippy clean on all touched crates.The ABA mis-delivery itself is not deterministically reproducible from JS (it requires the allocator to reuse a dead VM's exact address for a new VM while a stale producer is in flight), so there is no fail-before test for it; the new tests pin the delivery and drop behavior of the handle plumbing on both sides.