jsc: check for exceptions before using a stored constructEmptyArray result

[robobun]

What

JSC::constructEmptyArray returns nullptr with an exception pending in two cases: JSArray::tryCreate failed and it threw out of memory, or its own RETURN_IF_EXCEPTION (JSGlobalObjectInlines.h) fired. That macro goes through vm.hasExceptionsAfterHandlingTraps(), so a worker.terminate() (or a vm timeout) whose trap is first serviced inside the call also produces the null return. Three builders in the tree stored the result and dereferenced it before checking the scope:

• JSC__JSValue__upsertBunStringArray (BunString.cpp), in the branch that turns a repeated key into a two element array. Its only caller is MatchedParams::to_js (bake FrameworkRouter.rs), reached for every dev server request whose catch-all route (pages/[...slug].tsx) matches two or more segments. Out of memory or termination applies here.
• process.binding('http_parser').methods and .allMethods (ProcessBindingHTTPParser.cpp). These are static-table PropertyCallback builders, evaluated once per global; node:http reads both at load. JSC defers termination around these callbacks (Lookup.cpp, JSObject::reifyAllStaticProperties), so out of memory is the only failure that reaches them.
• process.binding('uv').getErrorMap() (ProcessBindingUV.cpp), where the window is wide enough to hit on demand. process.binding('uv'): fix segfault when worker.terminate() lands inside getErrorMap() #37441 fixes that one with a runtime test; its hunk is included here unchanged so the lint below passes on this branch. The two PRs merge cleanly in either order.

The crash is the same in all three: panic: Segmentation fault at address 0x4 (member call on a null JSC::JSObject at the first putDirectIndex).

BUN_JSC_validateExceptionChecks=1 does not flag the first two sites. putDirectIndex into a pre-sized array and putDirect(vm, ..) open no ThrowScope, so the RETURN_IF_EXCEPTION at the end of upsertBunStringArray satisfies the validator while the dereference sits before it, and the http_parser builders had no scope at all.

Fix

• BunString.cpp: RETURN_IF_EXCEPTION after constructEmptyArray and after each putDirectIndex, the shape every other stored constructEmptyArray in the tree uses (84 of 88 sites; the other four are the ones above).
• ProcessBindingHTTPParser.cpp: a TopExceptionScope and RETURN_IF_EXCEPTION(scope, {}), as constructVersions in BunProcess.cpp does. reifyStaticProperty leaves the property unreified when a builder returns empty and both of its callers propagate the exception, so the access throws the out of memory error and a later access retries. The putDirectIndex calls that follow cannot throw: the arrays are pre-sized to the 35 and 47 entries of the two method maps.
• FrameworkRouter.rs: MatchedParams::to_js returns JsResult instead of .expect("unreachable") on a result that is reachable (and already was, via the getIfPropertyExists check and array->push). Both DevServer callers already return JsResult, and the early return takes the same path as the compute_arguments_for_framework_request error a few lines below. Re-auditing that loop for the new early returns turned up that the two clone_utf8 copies per param were never released on any path (toJS and upsertBunStringArray take their own references; the Zig version had a defer deref() on each), so every params object leaked its keys and values. They are now OwnedStrings, and the same one-line fix is applied to the match() binding in the same file, which had the same leak.

Test

There is no reliable runtime reproduction for the BunString or http_parser crashes: the termination has to be observed between the preceding exception check and the one inside constructEmptyArray, and the http_parser builders run once per global and only fail on out of memory. The fix is instead pinned by a source lint, test/internal/source-lints/construct-empty-array-unchecked.test.ts: after every stored constructEmptyArray call, the following statements have to be checks of the scope or of the result ending in one that leaves (RETURN_IF_EXCEPTION, or an if on scope.exception() / !result whose body returns or jumps), or a plain return of the result. Comments do not count, which is what the old // RETURN_IF_EXCEPTION line in ProcessBindingUV.cpp relied on, and neither does a branch that only asserts or logs. It has fixtures for each detected and accepted shape, and on main it reports exactly the four sites listed above:

lint output on main

src/jsc/bindings/BunString.cpp:921: `array = constructEmptyArray(..)` is not followed by an exception check (next statement: `array->putDirectIndex(global, 0, existingValue);`)
src/jsc/bindings/ProcessBindingHTTPParser.cpp:13: `methods = constructEmptyArray(..)` is not followed by an exception check (next statement: `int index = 0;`)
src/jsc/bindings/ProcessBindingHTTPParser.cpp:28: `methods = constructEmptyArray(..)` is not followed by an exception check (next statement: `int index = 0;`)
src/jsc/bindings/ProcessBindingUV.cpp:160: `arr = constructEmptyArray(..)` is not followed by an exception check (next statement: `arr->putDirectIndex(globalObject, 0, JSC::jsString(vm, String(name)));`)

This covers the stored-result flavor only; #37296 adds a lint for the inline argument flavor of the same contract.

The leak has a regression test in test/bake/framework-router.test.ts: 200 match() calls with four 256 KiB params grow RSS by 204 MB on the current release and by 12 MB on the fixed ASAN debug build (quarantine disabled for the measurement, as the other RSS tests do), against a 64 MB bound. MatchedParams::to_js itself is only reachable through a dev server request, so the production site is covered by the identical one-line change rather than a test.

Success paths were run on the debug build with BUN_JSC_validateExceptionChecks=1: test/bake/dev/ssg-pages-router.test.ts -t catch-all (exercises the put, new-array and push branches of upsertBunStringArray through MatchedParams::to_js), test/bake/framework-router.test.ts, test/js/node/process-binding.test.ts, test/js/node/http/node-http-parser.test.ts, test-uv-errmap.js and test-http-parser-lazy-loaded.js; methods.length, allMethods.length and getErrorMap().size are unchanged (35, 47, 85).

[robobun]

Status: reproduced by analysis only (the window is between the preceding exception check and the one inside constructEmptyArray; no deterministic runtime trigger for the BunString/http_parser sites). Proof is the new source lint, which fails on main listing the four sites and passes with this branch. Overlaps #37441 on ProcessBindingUV.cpp by design (identical hunk; that PR is still open).

Head 625ce60: http_parser builders propagate with RETURN_IF_EXCEPTION, and the clone_utf8 leak in the params loops is fixed with an RSS regression test (see the comment below and the description). All review threads are resolved. CI is green on this head (Buildkite build 92296, 190/190 jobs). Ready for a maintainer.

[coderabbitai]

Walkthrough

Changes

Exception propagation hardening

Layer / File(s)	Summary
JSC binding exception checks src/jsc/bindings/BunString.cpp, src/jsc/bindings/ProcessBindingHTTPParser.cpp, src/jsc/bindings/ProcessBindingUV.cpp	C++ binding code checks exceptions after JavaScriptCore array construction and mutation operations. HTTP parser array creation uses a shared helper. UV error-map construction uses a constexpr entry table.
Framework conversion error propagation src/runtime/bake/FrameworkRouter.rs, src/runtime/bake/DevServer.rs	MatchedParams::to_js returns JsResult<JSValue>. Both framework request paths propagate conversion errors.
Unchecked array construction lint test/internal/source-lints/construct-empty-array-unchecked.test.ts	Added scanner fixtures and tree-wide checks for unchecked stored constructEmptyArray results in tracked C++ sources.

Possibly related PRs

• oven-sh/bun#37025: Improves exception propagation in JSC bindings.
• oven-sh/bun#37064: Adds explicit exception checks after potentially throwing JSC operations.
• oven-sh/bun#37269: Addresses pending JavaScript exception handling in C++ bindings.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The description explains the problem, fix, scope, and verification results, although its headings differ from the template.
Title check	✅ Passed	The title clearly identifies the main change: adding exception checks before using stored constructEmptyArray results.

---

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

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@src/runtime/bake/DevServer.rs`:
- Line 2688: Update the request handling flow around params.to_js(global)? so
conversion failures complete resp before propagating or reporting the JavaScript
exception. End resp directly or invoke the existing HTTP error-page helper,
ensuring on_saved_request is not reached with an incomplete response.

In `@test/internal/source-lints/construct-empty-array-unchecked.test.ts`:
- Around line 68-72: The isExceptionCheck scanner currently accepts exception
conditions without verifying control-flow termination. Update isExceptionCheck
and its surrounding statement analysis to require the exceptional branch to
return, throw, or otherwise terminate before treating the check as safe;
preserve existing recognized macro and negated-identifier cases. Add a detects
fixture covering a branch that only logs before later using the potentially null
array.

🪄 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: 4f497824-292e-4a56-8c14-98140c12fc4e

📥 Commits

Reviewing files that changed from the base of the PR and between 54d6d16 and 478a4fd.

📒 Files selected for processing (6)

• src/jsc/bindings/BunString.cpp
• src/jsc/bindings/ProcessBindingHTTPParser.cpp
• src/jsc/bindings/ProcessBindingUV.cpp
• src/runtime/bake/DevServer.rs
• src/runtime/bake/FrameworkRouter.rs
• test/internal/source-lints/construct-empty-array-unchecked.test.ts

[claude]

LGTM — mechanical exception-check additions following the tree's established pattern, with a source lint pinning the invariant.

What was reviewed:

• BunString.cpp / ProcessBindingUV.cpp: RETURN_IF_EXCEPTION after constructEmptyArray/putDirectIndex matches the shape used at the other 84 sites; ProcessBindingUV hunk is identical to already-merged #37441.
• ProcessBindingHTTPParser.cpp: constructMethodsArray helper is a verbatim copy of the Process_stubEmptyArray PropertyCallback pattern in BunProcess.cpp (TopExceptionScope + tryClearException + reportUncaughtExceptionAtEventLoop).
• FrameworkRouter.rs: to_js → JsResult<JSValue> — grepped for callers, both DevServer sites updated with ? and already return JsResult.
• Source lint: self-test fixtures cover each detected/accepted shape and guard against a vacuous scan; globAllSources().cxx and the git-tracked filter match sibling lints in test/internal/source-lints/.

Extended reasoning...

Overview

This PR adds missing exception checks after JSC::constructEmptyArray at four call sites that stored the result and dereferenced it before checking the throw scope. It touches three C++ binding files (BunString.cpp, ProcessBindingHTTPParser.cpp, ProcessBindingUV.cpp), propagates a now-reachable error through MatchedParams::to_js in FrameworkRouter.rs / DevServer.rs, and adds a new source-lint test that scans every C++ source for stored constructEmptyArray results not immediately followed by an exception check.

Security risks

None. This is purely defensive exception handling in JSC binding code — no auth, crypto, parsing of untrusted input, or permission changes. The only behavioral change on the success path is zero (extra RETURN_IF_EXCEPTION macros that don't fire).

Level of scrutiny

Low-to-medium. The C++ changes are mechanical additions of RETURN_IF_EXCEPTION(scope, {}) following the exact shape used at 84 other sites in the tree. The constructMethodsArray helper in ProcessBindingHTTPParser.cpp is a line-for-line copy of Process_stubEmptyArray in BunProcess.cpp (same TopExceptionScope / tryClearException / reportUncaughtExceptionAtEventLoop sequence), which is the established pattern for PropertyCallback lazy builders that cannot propagate. The ProcessBindingUV.cpp hunk is identical to #37441 which is already on main. The Rust signature change (JSValue → JsResult<JSValue>) is compile-time enforced; I grepped for params.to_js( and confirmed both callers are updated and both enclosing functions already return JsResult.

Other factors

• The new source lint follows the conventions of the 16 existing lints in test/internal/source-lints/: it uses globAllSources().cxx, filters to git-tracked non-symlink files, has self-test fixtures for every detected shape (6) and every accepted shape (11), and asserts scanned > 0 so a broken glob cannot make the tree-wide check pass vacuously. The regex correctly excludes == via (?!=), strips comments while preserving line numbers, and skips closing braces / preprocessor directives when finding the next statement.
• Success paths were verified on the debug build with BUN_JSC_validateExceptionChecks=1 per the PR description; the counts (methods.length 35, allMethods.length 47, getErrorMap().size 85) are unchanged.
• No outstanding reviewer comments; only the robobun status note is on the timeline.

[robobun]

^{Updated 7:47 AM PT - Aug 11th, 2026}

✅ @robobun, your commit 625ce60436e7bdf30884146c3a90014983619fdd passed in Build #92296! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37448

That installs a local version of the PR into your bun-37448 executable, so you can run:

bun-37448 --bun

[claude]

I reviewed this PR and the bug-hunting system found no issues. The exception-check additions and the JsResult propagation look correct and follow existing patterns, but since this touches JSC bindings (the memory-safety hot zone), refactors jsGetErrorMap beyond a pure check-insertion, adds a ~350-line source lint, and coordinates with #37441, a human look would still be worthwhile.

What was reviewed:

• constructMethodsArray matches Process_stubEmptyArray in BunProcess.cpp (TopExceptionScope + tryClearException + report + jsUndefined()).
• Both params.to_js(global)? call sites in DevServer.rs return JsResult, so ? propagates to the same on_request error arm as the neighboring compute_arguments_for_framework_request(..)?.
• The jsGetErrorMap constexpr-table refactor preserves the entry set (same BUN_UV_ERRNO_MAP expansion) and now has a real ThrowScope where the old code only had a // RETURN_IF_EXCEPTION comment.
• The lint's accepted shapes match live tree patterns (RETURN_IF_EXCEPTION, if (scope.exception()) with a leaving body, hands-off return/RELEASE_AND_RETURN); realpathSync dedup and git ls-tree filtering follow the sibling source-lint conventions.

Extended reasoning...

Overview

Adds RETURN_IF_EXCEPTION after stored constructEmptyArray results in three C++ files (BunString.cpp, ProcessBindingHTTPParser.cpp, ProcessBindingUV.cpp), converts MatchedParams::to_js to JsResult<JSValue> with ? propagation at its two DevServer callers, and adds a 352-line source lint under test/internal/source-lints/ that requires an exception check as the next statement after every stored constructEmptyArray.

Security risks

None identified. This is defensive hardening: it converts potential null-pointer dereferences (segfault at 0x4 on putDirectIndex) into propagated JS exceptions or reported-and-swallowed errors in lazy property builders. No new attack surface, no user-controlled input handling changes.

Level of scrutiny

High. The touched code is JSC bindings, which REVIEW.md calls out as the most-blocked category. The changes themselves are mechanical (adding checks that ~84 other sites already have), but ProcessBindingUV.cpp also restructures a lambda-with-macro-expansion into a static constexpr table + loop, and ProcessBindingHTTPParser.cpp introduces a shared helper with a TopExceptionScope. Both follow established in-tree patterns (Process_stubEmptyArray, BunString__createArray), and I verified the pattern match, but a maintainer should confirm the tryClearException + reportUncaughtExceptionAtEventLoop + return jsUndefined() shape is what they want for these two PropertyCallback builders.

Other factors

• The source-lint test is substantial (350 lines of regex/bracket-matching C++ scanning) but sits in an established directory of similar lints and self-tests via detects/ignores fixtures for each recognized shape.
• All CodeRabbit and comment-cop review threads are resolved: the lint now requires the exception branch to leave the function (1220859), and the helper comment was shortened to one line (9ad12db).
• The ProcessBindingUV.cpp hunk intentionally overlaps open PR #37441; a human should confirm the merge-order plan.
• The .expect("unreachable") removals in FrameworkRouter.rs were demonstrably reachable (via the same constructEmptyArray OOM/termination path and via the pre-existing getIfPropertyExists/array->push in upsertBunStringArray), so the JsResult conversion is a strict improvement.

[robobun]

Two changes after a self-review pass, both pushed:

• ProcessBindingHTTPParser.cpp (bc84ba9): the helper that cleared and reported the exception is gone. reifyStaticProperty's callers in this WebKit check for a pending exception after a PropertyCallback and propagate it, so the builders now use the same TopExceptionScope + RETURN_IF_EXCEPTION(scope, {}) shape as constructVersions. Termination is also deferred around these callbacks, so the description now says out of memory is the only trigger there; the lint text was reworded to match.
• FrameworkRouter.rs (625ce60): re-auditing the to_js loop for the new early returns showed the two clone_utf8 copies per param were never released (the Zig version deferred a deref on each). They are OwnedStrings now, same for the one copy in the match() binding, with an RSS regression test in test/bake/framework-router.test.ts (204 MB growth on the current release, 12 MB on the fixed ASAN build, 64 MB bound).

The description is updated accordingly.

[claude]

I reviewed this PR and didn't find any bugs. The exception-check additions follow the established constructVersions/BunString__createArray shapes, the OwnedString leak fix is the idiomatic RAII wrapper, and both DevServer callers already return JsResult. Because the change bundles a JSC bindings refactor (ProcessBindingUV.cpp), a signature change propagated across two crates, a mid-review leak fix, and a new 350-line source-lint scanner, a human look would still be worthwhile.

What was reviewed:

• OwnedString derefs to &String for put_bun_string_one_or_array and drops on ? early return — no double-free, no leak on the new error paths.
• ProcessBindingHTTPParser.cpp TopExceptionScope + RETURN_IF_EXCEPTION(scope, {}) matches BunProcess.cpp:222 exactly; reifyStaticProperty callers propagate the empty return.
• ProcessBindingUV.cpp constexpr-table refactor preserves the 85-entry output and keeps the code-size win the old lambda comment referenced.
• Source lint's nextStatement/unprotectedUse handle brace-less if, [[unlikely]], and preprocessor lines; the 27 fixtures cover each accepted/rejected shape and the tree passes.

Extended reasoning...

Overview

The PR adds RETURN_IF_EXCEPTION checks after four stored constructEmptyArray results that previously dereferenced null on OOM/termination: JSC__JSValue__upsertBunStringArray (BunString.cpp), two PropertyCallback builders in ProcessBindingHTTPParser.cpp, and jsGetErrorMap in ProcessBindingUV.cpp (shared hunk with #37441). It propagates the resulting fallibility through MatchedParams::to_js (.expect("unreachable") → JsResult) and its two DevServer callers. During self-review the author found and fixed a clone_utf8 leak in the same loop (and its sibling in match()) by wrapping in OwnedString, with an RSS regression test. A new 353-line source lint pins the tree-wide invariant.

Security risks

None. No user-input parsing, auth, crypto, or trust-boundary changes. The exception checks harden crash paths (null deref → propagated error) and the leak fix reduces resource exhaustion risk. The source lint runs git ls-tree on the local repo only.

Level of scrutiny

Medium-high. JSC exception handling in C++ bindings is the most-blocked review category per REVIEW.md, and getting scope semantics wrong (missing RELEASE_AND_RETURN, wrong scope type) causes debug-assert failures or silent swallowing. The ProcessBindingUV.cpp change is a structural refactor (lambda+macro → constexpr table+loop), not a pure addition. The Rust signature change touches two crates. The source lint is a regex-based C++ scanner that will run on every future commit — false positives would block unrelated PRs.

Other factors

• All three CodeRabbit/github-actions review threads are resolved: the lint now requires the exception branch to leave the function (commit 1220859), the DevServer error-path concern was withdrawn as consistent with sibling ?s, and the long http_parser comment was cut then removed entirely when the propagation approach changed (bc84ba9).
• CI was green (190/190) on the previous head; the latest push (625ce60) adds only the leak fix and http_parser propagation change.
• The PR description documents BUN_JSC_validateExceptionChecks=1 runs on all affected success paths and unchanged output sizes (35/47/85).
• The scope expanded mid-review (leak fix in 625ce60) — justified under REVIEW.md's "re-auditing that loop for the new early returns" rule, but adds surface a human should confirm.

Given the breadth (7 files, ~400 net lines, JSC bindings + Rust signature change + new tree-wide lint), this exceeds the "simple, mechanical, or obvious" bar for auto-approval.