util.isError: propagate a throwing getPrototypeOf trap instead of crashing

Closed
RO
3mo ago
+50-12

Problem

  • util.isError(new Proxy({}, { getPrototypeOf() { throw err; } })) crashes the process: release builds with Segmentation fault at address 0x0, debug builds with an abort, instead of throwing err the way err instanceof Error (what Node's util.isError is built on) does.
  • Cause, in jsFunctionIsError (src/jsc/modules/NodeUtilTypesModule.cpp):
    • the PropertySlot created with InternalMethodType::VMInquiry for the Symbol.toStringTag check stays alive until the end of the function, and a VMInquiry slot forbids entering the VM for as long as it exists. The object->getPrototype(globalObject) call further down runs the Proxy's getPrototypeOf trap, so in debug builds the VM entry check aborts; in release builds the trap is not run at all, the call yields undefined, and the Proxy turns that into a TypeError (so even a well-behaved trap made util.isError fail).
    • the value returned by getPrototype() is used without checking for that exception. A throwing getPrototype() returns the empty JSValue, which passes isCell(), so proto.inherits<ErrorInstance>() reads the structure of a null cell.

Fix

  • Scope the VMInquiry slot to the toStringTag check so it is destroyed before getPrototype() is called, and add RETURN_IF_EXCEPTION after getPrototype() so the trap's error propagates to the caller.
  • This matches Node, where util.isError(x) ends in x instanceof Error, which runs the same trap and throws the same error.
  • Verified with test/js/node/util/util.test.js, util > isError > handles Proxy getPrototypeOf traps (spawned child): the thrown error is the trap's error, and non-throwing traps returning Error.prototype / null give true / false. Without the fix the child exits 139 (release) or 134 (debug); with it the test passes and the rest of util.test.js (209 tests) still passes.

Background

  • PropertySlot with InternalMethodType::VMInquiry asks JSC for a side-effect-free lookup; JSC enforces that by holding a DisallowVMEntry token for the slot's lifetime, which makes any attempt to run JavaScript while the slot is alive fail (assert in debug, a TypeError-style failure in release).
  • A JSC exception is pending state on the VM: a function that threw returns a placeholder (here the empty JSValue) and the caller must check the scope before using the result.

History

This PR originally also fixed the stale-exception / getPrototype crash in the Bun.inspect property walk, the matching check in napi_get_all_property_names, the util.inspect lazy properties, and the BunObject.cpp debug-only report. Those are consolidated in #29642 (property walk, napi, BunObject.cpp) and #39382 (util.inspect lazy properties); the windowsEnv change only worked around the stale-structure assertion that #37001 fixes in JSC. This PR has been narrowed to the util.isError fix, which none of those cover. The same util.isError fix was also part of #37175 and #39412, both closed.