Make a failed util.inspect load throw from custom inspect functions instead of aborting

Closed
RO
19d ago
+167-40

Problem

  • The formatter loads util.inspect when it first runs a custom inspect function. If the load throws, the process aborts. Debug builds report ASSERTION FAILED: !(initializer.property.m_pointer & lazyTag) in LazyProperty::callFunc.
  • The load throws when a global that the module reads was replaced, or near the stack limit. require("util").inspect = 42 aborts as well.
  • Cause: m_utilInspectFunction and m_utilInspectStylizeColorFunction in ZigGlobalObject.cpp were LazyProperty members whose initializers returned without init.set() on an exception. LazyProperty asserts on that.

Fix

  • The two members become WriteBarriers that the getters fill in. When the load throws, the getters return nullptr with the exception pending and cache nothing. The next call loads again. InternalModuleRegistry::requireId has the same contract.
  • Correct because the failure is a normal JavaScript exception whose cause (a replaced global, a deep stack) can go away. Every caller already checks for it.
  • utilInspectFunction() now reads inspect from internal/util/inspect, as node does. A replaced util.inspect (a value, a Proxy, a mock) no longer reaches custom inspect functions. The internal export only has to be callable. One that is not throws a TypeError instead of an abort.
  • Verified: five child-process cases in test/js/bun/util/inspect.test.js. They fail the load of each member and load it again, and they check the export handling. All five abort on the current build.

Background

  • LazyProperty<Owner, T> is a JSC member that a callback fills in on first access. The callback must call init.set(value). callFunc asserts (RELEASE_ASSERT) if it did not. The type has no failed state.
  • WriteBarrier<T> is the ordinary GC-aware member pointer, marked through FOR_EACH_GLOBALOBJECT_GC_MEMBER. It can hold null, so a failed load leaves it empty.
  • InternalModuleRegistry::requireId evaluates a built-in module on first use. internal/util/inspect implements util.inspect. node:util re-exports it.
Notes

Triggers seen: globalThis.Symbol = 0 or a replaced Reflect (the fuzzer's samples), and the first custom inspect call from a stack overflow handler. On Windows Bun.env has a custom inspect function, so console.log(Bun) reaches this. With colors, the stylize helper is built from the same function and had the same problem. The abort needs a process that has not loaded the module yet, which is why the fuzzer reports it as flaky. The require("util").inspect = 42 case aborted because the initializer downcast the property without a check (debug: ASSERTION FAILED: isCell()). The callers already checked for an exception after the getters, but the assertion fired first.

Node passes the inspect of internal/util/inspect to custom inspect functions, so a replaced util.inspect has no effect there. The same script now prints the same result in node and bun.

Test block: "Bun.inspect when loading util.inspect for a custom inspect function throws". The child processes run with bunEnv, which makes require("internal/util/inspect") work. Cases: Symbol replaced then restored (colors path, then Bun.gc(true) to check that the cached functions are marked), the stack exhausted while the inspect function loads (no colors) and, with it already loaded, while the stylize helper is built from it (colors), the internal export set to 42 (TypeError) and then to a Proxy (accepted), and util.inspect replaced (the custom function still receives the real one). All five exit with code 134 in the child on the current release and debug builds. Also run with the debug build: the rest of inspect.test.js, test/js/node/util/custom-inspect.test.js, test/js/web/url/url.test.ts, test/js/web/broadcastchannel/broadcast-channel.test.ts, test/js/web/console/console-log.test.ts.

Since #39770, finishCreation installs the lazy initializers from offset tables (lazyFunctionInits). The two entries for these members are removed there. A WriteBarrier member must not have a table entry, because the table would call initLater on it. The branch is rebased past #39732 as well, which deleted the unused navigatorObject() next to which the getters were first placed; they now follow assignToStream, with no other change. A third rebase followed #39581, which deleted the unused m_performMicrotaskVariadicFunction next to the two entries this PR removes; main's deletion is kept and nothing else moved.

The callers of the getter: UtilInspect.cpp (custom inspect functions), JSURLSearchParams.cpp, WebStreamsInspectCustom.cpp, JSBroadcastChannel.cpp, and Bun__REPL__formatValue in bindings.cpp, which gets the exception check it was missing. The first four now hold a JSObject*. getCallData and profiledCall take a cell or a value, so nothing needed a JSFunction*. JSPerformanceEntry.cpp is one more caller, added on main by #39921 after this PR was opened. It keeps the result in a JSValue, checks for an exception right after the call and calls it through getCallData, so it needs no change. Bun__REPL__formatValue has no caller (repl.rs declares only evaluate, getCompletions and getProperty). This PR only adds the check it was missing. Deleting it is a separate cleanup.

History: this PR first also fixed the stale exception and getPrototype crash in the property walk, the napi_get_all_property_names check and the BunObject.cpp report. Those landed through #29642. The same abort was addressed with other designs in #29235, #30267, #37160, #37175, #37202 and #37213 (cache a placeholder or null, install a throwing stub, install an identity fallback). They were closed in favour of this one. #39524 was the same change as this PR. Its stack exhaustion tests are folded in here.

A self-review of the folded revision found two things in the getter. It narrowed the export to a JSFunction, which rejected a Proxy or a mock that user code had put on util.inspect, and it read that user-mutable property at all. Reading the internal module removes both from user reach. The JSObject plus isCallable shape is then only observable by code that can require the internal module (--expose-internals, or the test environment). It is kept because nothing needs a JSFunction* and it turns the remaining tampering case into a TypeError. A second self-review of the current revision found that no case reached the stylize helper's own failure (the inspect function always failed first). The last commit adds that case.

Other lazily built members whose builder runs JavaScript, for the record. Same failure on main: m_lazyRequireCacheObject (#37338, and #39804 handles it with the lazy property kept, setMayBeNull on failure and a getter that builds it again on the next access) and m_processEnvObject (#38821). Different policy already on main: m_lazyTestModuleObject keeps a placeholder object after a failure. The getter shape in this PR has the same behaviour as the #39804 shape (empty on failure, the exception pending, built again on the next access). It uses a plain WriteBarrier because both members are created and read in ZigGlobalObject.cpp. If the LazyProperty plus setMayBeNull shape is preferred for consistency with #39804, the conversion is small and I can push it.

no test proof · iteration 9 · 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