Repository navigation
Conversation
… a throwing getPrototypeOf trap JSC__JSValue__forEachPropertyImpl skipped a property whose getPropertySlot returned false without clearing the exception that made it return false (a lazy static property initializer or a Proxy trap that threw). The exception stayed pending while the remaining properties were visited: every later static property was reported as missing and dropped from the output, and debug builds hit releaseAssertNoException as soon as the next initializer ran. The prototype walk at the end of the loop also called getObject() on the empty value returned when a Proxy getPrototypeOf trap throws, which is a null dereference. Stop the walk instead; the exception is cleared on exit like the other ignored getter exceptions.
|
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; 0 remain after this review. Walkthrough
ChangesInspection exception handling
Possibly related PRs
Suggested reviewers: 🚥 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 — two targeted exception-handling fixes that match existing sibling patterns.
What was reviewed:
- Reordering
CLEAR_IF_EXCEPTIONbeforecontinuematchesforEachPropertyOrderedat bindings.cpp:5814-5816. - The
getPrototypeempty-check mirrors the fast path at bindings.cpp:5601-5611; the trailingtryClearExceptionat 5732 clears the trap's exception as the comment states. - Tests spawn isolated subprocesses, drain pipes concurrently, and assert the combined
{stdout, stderr, exitCode}object per harness conventions.
Extended reasoning...
Overview
Two small edits to JSC__JSValue__forEachPropertyImpl in src/jsc/bindings/bindings.cpp (the slow-path property walk behind console.log/Bun.inspect), plus two subprocess-based regression tests in test/js/bun/util/inspect.test.js.
Fix 1 moves CLEAR_IF_EXCEPTION(scope) to before the if (!hasProperty) continue; so a throwing lazy initializer or Proxy get trap doesn't leave an exception pending across iterations. This is the exact ordering JSC__JSValue__forEachPropertyOrdered already uses (bindings.cpp:5814-5816), so this is bringing a sibling into line rather than introducing a new pattern.
Fix 2 checks getPrototype()'s return for empty before calling .getObject() on it, breaking out of the loop when a Proxy getPrototypeOf trap throws. The fast path directly above (bindings.cpp:5601-5611) already guards this the same way, and the function's trailing scope.tryClearException() (5732-5735) clears the pending exception on the way out, so the added comment is accurate.
Security risks
None. This is inspect/formatting code; the fix removes a null-deref segfault and a pending-exception leak, both of which are strictly defensive improvements. No new user input surface, no auth/crypto/permission logic.
Level of scrutiny
Low-to-medium. The changes are ~8 lines of C++ in a well-understood function, each mirroring an existing pattern from a sibling code path in the same file. Both bugs have concrete crash reproductions (segfault at 0x5, debug releaseAssertNoException), and the PR description traces the mechanism precisely.
Other factors
The tests follow repo conventions well: subprocess isolation via bun -e, bunEnv/bunExe, concurrent pipe draining with Promise.all, and a single combined-object assertion. describe.concurrent is appropriate since both tests spawn independent subprocesses. The first test's reliance on Bun.$ reading process.env during initialization is a bit indirect, but the PR explains why (it's the actual fuzzer-found repro path) and the assertion checks specific property names rather than generic "no crash" output. The PR also explicitly scopes out the separate Bun.sql/Bun.SQL debug-assert issue rather than papering over it.
…roxy get trap case On Windows process.env is a Proxy whose target has a custom inspect function, and formatting it loads node:util's inspect helpers, which read process.env. Load them before the test makes process.env throw so that only the Bun.$ initializer throws during the walk. Also cover the same leak through a Proxy get trap that throws for one inherited property.
There was a problem hiding this comment.
LGTM. The two fixes are small and mechanically correct — the reordered clear-before-continue now matches forEachPropertyOrdered at bindings.cpp:5813-5818, and the split getPrototype guard mirrors the fast path's existing empty-check at :5601. The inline nit about the identical chain in napi.cpp is pre-existing and in a separate subsystem; worth a follow-up or a one-line exclusion note, but not blocking.
Extended reasoning...
Overview
Two exception-handling fixes in JSC__JSValue__forEachPropertyImpl (src/jsc/bindings/bindings.cpp), the slow-path property walker behind console.log / Bun.inspect, plus three subprocess regression tests in test/js/bun/util/inspect.test.js.
- Fix 1 reorders
CLEAR_IF_EXCEPTION(scope)to run before theif (!hasProperty) continue, so an exception from a lazy static initializer or Proxy trap that madegetPropertySlotreturn false doesn't leak into subsequent iterations. This is exactly the orderingforEachPropertyOrderedalready uses (lines 5813-5818). - Fix 2 splits
iterating->getPrototype(globalObject).getObject()so an emptyJSValue(returned when a ProxygetPrototypeOftrap throws) breaks the loop instead of null-derefing insideJSCell::getObject(). The pending exception is then cleared by the existing block at :5732-5735. The fast path at :5601 already guardsgetPrototypethe same way.
Security risks
None. This is output-only formatting code; the change replaces a segfault and a silent property-drop with graceful exception clearing. No new user input parsing, no allocation sizing, no trust boundary.
Level of scrutiny
Medium — native JSC bindings code with exception scopes, but the change is ~10 lines, purely local, and each half copies an existing pattern from a sibling site in the same file. There is no new control flow beyond an early break/continue and no lifetime or GC change.
Other factors
- Tests follow harness conventions:
bunEnv/bunExe, concurrent pipe drain, combined{stdout, stderr, exitCode}assertion,describe.concurrentfor independent subprocesses. They cover both fixes (throwing lazy initializer, throwinggettrap on an inherited property, throwinggetPrototypeOftrap) and exercise bothBun.inspectandconsole.log. - The first test depends on
Bun.$readingprocess.envduring initialization, which is an internal detail, but the comment explains it and the second test covers the same exception-leak path via a plain Proxy without that dependency. - The bug hunter flagged one sibling
getPrototype(globalObject).getObject()chain innapi.cpp:2080. It's the only other instance insrc/, is pre-existing, lives in a separate subsystem (N-API property enumeration), and this PR doesn't make it worse. Per REVIEW.md the author should either apply the same 3-line guard there or note the exclusion, but it's a nit rather than a blocker for these two crash fixes.
…criptor or prototype lookup throws The descriptor filter loop had the same problem as forEachProperty: a Proxy getOwnPropertyDescriptor trap that throws left the exception pending, the following getPrototype returned the empty value and getObject() dereferenced null. In own_only mode the pending exception reached NAPI_RETURN_SUCCESS instead. Check after both calls, which is also what Node returns in this situation.
robobun
changed the title
There was a problem hiding this comment.
LGTM — the napi.cpp sibling from the earlier review is now fixed with a matching checkSameOutput test, and the core forEachPropertyImpl change is a minimal, well-tested crash fix. The remaining inline note about util.isError in NodeUtilTypesModule.cpp is a pre-existing same-class site in a file this PR doesn't touch; folding it in or adding it to the "tracked separately" list would close out the class, but it doesn't block.
Extended reasoning...
Overview
Two exception-handling fixes in JSC__JSValue__forEachPropertyImpl (bindings.cpp): reorder CLEAR_IF_EXCEPTION before the continue when getPropertySlot returns false, and guard getPrototype() returning empty before .getObject(). After the previous review round, the same guard was applied to napi_get_all_property_names (napi.cpp) with NAPI_RETURN_IF_EXCEPTION on both getOwnPropertyDescriptor paths and the prototype step. Three subprocess tests in inspect.test.js and a checkSameOutput napi test cover both.
Security risks
None. Fixes a null-deref segfault reachable via hostile Proxy traps in console.log/Bun.inspect and napi_get_all_property_names — strictly a robustness improvement.
Level of scrutiny
Medium. C++ JSC exception handling is sensitive, but both changes are the standard local pattern: the CLEAR_IF_EXCEPTION reorder mirrors what forEachPropertyOrdered already does a few hundred lines below, and the split-then-check-empty on getPrototype matches the safe idiom already used at bindings.cpp:5506/5601. The napi.cpp early returns are through NAPI_RETURN_IF_EXCEPTION (returns napi_pending_exception), and everything acquired above them (exportKeys, filteredKeys) is GC-managed, so no cleanup is skipped.
Other factors
The author addressed the prior review feedback in full — napi.cpp fix plus a Node-parity test via checkSameOutput. The new inspect tests follow harness conventions (subprocess spawn, bunEnv, drain all pipes, assert combined {stdout, stderr, exitCode}), and the PR description already documents two adjacent pre-existing issues as tracked separately, which is the same treatment the remaining NodeUtilTypesModule.cpp nit warrants if not folded in here.
…it throws The VMInquiry slot used for the Symbol.toStringTag lookup was still alive when getPrototype() ran, so a Proxy trap could not be entered: debug builds crashed on the disallowed VM entry, release builds got a TypeError and then dereferenced the empty value that getPrototype returned. Release the slot's restriction after the lookup and check for an exception after getPrototype().
robobun
changed the title
There was a problem hiding this comment.
Both earlier same-class sites (napi.cpp and jsFunctionIsError) are now fixed. Beyond the inline nit, I checked that slot.disallowVMEntry.reset() in jsFunctionIsError is placed correctly — the following slot.getValue() is gated on slot.isValue() so it doesn't enter the VM, and the reset is needed before getPrototype can run a trap. Also checked that the new NAPI_RETURN_IF_EXCEPTION calls in napi.cpp don't introduce a new path that reads desc uninitialized (they return early, and the pre-existing if (!proto) break shape is unchanged).
Extended reasoning...
Both prior review comments were addressed by commits a414f0f (napi.cpp) and 71aa532 (NodeUtilTypesModule.cpp). The remaining finding this round is a test-isolation nit. Given this is native JSC exception-handling / crash-fix code across three files, deferring the final sign-off to a human; the note above records what was additionally verified so it isn't re-explored.
|
Closing as a duplicate of #29642, which fixes the same stale-exception / getPrototype null dereference in forEachPropertyImpl (the same two hunks in bindings.cpp) and now also carries the consolidated regression tests. |
|
To complete the picture for the version of this PR that was closed (it had grown since it was first compared): the napi_get_all_property_names checks here are the same ones already in #29642 (with napi tests), and the util.isError fix (NodeUtilTypesModule.cpp) is a separate bug that #37202 is being kept open for. Nothing from this PR is left without an open owner. |
There was a problem hiding this comment.
LGTM — all three prior review comments have been addressed (napi.cpp sibling, jsFunctionIsError, and spawning the util.isError crash test in a child process).
What was reviewed:
- The four exception-handling fixes each follow the pattern already used at sibling sites (
CLEAR_IF_EXCEPTIONreordering matchesforEachPropertyOrdered;NAPI_RETURN_IF_EXCEPTIONmatches the check at :2065;RETURN_IF_EXCEPTIONaftergetPrototypematches the other guarded sites in bindings.cpp). slot.disallowVMEntry.reset()is placed after the VMInquiry lookup and beforegetPrototype;slot.getValueinside theif (slot.isValue())branch reads the stored value without re-entering the VM, so resetting first is safe.- All five new crash tests run the crashing input in a spawned child and assert on
{stdout, stderr, exitCode}; the napi test compares against Node viacheckSameOutput.
Extended reasoning...
Overview
This PR fixes four instances of the same bug class — getPrototype() on a Proxy returning the empty JSValue when a trap throws, followed by a null dereference — across three C++ files: forEachPropertyImpl in bindings.cpp (two fixes: reorder CLEAR_IF_EXCEPTION before continue, and check for empty prototype before .getObject()), napi_get_all_property_names in napi.cpp (add NAPI_RETURN_IF_EXCEPTION after both descriptor lookups and the prototype step), and jsFunctionIsError in NodeUtilTypesModule.cpp (release the VMInquiry slot's disallowVMEntry and add RETURN_IF_EXCEPTION after getPrototype). Five new tests cover each fix as a spawned crash fixture.
Security risks
None. These are defensive exception checks that prevent null dereferences on adversarial Proxy inputs. No auth, crypto, or permissions code touched.
Level of scrutiny
Medium — native C++ in JSC exception-handling paths. However, every added line follows the exact pattern already used at neighboring sites in the same function or file, and the changes are purely additive (no removed guards, no changed control flow beyond the reordered CLEAR_IF_EXCEPTION). The disallowVMEntry.reset() addition is the standard JSC idiom for releasing a VMInquiry slot before code that may enter the VM; the author's response to my second review comment demonstrated they investigated the actual failure mechanism (the VMInquiry slot was blocking the trap from running at all) rather than applying my suggested fix verbatim.
Other factors
This PR went through three rounds of my review feedback, each addressed correctly with a confirming reproduction and a targeted fix commit. The PR description enumerates the remaining getPrototype call sites in src/ and explains why each is either already guarded or unreachable with a Proxy, and explicitly tracks the two related pre-existing issues (napi_get_prototype returning napi_ok with an empty handle, and the Bun.sql initializer assertion) as separate work. Test coverage is thorough: each crash test spawns a child process, drains all pipes concurrently, asserts on the combined {stdout, stderr, exitCode} object, and the napi test compares byte-for-byte against Node.
…p or getPrototypeOf throws during the property walk (#29642) ### Problem - `Bun.inspect()`, `console.log()` and `expect()` failure output crash with `Segmentation fault at address 0x5` (debug builds: UBSan "member call on null pointer of type 'JSC::JSCell'" in `JSCJSValueCell.h`) when a property lookup throws while an object is being formatted. Release repro: ```js const proto = new Proxy({ a: 1 }, { getPrototypeOf() { throw new Error("boom"); } }); console.log(Object.create(proto)); ``` and likewise with a Proxy whose `get` trap (or a getter reached through a Proxy) throws for one property. - The same happens with a lazily initialized property of the `Bun` object whose initializer throws (fuzzer sample: `globalThis.Symbol` replaced, then `Bun.inspect(Bun)`; the builtin behind `Bun.$` calls `Symbol("cwd")`). Release builds print `Bun` with most of its properties missing (72 of 115 on the shipped build); debug builds abort with `ASSERTION FAILED: Unexpected exception observed` / `Symbol is not a function. (In 'Symbol("cwd")' ...)` when the next lazy property is built. - Plain-JavaScript variant of the same leak: `console.log` of a module namespace during an import cycle, when one export is still in its temporal dead zone, throws `ReferenceError: Cannot access 'x' before initialization` out of `console.log` (the stale exception is picked up while the next export is formatted). `util.inspect` prints such an export as `<uninitialized>`. - Cause, in the slow path of `JSC__JSValue__forEachPropertyImpl` (`src/jsc/bindings/bindings.cpp`): - `object->getPropertySlot()` reports a throwing Proxy trap, a throwing lazy initializer or a TDZ namespace export as "not found" with the exception still pending, and the loop `continue`d before the `CLEAR_IF_EXCEPTION` below it. The following lookups and formatting callbacks then run with that exception pending (the dropped properties, the rethrown `ReferenceError`, the debug assertion). - When the walk moves to the next prototype, `iterating->getPrototype(globalObject).getObject()` runs `getObject()` on the empty `JSValue` that `getPrototype` returns when it threw (either because of the stale exception above or because the `getPrototypeOf` trap itself throws). The empty value passes `isCell()`, so this reads the type byte of a null cell: the fault at address 5. - `napi_get_all_property_names` (`src/jsc/bindings/napi.cpp`, descriptor filter loop) has the same `getPrototype().getObject()` chain after an unchecked `getOwnPropertyDescriptor`, so a Proxy trap throwing there returned `napi_ok` with an exception pending in own-only mode and segfaulted in include-prototypes mode. - `defaultBunSQLObject` / `constructBunSQLObject` (`src/jsc/bindings/BunObject.cpp`) had a debug-only block that handed a sql module load failure to `reportUncaughtExceptionAtEventLoop` while the exception was still pending on the VM, so `globalThis.Symbol = NaN; Bun.sql` (and the fuzzer sample above, once the walk gets past `Bun.$`) aborted debug builds with `ASSERTION FAILED: ... object->structure() == this` in `Structure::storedPrototype` instead of throwing. ### Fix - `bindings.cpp`: clear the exception after `getPropertySlot` regardless of its result, which is what the ordered variant `JSC__JSValue__forEachPropertyOrdered` already does; read the next prototype into a `JSValue`, clear the exception and stop the walk when it is empty. (An earlier revision also held the prototype being walked under an `EnsureStillAliveScope`; dropped, since the raw pointer is used after every call into JS in the loop body and so is live across them anyway.) - Behaviour change to note: a property whose lookup throws is now left out of the output instead of the whole `console.log` / `Bun.inspect` call throwing or crashing. For TDZ namespace exports this differs from `util.inspect`'s `<uninitialized>`; printing that marker would be a formatter feature on top of this fix and is not attempted here. - `napi.cpp`: check for an exception after `getOwnPropertyDescriptor` and after `getPrototype` and return `napi_pending_exception`, which is what Node returns for these cases. - `BunObject.cpp`: drop the debug-only report. The exception is propagated to the reader by the `RETURN_IF_EXCEPTION` right below it, so debug builds now behave like release builds (`Bun.sql` throws). - Why this is the right place: the formatter deliberately swallows errors thrown by individual properties (getters, traps) and prints the rest of the object; these two sites were the only ones in the walk that acted on a "not found" result or a prototype value before clearing the exception that produced it. Skipping just the property (or stopping at just the prototype) whose lookup threw is the existing behaviour for every other throw site in this function. - Verification: - `test/js/bun/util/inspect.test.js`, "Bun.inspect when a property lookup throws" (5 spawned cases): a Proxy `get` trap, a getter behind a Proxy, a `getPrototypeOf` trap, a throwing lazy `Bun` property, and a two-file import cycle with a TDZ export. Without the fix (shipped release build and an unfixed debug build) all five fail: the Proxy children segfault / fail UBSan, the `Bun` child prints `[false,false,...]` on release and aborts on debug, the cycle child exits 1 with the `ReferenceError`; with it each prints everything except the one property whose lookup threw. - `test/js/bun/util/BunObject.test.ts`, "a lazy property whose builtin fails to load throws from the read": `Bun.$` / `sql` / `SQL` / `postgres` with `Symbol` broken throw a `TypeError` on two consecutive reads. Aborts on an unfixed debug build (the `BunObject.cpp` hunk); passes on release either way, as the removed block is debug-only. The fixture builds `process.env` before breaking `Symbol` because the `$` builder reads it, and building it on Windows reifies another `Bun` property mid-lookup, which on a Windows debug build would hit the separate `storedPrototype` assertion that #37001 fixes (verified on Linux only). - `test/napi/napi.test.ts`: `getOwnPropertyDescriptor` trap throwing in own-only and include-prototypes mode (compared against Node), and a `getPrototypeOf` trap that throws on the second call so the check after `getPrototype` is the one that fires. - Repros above and the tests also run clean under `BUN_JSC_validateExceptionChecks=1`. ### Background - `forEachPropertyImpl` is the property walk behind Bun's native formatter. It collects the property names of the object and of up to five prototypes, looks each one up through the original object with `getPropertySlot`, and hands the value to a callback that formats it. Errors thrown by individual properties are swallowed on purpose so that one bad getter does not make `console.log` throw. - A JSC exception is "pending" state on the VM, not C++ unwinding. A function that throws returns a failure value (`false`, or the empty `JSValue`) and leaves the exception on the VM; until something clears or rethrows it, most JSC entry points return early as soon as they are called, and debug builds assert when a function that did not throw is observed returning with an exception pending. `CLEAR_IF_EXCEPTION` drops the pending exception. - The empty `JSValue` (`JSValue()`) is encoded as 0. `isCell()` is true for it, so `getObject()` on it dereferences a null cell pointer rather than returning null; callers have to test the value itself first. - Lazy properties of the `Bun` object are entries in a static property table whose value is produced by a builder the first time the property is read (`PropertyCallback`). Some builders evaluate built-in JavaScript modules (the shell for `Bun.$`, the sql module for `Bun.sql`), so they can throw when that module fails to evaluate, and JSC reports that to the reader as "property not found" plus a pending exception. ### Consolidated duplicates Found repeatedly by the fuzzer (fingerprint `d678cafe50a2ad6e`). Earlier round, folded in here in April: #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325. This round, closed in favour of this PR: #30099 #30245 #37160 #37175 #37213 #37256 #37428 #38700 #38921 #39363 #39365 #39380 #39412 #39413 (and #39411, closed earlier). The `BunObject.cpp` hunk and the `BunObject.test.ts` test come from #30245 / #37428; the same hunk is also part of #37001, which fixes the underlying `storedPrototype` assertion in JSC. Related fixes that are not part of this bug and stay open on their own: #39382 (a custom inspect function when `node:util` fails to load), #37202 (`util.isError` with a throwing `getPrototypeOf` trap), #37331 (`forEachPropertyOrdered` when the callback throws), #37001 (stale structure in `JSObject::getPropertySlot`), #32263 (additional checks in the same napi loop). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 11 · 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 <!-- robobun:evidence:end --> --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
What does this PR do?
Fixes two exception handling bugs in
JSC__JSValue__forEachPropertyImpl(src/jsc/bindings/bindings.cpp), the property walk behindconsole.log/Bun.inspectfor objects that take the slow path (objects with a static property table such asBun, objects with a Proxy in their prototype chain, ...), plus the one other place with the same prototype step (napi_get_all_property_names).When
getPropertySlotreturned false, the loop didcontinuebefore theCLEAR_IF_EXCEPTIONthat follows it.getPropertySlotreturns false with an exception pending when a lazy static property initializer or a Proxy trap throws, so that exception stayed pending for the rest of the walk. Every later lookup then fails as well (setUpStaticFunctionSlotand the Proxy trap code both bail out on a pending exception), so the remaining properties were silently dropped from the output: on a release build,Bun.inspect(Bun)with a throwingBun.$initializer loses everything fromArchivethroughinflateSync, and an inherited Proxy property whosegettrap throws hides every property after it. On debug builds the next initializer that is implemented in Rust tripsassert_exception_presence_matches, which is the crash this was found through:(
Symbol("cwd")is the first line of theBun.$initializer insrc/js/builtins/shell.ts; the fuzzer had replacedSymbolearlier and then formatted theBunobject.)The fix is to clear the exception before the
continue, the same orderJSC__JSValue__forEachPropertyOrderedalready uses.At the end of each prototype step the loop did
iterating->getPrototype(globalObject).getObject(). Wheniteratingis a Proxy andgetPrototypethrows (itsgetPrototypeOftrap throws, or an exception leaked by bug 1 is still pending), it returns the empty value, andgetObject()on it dereferences null:The walk now stops there; the exception is cleared on the way out like the other ignored getter exceptions in this function (and like the fast path already does for the same trap).
napi_get_all_property_names(src/jsc/bindings/napi.cpp) has the samegetPrototype(...).getObject()step in its descriptor filter loop (pointed out in review). Withnapi_key_include_prototypesand an enumerable/writable/configurable filter, a Proxy in the prototype chain whosegetOwnPropertyDescriptortrap throws leaves the exception pending, the followinggetPrototypereturns empty andgetObject()dereferences null; withnapi_key_own_onlythe pending exception instead reachedNAPI_RETURN_SUCCESS(an assertion on debug builds). Both lookups are now followed byNAPI_RETURN_IF_EXCEPTION, so the call returnsnapi_pending_exceptionand the trap's exception propagates to the caller, which is what Node does.util.isError(jsFunctionIsErrorinsrc/jsc/modules/NodeUtilTypesModule.cpp, also pointed out in review) readsgetPrototype()of its argument and dereferences the result without checking it.util.isError(new Proxy({}, { getPrototypeOf() { throw ... } }))segfaults on the release canary. There were two things wrong: theVMInquiryslot used for theSymbol.toStringTaglookup a few lines earlier was still alive, so the Proxy trap was not allowed to be entered at all (debug builds crash right there with "VM entry disallowed"; release builds get a TypeError instead of running the trap), and the empty value returned on that exception was then dereferenced. The slot's restriction is now released once the lookup is done (slot.disallowVMEntry.reset(), the usual JSC idiom) andgetPrototype()is followed byRETURN_IF_EXCEPTION, so the trap runs and whatever it throws propagates to the caller, which is also what the old Node implementation (instanceof-based) did.I went through the other
getPrototypecall sites insrc/(C++ and the Rustget_prototypecallers) after that. The rest either already check for the exception (WebStreamsInspectCustom.cpp,ObjectBindings.cpp, the deepEquals prototype comparison), or can only be reached with ordinary objects whose[[GetPrototypeOf]]cannot throw (JSBuffer__isBufferon aJSUint8Array,isAsyncFunction/isGeneratorFunctionon aJSFunction, the sqlite constructor setup, the test runner's scope functions, and the console / test-runner formatters, which never format a Proxy itself, only its target). The one remaining site with a real difference isnapi_get_prototype(src/runtime/napi/napi_body.rs), which returnsnapi_okplus a handle to the empty value instead ofnapi_pending_exceptionwhen the trap throws; it does not dereference anything, so it is left out of this PR and tracked separately.Two more pre-existing problems that these tests brushed against are tracked separately and not touched here: the
Bun.sql/Bun.SQLinitializers report a module load failure while the exception is still pending (debug builds only, soglobalThis.Symbol = NaN; console.log(Bun)still asserts on a debug build after this change, now inStructure::storedPrototypewhen the walk reachessql), and formatting a value with a custom inspect function aborts if loadingnode:util's inspect helpers throws (which is what happens toprocess.envon Windows onceprocess.envhas been made to throw; the first test below loads the helpers first to stay clear of it).How did you verify your code works?
Three tests added to
test/js/bun/util/inspect.test.js(exceptions thrown while walking properties), each running its snippet in a childbun -e:a lazy property initializer that throws only hides that property: makesprocess.envthrow (theBun.$initializer reads it), then checks thatBun.inspect(Bun)still listsArchiveandversion. Before the fix it fails with thereleaseAssertNoExceptionabort above on a debug build and printsfalse false true(Archive missing) on release builds, on Linux and Windows.a get trap that throws for one inherited property only hides that property: before the fix, on the Linux release build, the leaked exception hides the other properties and the walk then segfaults in the prototype step.a throwing getPrototypeOf trap in the prototype chain stops the walk: before the fix this segfaults on the Linux release build and is a UBSan null member call on the debug build.One test added to
test/napi/napi.test.ts(propagates an exception thrown by a Proxy trap while filtering, fixturetest_get_all_property_names_throwing_trapinnapi-app/module.js), which compares Bun's output with Node's for both key modes. Before the napi change the own_only case aborts inNAPI_RETURN_SUCCESSon the debug build and the include_prototypes case is the same UBSan null member call inJSValue::getObject.One test added to the
isErrorblock oftest/js/node/util/util.test.js(propagates an exception from a getPrototypeOf trap), which runs the call in a childbun -e. Before the change the child aborts on the debug build and segfaults with the release canary (exit 139), so only this test fails.All of these pass with
bun bd test test/js/bun/util/inspect.test.js,bun bd test test/napi/napi.test.tsandbun bd test test/js/node/util/util.test.js, as do the rest of those files; the three repro scripts are also clean underBUN_JSC_validateExceptionChecks=1. The fixed debug build runs the original fuzzer script cleanly.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