process.binding('uv'): fix segfault when worker.terminate() lands inside getErrorMap() - #37441
process.binding('uv'): fix segfault when worker.terminate() lands inside getErrorMap()#37441robobun wants to merge 1 commit into
Conversation
jsGetErrorMap built each [name, message] entry inside a void lambda that
dereferenced the result of constructEmptyArray without checking for an
exception (the check was left as a comment). constructEmptyArray returns
null once an exception is pending, and RETURN_IF_EXCEPTION also services
VM traps, so a worker.terminate() requested while a worker is inside
getErrorMap() turned into a null dereference that took down the whole
process ("Segmentation fault at address 0x4").
Build the map from a constexpr table in a plain loop instead, checking
the scope after every allocation, putDirectIndex and JSMap::set, so the
termination (or an out-of-memory error) propagates like any other
exception. The map contents and order are unchanged.
|
Warning Review limit reached
Next review available in: 11 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)
Comment |
|
Updated 12:18 AM PT - Aug 11th, 2026
✅ @robobun, your commit d1185a71e33d197eee2f7cc3eebe3180e39c2a6a passed in 🧪 To try this PR locally: bunx bun-pr 37441That installs a local version of the PR into your bun-37441 --bun |
|
Status: fix pushed, waiting on CI. Reproduced on the release build (1.4.0 canary 9008ae7) with the snippet in the description: a single worker looping on The same unchecked |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — textbook exception-check fix for a real null-deref crash.
What was reviewed:
- Verified
RETURN_IF_EXCEPTIONnow guardsconstructEmptyArray, bothputDirectIndexcalls, andJSMap::set; the constexpr table is built from the sameBUN_UV_ERRNO_MAPso map contents and order are unchanged. - Checked that
JSMap::create(vm, ...)is left unchecked consistently with sibling call sites (JSNodePerformanceHooksHistogramPrototype.cpp) — not a regression. - Test drains stdout/stderr/exited concurrently, is event-driven (no sleeps), asserts exact output before exit code, and the 20s timeout is commented per convention.
Extended reasoning...
Overview
This PR fixes a segfault in jsGetErrorMap (src/jsc/bindings/ProcessBindingUV.cpp) that occurs when worker.terminate() fires the VM termination trap while the worker is inside process.binding('uv').getErrorMap(). The old code called constructEmptyArray inside a void lambda and dereferenced the result without checking for a pending exception; when the termination trap surfaced there, the array was null and the process crashed. The fix replaces the macro-expanded lambda calls with a static constexpr table (built from the same BUN_UV_ERRNO_MAP X-macro) and a plain loop under a DECLARE_THROW_SCOPE, with RETURN_IF_EXCEPTION after each of the four throwing calls. A regression test spawns four workers looping on getErrorMap(), terminates each on its first message, and asserts clean exit codes.
Security risks
None. No untrusted input is parsed; the change only adds exception propagation to an internal binding that builds a fixed error-name map. The constexpr table is derived from compile-time constants.
Level of scrutiny
Low-to-moderate. This is native C++ in the JSC bindings, so memory-safety review rules apply, but the change is a mechanical, well-understood pattern: hoist a throw scope, check after every throwing call, and avoid check macros inside lambdas — exactly what REVIEW.md's "Exception checks after every call that can enter JS" section prescribes. The data flow is identical to before (same macro, same iteration order, same values), so there is no behavior change on the success path. I confirmed the pattern matches other constructEmptyArray callers in the tree, and that leaving JSMap::create unchecked matches sibling sites.
Other factors
- The test follows harness conventions cleanly:
bunEnv/bunExe, subprocess isolation for a crash repro, concurrent pipe draining, event-driven synchronization viapostMessage(no sleeps), stderr/stdout asserted before exit code, and a commented per-test timeout justified for debug/ASAN worker boot. The candidate concern about the explicit timeout was ruled out — it names the reason. - The added
import { describe, expect, test } from "bun:test"at the top of the test file is a benign explicit import of what was previously global. - No CODEOWNERS cover these paths. No prior human or bot reviews to address.
- The PR description documents that two sibling sites with the same shape (
ProcessBindingHTTPParser.cpp,BunString.cpp) were identified and are being handled separately, which satisfies the "if a site is intentionally excluded, say so" rule.
|
The automated review above has no action items, so nothing further is changing in this PR. Format and source lint checks are green; the Buildkite run (build 92016) is still in progress. |
|
FYI #37448 fixes the two other sites mentioned at the end of this description (BunString.cpp and ProcessBindingHTTPParser.cpp) and adds a source lint for the stored-result shape. For the lint to pass it carries this PR's ProcessBindingUV.cpp hunk unchanged, so the two merge cleanly in either order; the runtime test stays here. |
2 participants
Crash
Calling
worker.terminate()while the worker is insideprocess.binding("uv").getErrorMap()segfaults the whole process. Reproduces on the current release build every time (20 of 20 runs with a single worker):Under UBSan the first frame is
ProcessBindingUV.cpp:162: member call on null pointer of type 'JSC::JSObject'inBun::ProcessBindingUV::jsGetErrorMap. Besides directprocess.binding("uv")users,internal/fs/watch.tscalls this binding on its error path.Cause
jsGetErrorMapbuilt each[name, message]entry in avoidlambda:constructEmptyArrayreturns null after its ownRETURN_IF_EXCEPTION(or when it throws out of memory), andRETURN_IF_EXCEPTIONalso services VM traps (ExceptionScope.hgoes throughvm.hasExceptionsAfterHandlingTraps()), so thenotifyNeedTermination()issued byterminate()materializes as a pending termination exception inside one of the ~85constructEmptyArraycalls pergetErrorMap(). The lambda then dereferenced the null array.JSMap::setcan throw too (it materializes and grows the storage) and was unchecked as well.Fix
Replace the lambda with a
constexprtable built from the sameBUN_UV_ERRNO_MAPlist and a plain loop in the host function, with aRETURN_IF_EXCEPTIONafterconstructEmptyArray, eachputDirectIndexandJSMap::set(the shapecreatePatternFilledArrayin JSC and the otherconstructEmptyArraycallers in this tree use; REVIEW.md also asks for no check macros inside lambdas). The termination now propagates like any other exception and the worker exits with code 1. The map contents and iteration order are byte-identical to before (compared the JSON of[...getErrorMap()]from the release build and this build, 85 entries).Test
test/js/node/process-binding.test.tsgains a case that spawns bun, starts four workers that loop ongetErrorMap(), terminates each on its first message and expects[1,1,1,1]with an empty stderr and exit code 0. Without the fix the child segfaults (10 of 10 runs on the release build); with the fix it passes on the debug build.BUN_JSC_validateExceptionChecks=1is clean on the new loop, andtest/js/node/test/parallel/test-uv-errmap.jsandtest/js/node/watch/fs.watch.test.tsstill pass.While auditing for the same shape I found two more hoisted-but-unchecked
constructEmptyArrayresults (ProcessBindingHTTPParser.cppmethods/allMethodslazy builders, andJSC__JSValue__upsertBunStringArrayinBunString.cpp). They have much narrower windows and are being handled separately; this PR only changesProcessBindingUV.cpp.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file