Fix crashes when inspecting objects whose property enumeration throws

Closed
RO
3mo ago
+302-108

What does this PR do?

Fixes a crash found by fuzzing (flaky under the fuzzer because it depends on global state from earlier iterations in the same process; deterministic repro below), plus several more crashes in the same family that surfaced while fixing it.

Inspecting an object can run arbitrary JS: lazy property initializers on native objects, getters, and proxy traps. The property enumeration loop used by console.log and Bun.inspect (JSC__JSValue__forEachPropertyImpl) mishandled exceptions from that JS in two places, and several things downstream of it crashed once those were fixed.

1. Exception leaked across loop iterations. When getPropertySlot reported not-found with an exception pending (a lazy property initializer threw), the continue skipped CLEAR_IF_EXCEPTION, so the next property's initializer ran with the exception still set and tripped exception scope verification in debug builds:

globalThis.Symbol = -6;
console.log(Bun); // aborts debug builds: Bun.$'s initializer evaluates shell.ts, which calls Symbol("cwd")

ASSERTION FAILED: Unexpected exception observed on thread ...
Error Exception: Symbol is not a function. (In 'Symbol("cwd")', 'Symbol' is -6)
!exception()
ExceptionScope.h(62) : void JSC::ExceptionScope::releaseAssertNoException()

In release builds the stale exception silently made later properties vanish from the output. The fix clears the exception before the continue, matching what JSC__JSValue__forEachPropertyOrdered already does.

2. Null deref in the prototype walk. The same loop advanced with iterating->getPrototype(globalObject).getObject(). A Proxy getPrototypeOf trap that throws makes getPrototype return an empty JSValue, and getObject() on an empty value calls a member function on a null JSCell. This crashes release builds:

const p = new Proxy({ a: 1 }, { getPrototypeOf() { throw new Error("trap"); } });
console.log(Object.create(p)); // SIGSEGV in current releases

The same pattern had two other live occurrences: the prototype walk in napi_get_all_property_names (napi.cpp, both the include_prototypes and own_only arms now check for a pending exception after each trap) and util.isError (NodeUtilTypesModule.cpp). The util.isError one had a second problem at the same line: a VMInquiry PropertySlot was still alive when getPrototype ran the trap, and a live VMInquiry slot forbids VM entry (debug builds abort in checkVMEntryPermission; in release the blocked entry is what produced the empty value). The slot is now scoped to die before the prototype read. util.isError(new Proxy({}, { getPrototypeOf() { throw 0 } })) segfaults current releases.

3. Debug-only re-entry with a pending exception, and the Bun.sql module tree. defaultBunSQLObject and constructBunSQLObject called reportUncaughtExceptionAtEventLoop while the exception was still pending; that re-enters JS and trips a structure assertion. Every other caller clears first and these two propagate right after, so the calls are removed. With them gone, the sql tree had the same hazard as item 4 below: internal/sql/shared.ts read Bun.env at module scope, which reifies a property on the Bun object while the Bun.sql / Bun.SQL lookup evaluating the module is still in progress, so a tree that then fails to evaluate (globalThis.Error = -6; Bun.sql) left debug builds asserting in storedPrototype. It now reads process.env, which is the same object. That is the only module-scope read of a Bun property in the sql tree.

4. Windows: env setup reified a Bun property mid-lookup. The windowsEnv builtin read Bun.inspect.custom while building the process.env proxy. Bun.$'s initializer evaluates process.env before Symbol("cwd"), so on Windows the first Bun.$ access transitioned the Bun object's structure and then threw, poisoning the in-progress static property lookup (caught by this PR's Windows CI). The symbol is now passed in from C++.

5. util.inspect lazy slots could not fail. m_utilInspectFunction and m_utilInspectStylizeColorFunction were set-once LazyProperty slots whose initializers bailed without init.set when requiring node:util threw, which violates the LazyProperty contract and aborts both debug and release builds:

const o = { [Symbol.for("nodejs.util.inspect.custom")]() { return "x"; } };
globalThis.Symbol = -6;
console.log(o); // SIGABRT in current releases; same with Bun.inspect(o, { colors: true })

The failure is usually transient (the registry does not cache a failed load; a stack overflow during the first custom inspect is enough), so caching a fallback would leave inspect degraded for the rest of the process. The two slots are now WriteBarrier members filled by accessors: on failure the accessor returns null with the exception pending and caches nothing, so the next inspect retries. callCustomInspectFunction clears it and falls back to default formatting for that one value; the other callers (URL, URLSearchParams, BroadcastChannel, streams) already propagate a pending exception. The unused Bun__REPL__formatValue is deleted.

Not in this PR: globalThis.Symbol = -6; require.cache aborts in the same way through m_lazyRequireCacheObject (NodeModuleModule.cpp). It is a different subsystem and is tracked for a separate fix using the same retryable-slot shape. Also left out: the Rust side has its own prototype read, JSValue::get_prototype, which returns a bare value with no way to report a throwing trap. Its formatter callers are not reachable with a throwing trap (the formatter unwraps proxies to their targets first; checked under exception scope validation), but napi_get_prototype returns napi_ok with an empty result and the exception left pending where Node returns napi_pending_exception. That is a parity bug rather than a crash and the right fix is at the get_prototype signature, so it is tracked separately as well.

How did you verify your code works?

Regression tests, each verified against the unfixed binary:

  • test/js/bun/util/inspect.test.js: the clobbered-Symbol enumeration of Bun, the throwing getPrototypeOf trap, and the custom inspect fallback. The fallback test pins that the fallback applies with and without colors, that a repeated call still falls back (nothing cached), and that the hook works again once the global is restored. All fail or crash on current releases.
  • test/js/node/util/util.test.js: spawned util.isError repro (segfaults current releases).
  • test/napi/napi.test.ts + addon: napi_get_all_property_names with a throwing descriptor trap in both key modes, asserting byte parity with Node (the include_prototypes case segfaults current releases).
  • test/js/sql/adapter-env-var-precedence.test.ts: spawned failing-sql-tree repro. This one is a debug assertion only: it aborts the debug build without the shared.ts change (verified by reverting just that file) and passes release builds either way. The rest of that file exercises the env resolution that shared.ts does, and passes unchanged with process.env.

Full files pass on Linux (bun bd test on inspect, util, bun-inspect, custom-inspect, url, broadcast-channel, napi, and the sql env file) and items 1, 2, 4 and the earlier version of 5 were also verified on a Windows machine. The original fuzzer reproduction exits cleanly on the fixed debug build.

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/js/sql/adapter-env-var-precedence.test.ts test/napi/napi.test.ts