node: fix SEGV when worker termination races the first require("node:path") - #37296
node: fix SEGV when worker termination races the first require("node:path")#37296robobun wants to merge 10 commits into
Conversation
…ndex Zig::createPath returns null with an exception pending when a VM trap (such as a worker termination request) surfaces at its internal RETURN_IF_EXCEPTION. createNodePathBinding passed the result inline to putDirectIndex, which dereferences the empty JSValue in its isCustomGetterSetterSlow assert before the caller's exception check runs. Hoist each helper call into a local and check it before the put. jsNodeVmModuleGetModuleRequests had the same pattern with NodeVMModuleRequest::toJS; fixed the same way.
WalkthroughThe change adds pending-exception checks around JavaScript value conversion, process report construction, and array allocation in JSC bindings. It also scopes termination handling and adds a source-lint test for unchecked helper calls passed to ChangesBinding exception safety
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
…tion check A helper that allocates under its own ThrowScope returns null with an exception pending when a VM trap (worker termination, vm watchdog) surfaces at one of its internal RETURN_IF_EXCEPTION checks. Consuming such a call inline as the value argument of putDirect/putDirectIndex stores or dereferences an empty JSValue before the caller's check runs. The lint scans the C++ sources compiled into bun for the helper families that can return empty (create*/.toJS taking a global object first, and constructEmptyArray) inline inside putDirect* argument lists. Fix the remaining instances it found: process.report's empty cpus/networkInterfaces/nativeStack/libuv/workers arrays.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 7:49 PM PT - Aug 9th, 2026
✅ @robobun, your commit cffee829006d55fadcd462ac1cc30382b4d47bce passed in 🧪 To try this PR locally: bunx bun-pr 37296That installs a local version of the PR into your bun-37296 --bun |
There was a problem hiding this comment.
LGTM — the hoist-and-check transformation is mechanical and correct, and the earlier comment-length nit was addressed in 793d1cd. The two remaining inline nits (pre-existing construct*() lambda sites one frame up, and line-number drift in the lint's diagnostic output) are follow-up material and don't affect the crash fix.
Extended reasoning...
Overview
This PR fixes a fuzzer-caught SEGV where helper functions that return null-on-exception (because RETURN_IF_EXCEPTION services VM traps and can materialize a pending worker termination) were consumed inline by putDirect* before the caller's exception check ran. The fix is the canonical hoist-then-check shape across four C++ binding files (Path.cpp, NodeVMModule.cpp, BunProcess.cpp, BunProcessReportObjectWindows.cpp) plus a new source-lint test that enforces the invariant going forward, modeled on the existing empty-jsvalue-laundering.test.ts in the same directory.
Security risks
None. This is exception-propagation ordering in JSC bindings; no user-controlled input handling, auth, or crypto is touched.
Level of scrutiny
JSC bindings are memory-safety-adjacent, but the transformation here is purely mechanical: obj->putDirect*(.., helper(g), ..) → auto* v = helper(g); RETURN_IF_EXCEPTION(scope, {}); obj->putDirect*(.., v, ..). Each hunk is a strict improvement with no behavior change on the success path, and matches the shape already used elsewhere (e.g. SQLClient.cpp). The PR description demonstrates the root cause precisely (the 0x5 fault address is the m_type offset off a null cell in ASSERT(!value.isCustomGetterSetterSlow())), explains why validateExceptionChecks never caught it, and verifies the affected test suites still pass.
Other factors
- My prior nit (three-line narrative comments) was addressed in 793d1cd — both comments are now one line keeping only the non-obvious trap-servicing fact.
- The two new inline nits are non-blocking: the
construct*()lambda sites at BunProcess.cpp:2658-2683 are pre-existing (each lambda already had internalRETURN_IF_EXCEPTIONreturning{}before this PR), so nothing regresses; and the lint's block-comment stripper only affects the reportedfile:linepointer in the failure message, not pass/fail. - The new lint follows the established
test/internal/source-lints/conventions (globAllSources, tracked-file filter,expect(offenders).toEqual([])) and passes with zero suppressions on this branch.
The construct* lambdas in constructReportObjectComplete each return an empty JSValue from an internal RETURN_IF_EXCEPTION, so consuming them inline in putDirect had the same use-before-check shape one frame up. Widen the lint to catch zero-argument construct*/create* calls, and blank comments instead of deleting them so its reported line numbers stay accurate.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/internal/source-lints/putdirect-unchecked-helper.test.ts (1)
93-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd positive fixtures for every detector rule.
The test only asserts that the current tracked sources produce zero findings. A no-op scanner or an over-restrictive regex can therefore pass. Add table-driven fixtures for each unsafe helper pattern and assert the reported line and remediation. Include multiline calls and the
//-inside-string case from Line 81.As per coding guidelines, tests must prove they fail for the intended reason and must cover the complete relevant variant matrix.
🤖 Prompt for 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. In `@test/internal/source-lints/putdirect-unchecked-helper.test.ts` around lines 93 - 99, Add table-driven positive fixtures for every unsafe-helper detector rule in the putDirect* lint test, including multiline calls and the Line 81 case where // appears inside a string. Assert each fixture reports the expected line and remediation, while preserving the existing clean-source assertion and covering the complete relevant pattern matrix.Source: Coding guidelines
🤖 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/internal/source-lints/putdirect-unchecked-helper.test.ts`:
- Around line 80-81: Replace the regex-based stripping in the test’s content
preprocessing with a lexical scanner that removes line and block comments while
preserving normal and raw C++ string literals, including embedded // and /*
sequences. Keep non-comment characters and newlines intact so reported line
numbers remain accurate, and ensure putDirect calls following such literals are
still detected.
---
Outside diff comments:
In `@test/internal/source-lints/putdirect-unchecked-helper.test.ts`:
- Around line 93-99: Add table-driven positive fixtures for every unsafe-helper
detector rule in the putDirect* lint test, including multiline calls and the
Line 81 case where // appears inside a string. Assert each fixture reports the
expected line and remediation, while preserving the existing clean-source
assertion and covering the complete relevant pattern matrix.
🪄 Autofix
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: 9b906960-812d-42bb-944e-ddfa08bae945
📒 Files selected for processing (2)
src/jsc/bindings/BunProcess.cpptest/internal/source-lints/putdirect-unchecked-helper.test.ts
Replace the regex comment stripping with a small lexical scan that blanks comments and string/char literal contents while preserving newlines, so a literal containing // no longer truncates the line and reported line numbers stay accurate. Add positive and negative fixtures for every detector rule so a broken rule or no-op scan cannot pass.
… report helpers JSC__VM__ensureTerminationExceptionPending called handleTraps with no exception scope on the stack, so under exception-check validation the simulated throw was still unchecked when the next native entry point declared a scope and aborted (seen at JSC__JSModuleLoader__loadAndEvaluateModule when terminate() raced worker startup in worker-transfer-terminate-stress). Declare a top exception scope and observe the throw before returning; the termination exception itself stays pending for the caller's next check. Also hoist the remaining null-on-exception helpers consumed inline by putDirect in the process.report header: the createExecArgv extern shim (returns JSValue::ZERO when terminated) on both platforms, and the vm-first constructVersions/constructProcessReleaseObject, which carry their own scopes. Extend the lint to match underscore-qualified extern create* shims and document why vm-first calls stay unmodeled.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
50462cd was meant to remove one comment in bindings.cpp but its commit captured reverted content for every binding file this branch changes, leaving only the lint on the branch. Re-apply the Path, NodeVMModule, process.report, and ensureTerminationExceptionPending fixes exactly as reviewed.
There was a problem hiding this comment.
I reviewed cffee82 and found no further issues — all three prior rounds of feedback are addressed and the accidental revert in 50462cd is fully restored. Given this touches JSC exception-scope handling at the Rust/C++ termination boundary (JSC__VM__ensureTerminationExceptionPending now carries a release assertion) and adds a new 260-line source-lint with a hand-rolled C++ lexer, a human look is still worthwhile.
What was reviewed:
- The hoist-and-check transforms in Path.cpp, NodeVMModule.cpp, BunProcess.cpp, and BunProcessReportObjectWindows.cpp preserve property write order and add no new allocations.
DECLARE_TOP_EXCEPTION_SCOPE+releaseAssertNoExceptionExceptTermination()in bindings.cpp matches the pattern used across ~60 other binding files; checked that the only exceptionhandleTraps(NeedTermination)can raise is the termination exception, so the release assert holds.- The lint's
stripForScanlexer against raw strings, digit-separator', and//inside literals — the fixture set pins each rule and the earlier line-number/string-literal issues are fixed.
Extended reasoning...
Overview
Six files: five C++ bindings applying the same mechanical transform (hoist a null-on-exception helper into a local, RETURN_IF_EXCEPTION, then putDirect*) at ~25 sites in Path.cpp, NodeVMModule.cpp, BunProcess.cpp, and BunProcessReportObjectWindows.cpp; a two-line addition to JSC__VM__ensureTerminationExceptionPending in bindings.cpp declaring a top exception scope so handleTraps' simulated throw is observed before returning to Rust; and a new 263-line source-lint test that scans tracked C++ for the inline-helper pattern.
Security risks
None identified. No user-controlled input parsing, no auth/crypto/permissions surface. The changes are exception-propagation ordering only; on non-assert builds the pre-fix behavior wrote an array hole and unwound at the next check, so this is a crash-class fix on assert builds rather than memory corruption.
Level of scrutiny
Moderate-to-high. The C++ hoists themselves are mechanical and low-risk, but two aspects warrant a maintainer's eye: (1) bindings.cpp adds scope.releaseAssertNoExceptionExceptTermination() at the Rust FFI boundary — a release-build assertion on a worker-termination path that runs in production; (2) the new lint is repo infrastructure (a hand-rolled C++ lexical scanner with four regex rules) that will gate future PRs, and its false-positive/negative envelope — particularly the documented vm-first exclusion — is a design choice a maintainer should sign off on.
Other factors
This PR has been through three prior review rounds from me with substantive findings each time (missed sibling sites at the outer constructHeader layer, block-comment newline handling, //-in-string-literal handling, vm-first constructVersions/constructProcessReleaseObject, and an accidental full revert in 50462cd). All were addressed and the threads are resolved. The bug-hunting system found nothing on the current head. The commit history was rocky (a stale-index commit reverted every fix, then cffee82 restored them), which by itself argues for a human confirming the final tree matches intent. Existing test coverage: test/js/node/path/, vm.test.ts, and the process.report test all pass per the PR body; the new lint fails on base (25 offenders) and passes on the branch.
|
The const { Worker, isMainThread, parentPort } = require("worker_threads");
if (isMainThread) {
const w = new Worker(__filename);
w.on("message", () => w.terminate());
w.on("exit", () => console.log("survived"));
} else {
parentPort.postMessage("busy");
for (;;) process.report.getReport();
}On a debug build of main this dies on the first worker, 10/10 runs (UBSan reports the null cell one frame before the SEGV the assertion would otherwise produce): A debug build of this branch at cffee82 survives it (20 terminated workers per run, 3 runs), so the hoisting here covers it. db1d2ca on |
2 participants
Crash
Nested-worker fuzzing (release build with assertions and ASAN,
Malloc=1) hit a SEGV on a worker thread when a termination request raced the worker's firstrequire("node:path"):Trigger shape: main spawns a middle worker which spawns K leaf workers whose first statement is
require("node:path"); terminating the middle worker cascades termination requests into the leaves mid-require. Observed rate was about 1 hit per several thousand leaves that were mid-require when terminated.Cause
RETURN_IF_EXCEPTIONservices VM traps (ExceptionScope.hcallsvm.hasExceptionsAfterHandlingTraps()), so a pendingworker.terminate()can materialize as the termination exception at any such check.Zig::createPathdoesconstructEmptyObject(); RETURN_IF_EXCEPTION(scope, {});(Path.cpp:83) and returns null in that case, after the allocation already succeeded.createNodePathBindingpassedcreatePath(...)inline as theputDirectIndexargument, ahead of its own exception check, soputDirectIndexreceived the empty JSValue and dereferenced it inASSERT(!value.isCustomGetterSetterSlow())(the faulting address 0x5 is them_typeoffset off a null cell). On builds without assertions the empty value would be written as an array hole and the next check unwinds, so this is a crash class on assert-enabled builds rather than memory corruption.This is the case REVIEW.md calls out: the result of a throwing call was used before
RETURN_IF_EXCEPTION.Fix
Hoist each helper call into a local and check it before the put (the shape SQLClient.cpp already uses). An audit of the C++ compiled into bun for the same pattern (a null-on-exception helper consumed inline by
putDirect*) found the same bug in more places, fixed the same way:jsNodeVmModuleGetModuleRequestsconsumingNodeVMModuleRequest::toJS(same SEGV shape, reachable fromvm.SourceTextModulein a terminating worker)process.report.getReport()(BunProcess.cpp,BunProcessReportObjectWindows.cpp):constructEmptyArrayinline inputDirectfor the emptycpus/networkInterfaces/nativeStack/libuv/workersarrays; the 13construct*()section-builder lambdas inconstructReportObjectComplete, each of which returns an empty JSValue from an internalRETURN_IF_EXCEPTION; theBun__Process__createExecArgvextern shim (its Rust side returnsJSValue::ZEROwhen terminated) on both platforms; and the vm-firstconstructVersions/constructProcessReleaseObject, which carry their own scopesCI on this branch then caught the adjacent flavor of the same bug at the Rust boundary:
JSC__VM__ensureTerminationExceptionPendingcalledhandleTrapswith no exception scope on the stack, so the simulated throw was still unchecked when the next native entry point declared a scope, and the exception-check validator aborted atJSC__JSModuleLoader__loadAndEvaluateModule(hit byworker-transfer-terminate-stress.test.tson the x64-asan lane whenterminate()raced worker startup). The function now declares a top exception scope and observes the throw before returning to Rust; the termination exception itself stays pending for the caller's next check point, which is the function's contract.Test
The race itself cannot be made deterministic: the window is a single
constructEmptyObjectbetween two trap checks, and the termination bit is set by another thread.BUN_JSC_validateExceptionChecks=1cannot see the use-before-check either, becauseputDirectIndexon a freshArrayWithUndecidedarray never constructs aThrowScope, so the simulated throw from the helper's scope destruction is cleared by the too-late check with nothing in between to assert on (which is also why the exception-validation CI lane never flagged these sites).Instead the regression test is a source lint,
test/internal/source-lints/putdirect-unchecked-helper.test.ts, following the existing lints in that directory (empty-jsvalue-laundering.test.tsis the Rust flavor of the same invariant). It scans the C++ sources compiled into bun for the helper families that can return empty (create*/.toJStaking a global object first, including underscore-qualified extern shims,constructEmptyArray, and zero-argumentconstruct*/create*scope-capturing helpers) consumed inline insideputDirect*argument lists. The comment/string stripping is a small lexical scan so literals containing//cannot mask an offender, and every rule is pinned by positive and negative fixtures. vm-first calls are deliberately not modeled: the common vm-first builders are infallible and the name alone cannot tell the rare fallible ones apart, so those were hoisted by hand and the exclusion is documented in the lint header. On the unfixed tree the lint reports 25 sites with accurate file:line and fails; on this branch it passes with zero suppressions.Verification:
worker-transfer-terminate-stress.test.tspasses 6/6 consecutive runs underBUN_JSC_validateExceptionChecks=1on the fixed build (the configuration that aborted on the x64-asan lane)test/js/node/path/123 pass,test/js/node/vm/vm.test.ts214 pass, theprocess.reporttest passes, andprocess.report.getReport()smoke-checked on Linux and Windows (branch compile-verified natively on Windows)BUN_JSC_validateExceptionChecks=1stays clean onrequire("node:path")[review] gate passed · iteration 2 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 2 rejected · iteration 2
evidence per changed file