Propagate exceptions from boxed primitive unwrapping in YAML/TOML/JSON5 stringify - #37040
Propagate exceptions from boxed primitive unwrapping in YAML/TOML/JSON5 stringify#37040robobun wants to merge 1 commit into
Conversation
JSC__JSValue__unwrapBoxedPrimitive called toNumber()/toString() on Number and String wrapper objects without checking for exceptions. When the conversion threw (a user-defined valueOf/toString, or a stack overflow near the stack limit), the function returned a non-empty value with the exception still pending, violating the zero-is-throw FFI convention and tripping the exception scope assertion in debug builds. Found by Fuzzilli via Bun.YAML.stringify(new String()) with a nearly exhausted stack.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesYAML boxed primitive exception propagation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Closing as a duplicate of #37025, which fixes the same missing exception check in JSC__JSValue__unwrapBoxedPrimitive with an identical code change and was opened first. That PR's repro reaches the bug through Symbol.toPrimitive, this one through overridden toString/valueOf and a near-exhausted stack, but both paths go through the same two conversion calls, so one fix and one test cover it. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — textbook exception-scope fix for a fuzzer-found abort.
What was reviewed:
- Verified the Rust side wraps this in
from_js_host_call, so returning{}on exception correctly propagates asJsError::Thrown; all call sites (YAML/TOML/JSON5 stringifiers,Space::init) already use?. - Checked the remaining branches:
BooleanObject/BigIntObjectreadinternalValue()directly (no user JS, cannot throw) and the fallthrough just returns the input object — no missing exception checks. - Test asserts specific error messages and covers both wrapper types at root and nested positions.
Extended reasoning...
Overview
This PR fixes JSC__JSValue__unwrapBoxedPrimitive in src/jsc/bindings/bindings.cpp to declare a throw scope and RETURN_IF_EXCEPTION after object->toNumber() and object->toString(), which can invoke user-defined valueOf/toString/Symbol.toPrimitive and throw. Previously the function returned a non-empty encoded value with an exception pending, tripping releaseAssertNoException in debug builds (Fuzzilli fingerprint daec39a4e7f75e61). A regression test is added to test/js/bun/yaml/yaml.test.ts.
Security risks
None. This is purely error-propagation hygiene in a stringifier helper; no new inputs are accepted and no security-relevant surface is touched.
Level of scrutiny
Low-to-medium. The change is 8 lines of C++ following the exact idiom REVIEW.md mandates ("Every call that can throw or run user code needs RETURN_IF_EXCEPTION under a ThrowScope before its result is used"). I confirmed via grep that the Rust wrapper (JSValue::unwrap_boxed_primitive → host_fn::from_js_host_call) already interprets the empty return value as Err, and every one of the ~10 call sites in YAMLObject.rs, TOMLObject.rs, and JSON5Object.rs propagates with ?, so no caller needs updating. The unchanged BooleanObject/BigIntObject branch reads internalValue() on a JSWrapperObject — that's a direct field read, cannot enter user JS, so no scope check is needed there.
Other factors
The test is well-constructed: it exercises both throwing paths (String wrapper via toString, Number wrapper via valueOf), at both the root and inside a container, and asserts the specific thrown message rather than a bare toThrow(). It's placed alongside the existing boxed-primitive tests in the YAML suite. The PR description accurately explains why the WebKit original doesn't need this (its callers do the check) but the FFI export does. This is a small, self-contained, obviously-correct fix.
1 participant
Fixes a Fuzzilli-found abort (fingerprint
daec39a4e7f75e61):Bun.YAML.stringify(new String())called with a nearly exhausted stack trippedExceptionScope::releaseAssertNoExceptionwith a pending "Maximum call stack size exceeded" exception.Root cause
JSC__JSValue__unwrapBoxedPrimitiveinbindings.cpp(used by the YAML, TOML, and JSON5 stringifiers throughJSValue::unwrap_boxed_primitive) callsobject->toNumber()/object->toString()on Number and String wrapper objects. Both can run user JavaScript (valueOf/toString/Symbol.toPrimitive) and can throw, including a stack overflow when the stack is nearly gone. The function ignored the exception and returned a non-empty encoded value, violating the zero-is-throw convention the Rustfrom_js_host_callwrapper validates. Debug builds abort on the exception scope assertion; release builds kept stringifying with an exception pending.The WebKit helper this was copied from (
unwrapBoxedPrimitiveinJSONObject.cpp) relies on its callers doingRETURN_IF_EXCEPTIONright after; the FFI version has to do that itself.A stack overflow isn't needed to hit this. The deterministic repro used for the regression test:
Fix
Add a throw scope and return the empty value when
toNumber()/toString()throws, so the Rust side propagates the error normally.Testing
test/js/bun/yaml/yaml.test.tscovering throwingtoString/valueOfon boxed primitives at the root and nested. The assertion only exists in debug builds (release already surfaced the error because the pending exception was noticed once the host function returned), so the test guards the regression by aborting the debug test runner if it comes back.test/js/bun/yaml/,test/js/bun/toml/,test/js/bun/json5/all pass with the debug build.