process.binding('uv').getErrorMap(): check for termination between puts

[Jarred-Sumner]

What

process.binding("uv").getErrorMap() builds its Map with constructEmptyArray / putDirectIndex / map->set calls whose RETURN_IF_EXCEPTIONs service VM traps, so a pending worker.terminate() (or node:vm timeout) is delivered inside the loop and the next call runs on a null cell (ProcessBindingUV.cpp: member call on null pointer under UBSan; SEGV on release). This is the last surviving face of the "termination lands on an ERR_* / error-table path" ledger row after #38457 fixed ErrorCodeCache::createError; the fix hoists each fallible result into a local and checks the scope before using it (this is #37441's change, rebased) and adds a regression test for the node:vm-timeout face of the same row that #38457 fixed.

Repro (before)

const { Worker, isMainThread, parentPort } = require("worker_threads");
if (isMainThread) { let n = 0; const again = () => { const w = new Worker(__filename);
    w.on("message", () => setTimeout(() => w.terminate(), n % 6));
    w.on("exit", () => (++n < 30 ? again() : console.log("survived"))); }; again();
} else { parentPort.postMessage("busy"); for (;;) process.binding("uv").getErrorMap(); }

Tests

test/js/node/process-binding.test.ts — "process.binding('uv').getErrorMap() survives worker.terminate() landing mid-call" (fails on the ASan canary and debug main); test/js/node/vm/vm.test.ts — "timeout landing while node validation errors are being constructed does not crash" (fails on the pre-#38457 canary; regression coverage for that half of the row).

Supersedes #37441.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 59 minutes

Limit details: You’ve used all 1 included review currently available under your plan. You completed 72 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e4b1805b-fc1b-4b30-bb4c-47fa5270bfc4

📥 Commits

Reviewing files that changed from the base of the PR and between 771c7e6 and 479d596.

📒 Files selected for processing (3)

• src/jsc/bindings/ProcessBindingUV.cpp
• test/js/node/process-binding.test.ts
• test/js/node/vm/vm.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

LGTM — mechanical exception-check fix with regression tests.

What was reviewed

• jsGetErrorMap: verified RETURN_IF_EXCEPTION now follows every fallible call (constructEmptyArray, both putDirectIndex, map->set) under a proper DECLARE_THROW_SCOPE; JSMap::create(vm, ...) takes no globalObject so no check needed there.
• The lambda→static constexpr table refactor preserves the exact entry set (same BUN_UV_ERRNO_MAP expansion) and avoids the per-entry code-bloat the old comment warned about.
• Both new tests assert a positive stdout value + exit code (not absence-of-crash strings), use await using, drain stdout before checking exitCode, and stderr is inherited so no pipe deadlock.

Extended reasoning...

Overview

The PR fixes a null-dereference crash in process.binding('uv').getErrorMap() (src/jsc/bindings/ProcessBindingUV.cpp) when a VM termination trap (worker.terminate() or node:vm timeout) is serviced mid-loop. The old code called constructEmptyArray inside a lambda with the RETURN_IF_EXCEPTION literally commented out, then dereferenced the result unconditionally. The fix declares a throw scope, converts the macro-expanded lambda calls into a static constexpr table + range-for, and checks the scope after each of the four fallible calls per iteration. Two subprocess regression tests are added: one for the getErrorMap/worker.terminate face and one for the node:vm-timeout/ERR_* face already fixed by #38457.

Security risks

None. This is purely adding missing exception-scope checks in a read-only error-table builder; no user input parsing, no auth, no I/O.

Level of scrutiny

Low-to-medium. The C++ change is 20 lines and is the textbook "add RETURN_IF_EXCEPTION after every call that can enter JS" pattern the repo's REVIEW.md explicitly requires. The refactor from ~85 macro-expanded lambda invocations to a constexpr table + loop is behavior-preserving (same BUN_UV_ERRNO_MAP X-macro drives both) and incidentally addresses the code-size concern the removed comment referenced. I checked that JSMap::create(vm, structure) does not take a globalObject and therefore needs no check, and that the final return needs no RELEASE_AND_RETURN because the last statement in the loop is already a scope check.

Other factors

• Tests follow harness conventions: bunExe()/bunEnv, await using on the subprocess, stdout asserted before exitCode, stderr: "inherit" (nothing to drain). The worker test asserts [1,1,1,1] (all four workers exit with the terminated code) rather than grepping for absence of a crash string, so it can actually fail. The vm test's 120 × ≤6ms timeouts keep it well under a second.
• The added import { describe, expect, test } from "bun:test" in process-binding.test.ts makes the file's implicit globals explicit; harmless.
• No prior human reviews or unresolved comments on the timeline (only a CodeRabbit rate-limit notice). The bug-hunting system found nothing.