Fix exception handling in unwrapBoxedPrimitive - #37025
Conversation
Unwrapping a String or Number wrapper object calls toString/toNumber, which can re-enter JS through Symbol.toPrimitive and throw. The binding returned the partial result with the exception still pending, tripping the exception scope assertion in debug builds when YAML.stringify, TOML.stringify, or JSON5.stringify received such a wrapper. Check for the exception and return empty so the error propagates as a JS throw.
Walkthrough
ChangesBoxed primitive error handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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
🤖 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/js/bun/yaml/yaml.test.ts`:
- Around line 4280-4296: Update the child-process wait around Bun.spawn to
consume proc.stderr.text() alongside proc.stdout.text() and proc.exited in the
existing Promise.all, preserving the current stdout and exit-code handling.
🪄 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: 5d89cc45-91a5-4868-ac16-5d580e2506cf
📒 Files selected for processing (2)
src/jsc/bindings/bindings.cpptest/js/bun/yaml/yaml.test.ts
|
Good catch, the test piped stderr without reading it. Pushed b084997 to drain stderr in the Promise.all and assert it is empty. |
|
Updated 1:40 AM PT - Aug 6th, 2026
@dylan-conway, your commit 31a8c45 is building: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
The stderr finding was already addressed in b084997: the Promise.all now drains proc.stderr.text() and the test asserts stderr is empty, so a regressed build surfaces the assertion dump in the failure output. The review ran against the initial commit. |
There was a problem hiding this comment.
LGTM — the stderr-drain nit from my earlier pass is now addressed in the current diff (b084997), and the C++ change is the textbook DECLARE_THROW_SCOPE + RETURN_IF_EXCEPTION pattern.
Checked: toNumber/toString are the only fallible calls in this function (the BooleanObject/BigIntObject arm reads internalValue() directly, no JS re-entry). Returning {} on exception satisfies the zero-is-throw contract enforced by from_js_host_call on the Rust side. The subprocess test now drains stdout+stderr+exited concurrently and covers the String, Number, and space-argument paths.
Extended reasoning...
Overview
JSC__JSValue__unwrapBoxedPrimitive in src/jsc/bindings/bindings.cpp calls object->toNumber() / object->toString() on Number/String wrapper objects. Those can re-enter JS via Symbol.toPrimitive and throw. The old code returned the (garbage) result with an exception still pending; the Rust caller wraps this via host_fn::from_js_host_call → call_zero_is_throw, which asserts the return is empty iff an exception is pending, so debug builds hit releaseAssertNoException and abort. The fix declares a throw scope after the not-an-object early return and adds RETURN_IF_EXCEPTION(scope, {}) after each fallible call. The BooleanObject/BigIntObject branch uses internalValue() (a direct internal-slot read, cannot throw) and correctly needs no check.
A subprocess regression test in test/js/bun/yaml/yaml.test.ts exercises three routes into this binding: a boxed String at the top level, a boxed Number nested in an object, and a boxed String passed as the space argument (which YAMLObject.rs::Space::init also routes through unwrap_boxed_primitive). It asserts the exact TypeError messages on stdout, empty stderr, and exit 0.
Security risks
None. This tightens error propagation on an existing extern-C binding; no new user-controlled data flows are introduced.
Level of scrutiny
Low-to-medium. The C++ change is 6 lines applying the canonical JSC exception-check idiom that REVIEW.md mandates for every call that can enter JS. It matches WebKit's own unwrapBoxedPrimitive in JSONObject.cpp, whose callers do RETURN_IF_EXCEPTION immediately after — here the check just moves inside the extern-C boundary because the caller is Rust. All Rust call sites (YAMLObject.rs, TOMLObject.rs, JSON5Object.rs) already use ? on the JsResult, so the propagated error surfaces cleanly with no further changes needed.
Other factors
My previous inline comment (stderr piped but not drained) ran against the initial commit and was fixed in b084997 before that review posted; the current diff drains proc.stderr.text() in the Promise.all and asserts it empty, matching the harness template. CodeRabbit's identical finding is marked resolved. No open reviewer feedback remains. The test uses bunExe()/bunEnv/await using, asserts stdout before exitCode, and the PR description confirms it exits 134 on the unfixed build — so it fails for the right reason.
There was a problem hiding this comment.
LGTM — the stderr-drain nit from the earlier review is addressed in b084997.
What was reviewed:
DECLARE_THROW_SCOPE+RETURN_IF_EXCEPTIONaftertoNumber/toString; empty return honors the zero-is-throw contract thatfrom_js_host_call→call_zero_is_throwenforces on the Rust side, and every caller (YAMLObject.rs,TOMLObject.rs,JSON5Object.rs) already propagates with?.- Boolean/BigInt branch reads
internalValue()without re-entering JS, so no scope check needed there. - Subprocess test now drains stdout+stderr concurrently, asserts stderr empty and exit 0; covers String wrapper, Number wrapper nested in an object, and the
spaceargument path.
Extended reasoning...
Overview
Two-file change: src/jsc/bindings/bindings.cpp adds a throw scope to JSC__JSValue__unwrapBoxedPrimitive and returns empty on exception from toNumber/toString (both of which can re-enter JS via Symbol.toPrimitive/valueOf/toString on wrapper objects). test/js/bun/yaml/yaml.test.ts gains a subprocess regression test exercising three routes into the binding.
Security risks
None. This tightens exception handling on an existing extern-C binding; no new attack surface, no auth/crypto/permissions.
Level of scrutiny
Low-to-medium. The change is the canonical JSC exception-check pattern (DECLARE_THROW_SCOPE + RETURN_IF_EXCEPTION) applied to a copy of WebKit's own unwrapBoxedPrimitive. I verified the Rust wrapper unwrap_boxed_primitive in src/jsc/JSValue.rs:2298 routes through host_fn::from_js_host_call → call_zero_is_throw, so an empty encoded return correctly surfaces as Err(JsError). All call sites in YAMLObject.rs, TOMLObject.rs, and JSON5Object.rs use ?, so the error propagates cleanly. The BooleanObject/BigIntObject branch uses JSWrapperObject::internalValue(), which is a direct field read and cannot throw, so leaving it uncovered by the scope is correct.
Other factors
My prior review flagged that the test piped stderr without reading it; commit b084997 fixed that — the test now drains proc.stderr.text() in the Promise.all and asserts it is empty, matching the harness template. Both the coderabbit and my prior inline comments are marked resolved. The PR includes gate evidence showing the test fails (exit 134 / assertion abort) on the unfixed debug build and passes on the fixed build across debug+ASAN and release. The test asserts exact error messages and covers the String, Number, and space-argument variants. Small, mechanical, well-tested fix following an established pattern.
|
On the red CI: the two failures in build 89525 are worker terminate tests on the debian 13 x64-asan lane (worker-transfer-terminate-stress.test.ts and test-worker-message-port-transfer-terminate.js), both hitting an assertNoException abort during worker teardown. The previous build 89499 ran the identical tree and that lane passed, and this PR's change is only reachable through YAML/TOML/JSON5 stringify, so they look like a preexisting race on main. Both are reported for triage. The other 193 jobs passed, including the YAML suites. |
There was a problem hiding this comment.
LGTM — the stderr-drain nit from the earlier pass is addressed.
What was reviewed:
RETURN_IF_EXCEPTIONplacement aftertoNumber/toStringand that returning{}matches thefrom_js_host_callzero-is-throw contract on the Rust side.- Confirmed all Rust callers (
YAMLObject.rs,TOMLObject.rs,JSON5Object.rs) propagate via?, so the empty return surfaces as a JS exception everywhere. - Boolean/BigInt branch uses
internalValue()which cannot re-enter JS, so no scope check needed there. - Subprocess test now drains stdout+stderr concurrently, asserts exact error messages for String/Number/space paths, and checks exit 0.
Extended reasoning...
Overview
The PR fixes a fuzzer-found abort in JSC__JSValue__unwrapBoxedPrimitive (src/jsc/bindings/bindings.cpp). When a boxed String/Number has a Symbol.toPrimitive that throws (e.g. returns an object), toString/toNumber leave a pending exception; the old code returned a value anyway, violating the zero-is-throw contract enforced by host_fn::from_js_host_call and tripping releaseAssertNoException in debug builds. The fix adds a DECLARE_THROW_SCOPE and RETURN_IF_EXCEPTION(scope, {}) after each throwing call. A subprocess regression test in test/js/bun/yaml/yaml.test.ts covers String, Number, and the space argument path.
Security risks
None. This tightens error propagation on an existing code path; no new input parsing, no auth/crypto/permissions surface. The change turns a debug-build abort (and a silently-dropped exception in release) into a proper JS TypeError.
Level of scrutiny
Low-medium. The C++ change is 9 lines applying the canonical JSC exception-check pattern (DECLARE_THROW_SCOPE + RETURN_IF_EXCEPTION) that appears throughout bindings.cpp. I verified the Rust side: unwrap_boxed_primitive wraps the FFI call in from_js_host_call (call_zero_is_throw), so an encoded-empty return becomes Err(JsError), and every call site in YAMLObject.rs/TOMLObject.rs/JSON5Object.rs uses ?. The BooleanObject/BigIntObject branch reads internalValue() directly and cannot re-enter JS, so it correctly needs no check.
Other factors
- The one review nit (stderr piped but not drained) from both CodeRabbit and my earlier pass was fixed in b084997 — the diff now shows
Promise.all([stdout, stderr, exited])withexpect(stderr).toBe(""). - PR evidence shows the test fails (exit 134) on the unfixed debug+ASAN build and passes on the fixed build, satisfying the fails-without-fix requirement.
- The two red CI jobs are unrelated worker-terminate flakes on debian-13 asan that passed on the identical tree in the prior build; this change is only reachable via YAML/TOML/JSON5 stringify.
2 participants
Fixes a fuzzer-found abort (fingerprint
2f7e21cbc0aaac7b).JSC__JSValue__unwrapBoxedPrimitivecallstoString/toNumberon String and Number wrapper objects. Those re-enter JS when the wrapper has aSymbol.toPrimitive(or overriddenvalueOf/toString), so they can throw, for example whenSymbol.toPrimitivereturns an object. The binding ignored the pending exception and returned a value anyway, violating the zero-is-throw contract checked on the Rust side and trippingreleaseAssertNoExceptionin debug builds:WebKit's
unwrapBoxedPrimitivein JSONObject.cpp (which this was copied from) relies on its callers doingRETURN_IF_EXCEPTIONright after; our extern C copy returns straight to Rust, so the check has to live in the binding itself. This adds a throw scope and returns empty whentoNumber/toStringthrows, so the error surfaces as a normal JS exception fromBun.YAML.stringify,Bun.TOML.stringify, andBun.JSON5.stringify(all reach this throughunwrap_boxed_primitive).The regression test runs in a subprocess because the unfixed binary aborts the whole process in debug builds. Verified the spawned script exits 134 on the unfixed build and exits 0 with the expected TypeError output on the fixed one; yaml, toml, and json5 suites pass.
[stamp-90s] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file