event_loop: don't debug-panic on late main-VM enqueue_task_concurrent during process.exit - #36020
event_loop: don't debug-panic on late main-VM enqueue_task_concurrent during process.exit#36020robobun wants to merge 6 commits into
Conversation
…r process.exit Under BUN_DESTRUCT_VM_ON_EXIT (set by the CI runner), process.exit() on the main thread runs VirtualMachine::destroy(), which sets has_terminated = true, before libc exit(). A thread-pool fs.write that was blocked in the kernel and returns during the atexit chain (for example because the only FIFO reader closed) then calls enqueue_task_concurrent against a terminated VM. The debug-assertions guard in enqueue_task_concurrent turned that into panic: EventLoop.enqueueTaskConcurrent: VM has terminated on the work-pool thread. The main-thread VirtualMachine box is never dealloc'd, and neither concurrent_tasks nor the uws loop is torn down, so the late push + wakeup are safe; the task simply lands in a queue that is never drained again. Limit the assert to worker VMs, where has_terminated precedes a raw dealloc and the push is UAF-adjacent (the class of bug the assert was added to catch). The test drives the race deterministically: a tinycc-compiled atexit handler closes the FIFO reader and usleep()s after destroy() has returned, so the blocked write unblocks and posts while has_terminated is already set.
WalkthroughChangesThe debug termination guard now distinguishes main-thread and worker VMs. A Linux ASAN test reproduces a blocked filesystem write during VM shutdown race handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:30 PM PT - Jul 26th, 2026
✅ @robobun, your commit 4cffc9ad15282a3a1e8007214b8ffb25be6f56d0 passed in 🧪 To try this PR locally: bunx bun-pr 36020That installs a local version of the PR into your bun-36020 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nits: checked that the new cross-thread vm.is_main_thread() read is race-free (worker is set once at VM init and never mutated), and traced global_exit() → destroy() → bun_core::Global::exit() to confirm the main-thread VM box is never dealloc'd, so the late concurrent_tasks.push + wakeup after has_terminated = true are sound as claimed.
Extended reasoning...
The two inline findings are both non-blocking test-hygiene nits. The runtime change itself is a one-line narrowing of a cfg!(debug_assertions)-only panic — release builds are unaffected. I verified the added vm.is_main_thread() call reads self.worker.is_none(), a field set once at VM construction and never mutated, so calling it from the thread-pool completion path introduces no new data race beyond the pre-existing non-atomic has_terminated read. I also read VirtualMachine::global_exit() end-to-end and confirmed it calls self.destroy() then bun_core::Global::exit() without freeing the VM box, matching the PR's justification that the main-thread push lands in a never-drained-but-still-valid queue. Deferring rather than approving because relaxing a cross-thread teardown assert on a lifetime-invariant claim is worth a maintainer's eyes.
Non-ASAN Linux builds take the quick_exit() branch of Global::exit(), which skips __cxa_atexit handlers, so the atexit-driven reader close never runs there and the test passes vacuously. Gate on isASAN so the skip condition matches the only builds where both the mechanism and the debug assert exist. Replace the fixed 50ms setTimeout with a bounded readSync poll on the O_NONBLOCK reader: the first byte visible on rd proves the work-pool thread has entered the kernel write() and is blocked.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
4effc50 to
49f1c50
Compare
…iter bytes The bounded readSync poll used to fall through silently on deadline expiry and emit the same "alive" output as the success path, so the parent could not tell whether the race was actually armed. Track a sawBytes flag and exit 1 with a stderr message on fallthrough instead; the parent assertion already checks stderr/exitCode.
TinyCC ships only builtin headers, so a system #include may fail to resolve on some runners and trip its longjmp error path (which conflicts with ASAN). Match the existing __cxa_atexit forward-decl so the helper compiles with no system headers.
There was a problem hiding this comment.
LGTM — all four rounds of prior feedback have been addressed in be3956b, 80e7d7e, 49f1c50, and 332829f.
What was reviewed:
- The assert narrowing is sound:
destroy()doesn't touchconcurrent_tasksor the uws loop, andglobal_exit()never dealloc's the main-thread VM box, so the late push+wakeup are benign; the worker path (raw-dealloc'd inWebWorker::shutdown) keeps the guard. is_main_thread()isself.worker.is_none()— no side effects, safe to call from a thread-pool thread against the terminated-but-static main VM.- Test now gates on
isASAN, pollsreadSyncfor the observable writer-in-kernel signal, fails loudly on deadline expiry, and forward-declaresclose/usleepso TinyCC never hits its longjmp error path under ASAN.
Extended reasoning...
Overview
Two-file change: a 3-line narrowing of the cfg!(debug_assertions)-only guard in EventLoop::enqueue_task_concurrent (src/jsc/event_loop.rs) so it only panics for terminated worker VMs, plus a new Linux+ASAN regression test (test/js/node/fs/fs-write-exit-race.test.ts) that deterministically reproduces the CI flake from build 82810.
Security risks
None. The change only affects a debug-build assertion; release builds already compile the guard out entirely and take the same push + wakeup path being permitted here. The test spawns a hermetic child in a tempDir with no network or elevated-privilege interaction.
Level of scrutiny
Low-to-medium. VM shutdown ordering is subtle, but the change surface is minimal: it turns a debug false-positive into a no-op on the one path (global_exit → destroy → atexit chain, main-thread VM only) where the pushed task is provably harmless — the VM box is process-static, concurrent_tasks is left intact by destroy(), and the queue is simply never drained again. I traced is_main_thread() (= worker.is_none()), has_terminated (set at the tail of destroy()), and global_exit() to confirm the PR description's claims. The worker-VM case, which #34154 addresses, retains the guard.
Other factors
This PR has gone through four rounds of my own review feedback, each addressed promptly:
- ASAN-only reachability of
__cxa_atexit→ gated on!isASAN(be3956b) setTimeout(50)→ replaced with boundedreadSyncpoll (be3956b)- Silent poll-deadline fallthrough →
sawBytesflag + stderr + exit(1) (80e7d7e) <unistd.h>under TinyCC+ASAN → forward-declaredclose/usleep(332829f)
The comment-cop bot's length complaint was also addressed (49f1c50). No bugs were surfaced by the bug-hunting system on the current revision. The test's remaining reliance on cc() in an ASAN subprocess matches the pattern of test/regression/issue/26249 and 20144, which run green on linux-x64-asan; with system headers removed, the residual TinyCC/ASAN risk I flagged is eliminated.
There was a problem hiding this comment.
LGTM — all four rounds of feedback have been addressed.
What was reviewed:
- The runtime change narrows a
cfg!(debug_assertions)-only guard; release behavior is unchanged, andis_main_thread()readsself.worker(set once at VM init), so no new cross-thread hazard is introduced beyond the pre-existinghas_terminatedread. - Test now gates on
isASAN, polls for the writer-in-kernel signal with a loud deadline failure, and forward-declaresclose/usleepso tinycc never hits itslongjmperror path. - Checked that
helper.cuses no system headers and that thecc()-in-subprocess-under-ASAN pattern matches the two existing ungated precedents (issue 20144 / 26249).
Extended reasoning...
Overview
Two files: a 3-line change to src/jsc/event_loop.rs narrowing the debug-only has_terminated panic in enqueue_task_concurrent to worker VMs only, and a new Linux+ASAN regression test at test/js/node/fs/fs-write-exit-race.test.ts that deterministically reproduces the late work-pool completion racing process.exit().
Security risks
None. The change relaxes a cfg!(debug_assertions) assertion; release builds already push into the concurrent queue unconditionally on this path. The main-thread VirtualMachine box is process-static (global_exit never deallocs it) and destroy() leaves both the MPSC queue and uws loop intact, so the late push+wakeup are sound. The worker-VM guard — the case that IS UAF-adjacent because WebWorker::shutdown raw-deallocs the box — remains in place.
Level of scrutiny
Low for the runtime change: it only affects debug builds, the reasoning is documented, and is_main_thread() (self.worker.is_none()) reads init-time-immutable state so there's no new cross-thread read hazard. Medium for the test: it's an elaborate fixture (FIFO + tinycc __cxa_atexit handler) that only runs on the linux-x64-asan lane, but it has been through four review iterations — isASAN gating (be3956b), poll-instead-of-sleep (be3956b), loud deadline failure (80e7d7e), and forward-declared libc symbols to avoid TinyCC's longjmp under ASAN (332829f) — and now follows harness conventions.
Other factors
The author's evidence marker notes the test couldn't be run locally (platform-specific), so CI is the arbiter; if the fixture misbehaves on the linux-x64-asan lane it will fail loudly (the parent asserts an exact {stdout, stderr, exitCode} object), not vacuously. All prior inline comments are marked resolved and the comment-cop bot is satisfied after the comment was reduced to one line (49f1c50).
|
CI status: the diff itself is green. The new test ( The remaining red is unrelated flake that passed on retry in both runs: Ready for maintainer review. |
1 participant
What
Under
BUN_DESTRUCT_VM_ON_EXIT=1(set by the CI runner),process.exit()on the main thread runsVirtualMachine::destroy()(which setshas_terminated = true) beforelibc::exit(). If a thread-poolfs.writethat was blocked in the kernel returns during the atexit chain, its work-pool completion callsenqueue_task_concurrentagainst a VM whosehas_terminatedis already set, and thedebug_assertions-gated guard panics:Seen on linux-x64-asan in build 82810 via
test/js/node/fs/fs.test.ts(acreateWriteStreamto a FIFO whose reader is held by the parent process).Why this is correct
The main-thread
VirtualMachinebox is process-static (global_exit()neverdeallocs it).destroy()leaves both theconcurrent_tasksMPSC queue and the uws loop intact, so the latepush+wakeupare sound; the task just lands in a queue that is never drained again. Release builds already behave that way since the guard is compiled out. The assert exists to catch producers that enqueue into a worker VM afterdestroy(), where the box is about to be raw-dealloc'd inWebWorker::shutdown(), so the guard is kept for that case.Fix
Gate the debug panic on
!vm.is_main_thread().Test
test/js/node/fs/fs-write-exit-race.test.tsreproduces the race deterministically on Linux: the child opens a FIFO, dispatches a blockingfs.writeto the thread pool, and registers a libc atexit handler (viabun:fficc()) that closes the only reader fd and thenusleep()s. The handler runs afterdestroy()has returned, so the blockedwrite()getsEPIPEand posts its completion whilehas_terminatedis already set.On an unfixed debug/ASAN build the child emits the panic + backtrace (assertion fails); with the fix it exits with
{stdout: "alive\n", stderr: "", exitCode: 0}.Related: #34154 tackles the worker-VM side of this class (the UAF) and leaves the guard for workers in place; this change only stops the main-VM false positive.
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/node/fs/fs-write-exit-race.test.ts