inspect: don't leave an exception pending while walking an object's properties - #39419
inspect: don't leave an exception pending while walking an object's properties#39419robobun wants to merge 1 commit into
Conversation
…roperties JSC__JSValue__forEachProperty skipped a property when getPropertySlot returned false, but did not clear the exception that made it return false (a lazy static property whose initializer threw, or a Proxy get trap on a prototype). The next lazy property on the object was then built with that exception still pending, which trips the no-exception assertion in debug builds and silently drops every later property in release builds. The stale exception also made getPrototype() on a Proxy prototype return the empty value, which was dereferenced; a getPrototypeOf trap that throws hit the same dereference on its own. Clear the exception whether or not the lookup found the property, and stop walking the prototype chain when getPrototype() throws.
|
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)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 2 remain after this review. WalkthroughChangesBun.inspect exception handling
Possibly related PRs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change makes property inspection safely skip throwing properties while preserving the rest of the object and avoiding prototype-walk crashes. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-understood exception-handling fix that matches the sibling forEachPropertyOrdered pattern.
What was reviewed:
- Verified the first hunk mirrors
forEachPropertyOrdered(bindings.cpp:5814-5818): capturehasProperty, clear exception, then branch. - Verified the second hunk matches the fast path's
getPrototypehandling (bindings.cpp:5601-5611); the trailingtryClearException()at 5732 clears the trap's exception afterbreak. - Checked both new tests follow harness conventions (subprocess isolation,
Promise.allon pipes, combined{stdout, stderr, exitCode}assertion) and cover both fixed paths independently.
Extended reasoning...
Overview
Two-hunk change to JSC__JSValue__forEachPropertyImpl in src/jsc/bindings/bindings.cpp (the property walk behind Bun.inspect/console.log), plus two subprocess tests in test/js/bun/util/inspect.test.js. The first hunk moves CLEAR_IF_EXCEPTION to run whether or not getPropertySlot found the property (previously it only ran on the found branch, so a throwing lazy initializer or Proxy get trap that reported not-found left the exception pending into the next iteration). The second hunk checks for an exception after getPrototype() before dereferencing the result, since a throwing getPrototypeOf trap returns an empty JSValue.
Security risks
None. This is an exception-handling correctness fix in a formatter code path. No new user-controlled data flows, no auth/crypto/permissions.
Level of scrutiny
Medium — it's C++ in the JSC bindings, but the change is mechanical and directly mirrors two existing patterns in the same file: forEachPropertyOrdered at lines 5813-5818 already does exactly the hasProperty = ...; clear; if (!hasProperty) continue shape, and the fast path at 5601-5611 already null-checks the prototype and clears the exception. The PR description traces the crash mechanism precisely (stale exception → next lazy initializer's to_js_host_call assertion in debug, or getPrototype() returning empty → .getObject() null deref in release), and the fix is the minimal change that addresses both.
Other factors
- Both tests run in a child process (isolating the
process.envoverride and the segfault), drain stdout/stderr/exited concurrently, and assert on a combined object per repo test guidance. - The first test's assertion that only
$is missing is somewhat coupled toBun.$'s initializer readingprocess.envand no other lazyBunproperty doing so — but that's the strongest invariant available for the fuzzer's exact repro, and a future change that breaks it would fail loudly and be trivially updated. - The second test covers both hunks independently: the
gettrap case exercises hunk 1, thegetPrototypeOftrap case exercises hunk 2 (and would still segfault with only hunk 1 applied, per the description). - PR states
USE_SYSTEM_BUN=1fails andbun bd testpasses, plus the wider inspect test suite passes. - No prior reviews or comments on the PR.
|
Closing this as a duplicate of #29642, which has the same two changes to forEachPropertyImpl (clear the exception before skipping a property that failed to look up, and stop the walk instead of dereferencing the empty value when getPrototype throws), plus the matching napi fix, and already covers the throwing lazy Bun property, Proxy get trap and getPrototypeOf trap cases in its tests. The crash this PR was opened for (the Bun.$ initializer throwing while the Bun object is formatted) is fixed by that PR as well, so nothing here needs to land separately. |
1 participant
What does this PR do?
Fixes a fuzzer-found assertion failure (and, as it turns out, a release-build segfault) in
JSC__JSValue__forEachPropertyinsrc/jsc/bindings/bindings.cpp, the property walk behindBun.inspect,console.logand the "received" value in matcher error messages.The fuzzer hit it by formatting the
Bunobject while the lazyBun.$initializer threw:Root cause: in the slow path of the walk, when
getPropertySlotreturned false the loop didcontinuewithout clearing the exception that made it return false. That happens when a lazy static property's initializer throws (setUpStaticFunctionSlotreports the property as missing in that case) or when a Proxygettrap on a prototype throws. The exception then stayed pending while the next property was looked up:Bun, the next lazy initializer runs with the exception still pending. Itsto_js_host_callwrapper asserts that a returned value means no exception is pending, which is the crash above (stack:to_js_host_call<BunObject_lazyPropCb_Archive><-reifyStaticProperty<-setUpStaticFunctionSlot<-getPropertySlot<-JSC__JSValue__forEachPropertyImpl). In release builds every later property on the object silently fails its lookup and is dropped from the output: withBun.$throwing,Bun.inspect(Bun)lost 55 properties.getPrototype()bail out and return the empty value, and.getObject()on it dereferences null. Release builds segfault (Segmentation fault at address 0x5) onconsole.log(Object.create(new Proxy({ a: 1 }, { get() { throw new Error() } }))). AgetPrototypeOftrap that throws reaches the same dereference without any stale exception.The fix is two lines of behavior in that function:
getPropertySlotwhether or not it found the property (this is what the siblingforEachPropertyOrderedalready does), andgetPrototype()and stop walking the chain instead of using the empty value. This follows what the fast path in the same function already does for prototype traversal (ignore the trap's exception); the finaltryClearException()in the function clears it.A property whose initializer or trap throws is still left out of the output, matching how the formatter already treats throwing getters; everything else on the object is now printed. The
JSPropertyIteratorpath was checked too: it propagates the exception rather than iterating past it, so it is not affected.How did you verify your code works?
Two tests added to
test/js/bun/util/inspect.test.js, both running the scenario in a child process:Bun.inspect skips a lazy property whose initializer throws and still prints the rest: makesprocess.envthrow (theBun.$initializer reads it, which is the fuzzer's path) and checks that$is the only own property missing fromBun.inspect(Bun). Unfixed release build: 55 properties missing. Unfixed debug build: the assertion above.Bun.inspect survives Proxy traps on the prototype chain that throw: a prototype Proxy with a throwinggettrap and one with a throwinggetPrototypeOftrap. Unfixed release build: exit 139 on the first case; with only the first hunk applied, the second case still fails (UBSan null member call in debug, segfault in release).Both fail with
USE_SYSTEM_BUN=1 bun testand pass withbun bd test. The fuzzer's original script, and a reduced version of it going throughexpect(Bun).toHaveReturnedTimes(), run cleanly on the fixed debug build; the reduced version reproduces the assertion on the unfixed one.inspect.test.js,bun-inspect.test.ts,custom-inspect.test.js,console-table.test.ts,console-log.test.tsandbuiltin-esm-lazy-exports.test.tspass with the debug build. (inspect-error.test.jshas two "Error inside minified file" snapshot failures with a debug build of current main without this change as well; unrelated.)no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js