inspect: clear exceptions thrown by property lookups in forEachProperty - #38921
inspect: clear exceptions thrown by property lookups in forEachProperty#38921robobun wants to merge 4 commits into
Conversation
JSC__JSValue__forEachPropertyImpl's slow path continued to the next property when getPropertySlot returned false, without clearing the exception that made it return false (a lazy static property whose builder threw, or a throwing proxy get trap). The stale exception then leaked into the next lookup: debug builds assert in the next lazy property's host call wrapper, and a proxy anywhere in the prototype chain makes getPrototype return the empty value, which was dereferenced unconditionally. Clear the exception regardless of the result, as forEachPropertyOrdered already does, and stop walking the prototype chain when getPrototype throws. Bun.sql / Bun.SQL builders reported a failed internal module load as an uncaught exception while that exception was still pending (debug builds only), which trips a structure assertion inside the handler once the property walk reaches them with the stack nearly exhausted. The exception is propagated to the caller anyway, so drop the extra report.
|
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 (6)
💤 Files with no reviewable changes (1)
WalkthroughChangesThe PR removes debug-only SQL exception reporting and makes JavaScript and N-API property/prototype traversal handle proxy exceptions safely. Regression tests cover inspection, descriptor traps, and Exception-safe traversal
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 9ad6065 has some failures in 🧪 To try this PR locally: bunx bun-pr 38921That installs a local version of the PR into your bun-38921 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
napi_get_all_property_names walked the prototype chain with the same getPrototype(...).getObject() chain: a getOwnPropertyDescriptor trap that throws made the next getPrototype on the proxy return the empty value, which was then dereferenced. Report the pending exception instead, in both the prototype walk and the own-only branch.
Creating process.env on Windows and the util.inspect function both enter JS from a lazy initializer, which aborts when it fails at the stack limit independently of this change, so set both up before recursing. Check the entry following the last builder that enters JS instead of Archive, which some builds print even without the fix, and drop the assertion that `$` is absent.
There was a problem hiding this comment.
Re-reviewed after the two follow-up commits — both earlier notes are addressed and this pass found no new issues.
napi.cpp's prototype-walk sibling now returns on a throwinggetOwnPropertyDescriptor/getPrototypetrap (both branches), with tests — thegetOwnPropertyDescriptorcase compared against Node, thegetPrototypeOfcase Bun-only with the reason commented.- The static-table inspect test dropped the stack-layout-dependent
$:half; checking only that the entry after the last JS-entering builder appears can't false-positive on a fixed build regardless of which lane first clears the soft stack limit.
Because this is JSC exception-scope handling in the formatter's property walk and in napi_get_all_property_names, and it removes the debug-only reportUncaughtExceptionAtEventLoop calls from the sql/SQL builders, a maintainer glance is still worth having.
Extended reasoning...
Overview
Fixes a fuzzer-found abort/segfault in JSC__JSValue__forEachPropertyImpl (the slow property walk behind Bun.inspect/console.log): getPropertySlot returning false because it threw left the exception pending across the rest of the walk, and getPrototype(globalObject).getObject() on a proxy dereferenced the empty value a throwing trap returns. The fix moves CLEAR_IF_EXCEPTION before the continue (matching forEachPropertyOrdered a few lines below) and splits the prototype step to break on empty. The two #if BUN_DEBUG reportUncaughtExceptionAtEventLoop blocks in defaultBunSQLObject/constructBunSQLObject are removed — running the uncaught-exception handler with an exception still pending trips a getPropertySlot structure assertion, and the exception already propagates via the following RETURN_IF_EXCEPTION.
Since the first review pass, two commits landed. 7144657a applies the same-class fix to napi_get_all_property_names's prototype walk in napi.cpp (my earlier 🟣 note): NAPI_RETURN_IF_EXCEPTION after both getOwnPropertyDescriptor and getPrototype, in both the include_prototypes loop and the own_only branch, plus two tests in module.js/napi.test.ts. b52a0b42 reworks the static-table inspect test (my earlier 🟡 note): it now pre-initializes process.env and the util.inspect custom-symbol path (whose lazy JS init would otherwise abort under stack pressure on Windows), computes afterSQL from Object.getOwnPropertyNames(Bun) at runtime, and asserts only that the entry after the last JS-entering builder appears — no longer depending on $ failing at the first unwind depth that lets Bun.inspect through.
Security risks
None identified. The change swallows exceptions from proxy traps during inspection (already the documented behaviour of the surrounding code — the pre-existing comment reads "Ignore exceptions from Get proxy traps") and propagates them from NAPI (matching Node). No new user-controlled input reaches an allocation or size computation.
Level of scrutiny
High. This is native C++ touching JSC exception scopes on paths that reach user-visible output for every console.log of an exotic object, and NAPI's property-name enumeration. A missed CLEAR_IF_EXCEPTION/RETURN_IF_EXCEPTION here is exactly the class of bug REVIEW.md flags as most-blocked. The changes are small and each is locally correct against the surrounding pattern, but the choice between swallow (inspect) and propagate (NAPI) and the removal of a debug-only diagnostic hook are judgment calls a maintainer should sign off on.
Other factors
- Tests cover both proxy variants (release-build segfault → deterministic output) plus the debug-build assertion via a stack-overflow-then-inspect child process; the NAPI
getOwnPropertyDescriptorcase ischeckSameOutputagainst Node, and thegetPrototypeOfcase is Bun-only with the divergence reason ("Node only walks once") commented in the fixture. Object.getOwnPropertyNames(Bun)in the test enumerates static-table names without reifying values, so the builders are still lazy whenBun.inspect(Bun)runs during unwind.- The napi fix uses
!proto.isObject()on theJSValuebeforeasObject, so the empty-value case and a non-object prototype both break cleanly; a successfulgetOwnPropertyDescriptor(loop condition false) can't leave an exception pending in JSC, so no check is needed on that exit.
|
Pushed two follow-ups. CI (Windows lanes): the static table test died with exit code 9 on both Windows lanes. That is the low byte of Review comments: agreed on both.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
LGTM — both earlier review points are addressed (the $: precondition assertion is dropped, and the sibling getPrototype(...).getObject() chain in napi_get_all_property_names now returns napi_pending_exception with tests for both trap variants).
What was reviewed: the CLEAR_IF_EXCEPTION reorder in forEachPropertyImpl matches what forEachPropertyOrdered already does; the getPrototype split with an empty-value guard before .getObject() removes the null-cell dereference. The napi filter loop now checks for exceptions after both getOwnPropertyDescriptor and getPrototype (and in the own_only branch), using the same NAPI_RETURN_IF_EXCEPTION macro as the key-collection step above it. The BunObject.cpp change removes a debug-only reportUncaughtExceptionAtEventLoop that ran with the exception still pending — the RETURN_IF_EXCEPTION on the next line still propagates it.
Extended reasoning...
Overview
Six files touched. Native: bindings.cpp (JSC__JSValue__forEachPropertyImpl slow path — clear the exception regardless of what getPropertySlot returned, and split getPrototype().getObject() so a throwing getPrototypeOf trap breaks the walk instead of dereferencing an empty JSValue), napi.cpp (napi_get_all_property_names filter loop — NAPI_RETURN_IF_EXCEPTION after getOwnPropertyDescriptor and getPrototype, plus the own_only branch), and BunObject.cpp (drops two debug-only reportUncaughtExceptionAtEventLoop calls that ran with the exception still pending). Tests: three subprocess-spawned inspect.test.js cases and two napi_get_all_property_names cases (one compared against Node, one Bun-only for the second-walk getPrototypeOf throw).
Security risks
None. This hardens against hostile Proxy traps that previously segfaulted the process at address 0x5; the fix clears/propagates the exception rather than introducing any new user-code entry points. No auth, crypto, or permission surface.
Level of scrutiny
Moderate-high — this is C++ in JSC bindings, memory-safety-adjacent. But each change is small (~20 native lines total) and copies the exact pattern already used one level over: forEachPropertyOrdered already clears unconditionally, and the napi key-collection step in the same function already uses NAPI_RETURN_IF_EXCEPTION. The proto.isObject() / asObject(proto) pair is safe because the exception check runs first, so proto is never the empty value. The BunObject.cpp deletion is debug-only and the exception still propagates via RETURN_IF_EXCEPTION immediately after.
Other factors
I reviewed an earlier revision and left two comments (a stack-layout-dependent test assertion, and the same-class sibling in napi.cpp); both are addressed in this revision with follow-up commits and new tests. The author verified the static-table test on Linux and Windows x64 against both this branch and the unfixed canary, and the two Proxy tests deterministically cover the release-build segfault. The napi getOwnPropertyDescriptor case is compared against Node. The bug-hunting pass on this revision found nothing.
|
CI status for the record: the re-run (#97823) has no test failures (178 jobs passed, the handful of flaky entries all passed on retry). The build is marked failed only because the |
1 participant
What does this PR do?
Fixes a fuzzer-found abort in the property walk behind
Bun.inspect/console.log(JSC__JSValue__forEachPropertyImpl, the slow path used for objects with static tables and for prototype chains containing exotic objects).The loop looked each property up with
getPropertySlotandcontinued when it returned false. If the lookup returned false because it threw, the exception stayed pending and the walk carried on:Bun.$calls into JS to build the shell object, which throws a stack overflow when the formatter runs right after unwinding one) leaves its exception pending; the next entry in the table (Bun.Archive) is reified through the Rust host call wrapper, whose exception scope then asserts. This is the fuzzer crash (releaseAssertNoExceptionwhile formatting theBunobject for anERR_INVALID_ARG_VALUEmessage).gettrap throws does the same thing. Once the exception is pending, the prototype step at the end of the loop (iterating->getPrototype(globalObject).getObject()) gets the empty value back from the proxy and dereferences it: release builds segfault at address0x5onBun.inspect(Object.create(new Proxy({}, { get() { throw 1 } })))with any keys reported byownKeys. AgetPrototypeOftrap that throws hits the same dereference without needing the first bug.The fix clears the exception regardless of what
getPropertySlotreturned (this is already whatforEachPropertyOrdereddoes a few lines down), and stops walking the chain whengetPrototypethrows.napi_get_all_property_nameshad the samegetPrototype(...).getObject()chain in its filter loop (found in review). AgetOwnPropertyDescriptortrap that throws, or agetPrototypeOftrap that throws on the second walk, segfaulted at the same address there; it now returnsnapi_pending_exceptionlike the key collection step above it already did. That is the only other site with the pattern undersrc/jsc/bindings; the fast path offorEachPropertyImplandforEachPropertyOrderedalready handle both cases.With the walk fixed, the fuzzer script got further and tripped a second debug-only assertion: the
Bun.sql/Bun.SQLbuilders reported a failed internal module load viareportUncaughtExceptionAtEventLoopwhile that exception was still pending (#if BUN_DEBUGblock). Running the uncaught exception handler with an exception pending makesprocess->get("_fatalException")reify the static entry but report it as missing, which fails a structure assertion ingetPropertySlot. Every other caller ofreportUncaughtExceptionAtEventLoopclears first, and the exception is propagated to whoever touched the property anyway, so the block is removed rather than reworked.Not changed here: two lazy initializers that enter JS (
process.envon Windows, and theutil.inspectfunction used forinspect.custom) still abort the process when they fail at the stack limit. That is independent of the walk (it reproduces on main without any of this) and is tracked separately; the static table test below sets both up before recursing so it does not run into them.How did you verify your code works?
ERR_INVALID_ARG_VALUEerror, exit code 1, no assertions). The Proxy variants segfault on the current canary release build and print{ b: 2 }/{}with the fix.test/js/bun/util/inspect.test.js: three tests, each run in a child process. All three fail withUSE_SYSTEM_BUN=1(truncated output for the static table case, dead child for the proxy cases) and pass withbun bd test. The static table test inspectsBunat every depth while unwinding a stack overflow and checks that the entry after the last builder that needs JS ($,sql,postgres,SQLall fail there) is still printed. Checked on Linux (debug+ASAN build, and the unfixed release canary printsfalse) and on Windows x64 (debug build of this branch printstrue, the unfixed canary printsfalse); the first version of this test died on Windows because of theprocess.envinitializer mentioned above.test/napi/napi.test.ts: two new cases undernapi_get_all_property_names. The throwinggetOwnPropertyDescriptorcase is compared against Node; the statefulgetPrototypeOfcase is Bun only, since Node walks the chain once and never calls the trap a second time. Both segfault withUSE_SYSTEM_BUN=1and pass with the debug build.inspect.test.js,bun-inspect.test.ts,custom-inspect.test.js,BunObject.test.ts,console-table.test.ts,node-inspect-tests/and thenapi_get_all_property_namesdescribe block pass locally.no test proof · iteration 1 · 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 test/napi/napi.test.ts