inspect: clear exceptions thrown by property lookups in forEachProperty

Closed
RO
3mo ago
+167-12

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 getPropertySlot and continued when it returned false. If the lookup returned false because it threw, the exception stayed pending and the walk carried on:

  • A lazy static property whose builder throws (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 (releaseAssertNoException while formatting the Bun object for an ERR_INVALID_ARG_VALUE message).
  • A proxy in the prototype chain whose get trap 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 address 0x5 on Bun.inspect(Object.create(new Proxy({}, { get() { throw 1 } }))) with any keys reported by ownKeys. A getPrototypeOf trap that throws hits the same dereference without needing the first bug.

The fix clears the exception regardless of what getPropertySlot returned (this is already what forEachPropertyOrdered does a few lines down), and stops walking the chain when getPrototype throws.

napi_get_all_property_names had the same getPrototype(...).getObject() chain in its filter loop (found in review). A getOwnPropertyDescriptor trap that throws, or a getPrototypeOf trap that throws on the second walk, segfaulted at the same address there; it now returns napi_pending_exception like the key collection step above it already did. That is the only other site with the pattern under src/jsc/bindings; the fast path of forEachPropertyImpl and forEachPropertyOrdered already handle both cases.

With the walk fixed, the fuzzer script got further and tripped a second debug-only assertion: the Bun.sql / Bun.SQL builders reported a failed internal module load via reportUncaughtExceptionAtEventLoop while that exception was still pending (#if BUN_DEBUG block). Running the uncaught exception handler with an exception pending makes process->get("_fatalException") reify the static entry but report it as missing, which fails a structure assertion in getPropertySlot. Every other caller of reportUncaughtExceptionAtEventLoop clears 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.env on Windows, and the util.inspect function used for inspect.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?

  • The fuzzer script aborts within a second on an unfixed debug build; with this change it runs to the same result as a release build (the ERR_INVALID_ARG_VALUE error, 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 with USE_SYSTEM_BUN=1 (truncated output for the static table case, dead child for the proxy cases) and pass with bun bd test. The static table test inspects Bun at every depth while unwinding a stack overflow and checks that the entry after the last builder that needs JS ($, sql, postgres, SQL all fail there) is still printed. Checked on Linux (debug+ASAN build, and the unfixed release canary prints false) and on Windows x64 (debug build of this branch prints true, the unfixed canary prints false); the first version of this test died on Windows because of the process.env initializer mentioned above.
  • test/napi/napi.test.ts: two new cases under napi_get_all_property_names. The throwing getOwnPropertyDescriptor case is compared against Node; the stateful getPrototypeOf case is Bun only, since Node walks the chain once and never calls the trap a second time. Both segfault with USE_SYSTEM_BUN=1 and pass with the debug build.
  • The rest of inspect.test.js, bun-inspect.test.ts, custom-inspect.test.js, BunObject.test.ts, console-table.test.ts, node-inspect-tests/ and the napi_get_all_property_names describe 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