Skip to content

Fix crashes when inspecting objects whose property enumeration throws #37175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 11 commits into from
Closed

Fix crashes when inspecting objects whose property enumeration throws #37175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
493876b
inspect: handle exceptions thrown while enumerating properties
robobun Aug 8, 2026
79b42b1
review: drain stderr in new tests, guard napi prototype walk
robobun Aug 8, 2026
37d37cd
inspect: survive node:util and env setup failing under clobbered globals
robobun Aug 8, 2026
bd8a5ac
test: cover custom inspect fallback when node:util cannot load
robobun Aug 8, 2026
af229dc
napi: stop the prototype walk before running more JS with an exceptio…
robobun Aug 8, 2026
bdd7a2e
util.isError: survive a throwing getPrototypeOf proxy trap
robobun Aug 8, 2026
6db3197
test: cover napi descriptor-trap exception paths, strengthen inspect …
robobun Aug 8, 2026
8e1c770
test: spawn the util.isError crash repro instead of running it in-pro…
robobun Aug 8, 2026
d2806ce
inspect: make the util.inspect slot retryable when node:util fails to…
robobun Aug 13, 2026
d4446c7
inspect: check for an exception between the two util.inspect accessor…
robobun Aug 13, 2026
54367dc
ci: retrigger (darwin aarch64 test shards expired unrun in build 97246)
robobun Aug 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/js/builtins/ProcessObjectInternals.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,8 +449,13 @@ export function windowsEnv(
editWindowsEnvVar: EditWindowsEnvVarCb,
coerceForWrite,
resetForDelete,
inspectCustomSymbol: symbol,
) {
(internalEnv as any)[Bun.inspect.custom] = () => {
// The symbol is passed from C++ rather than read off Bun.inspect.custom:
// reading Bun.inspect here reifies a lazy property on the Bun object, and env
// setup can run inside another Bun lazy property's initializer, where that
// structure transition poisons the outer property lookup if it then throws.
(internalEnv as any)[inspectCustomSymbol] = () => {
let o = {};
for (let k of envMapList) {
o[k] = internalEnv[k.toUpperCase()];
Expand Down
6 changes: 5 additions & 1 deletion src/js/internal/sql/shared.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1528,7 +1528,11 @@ function parseSQLiteOptions(

const DEFAULT_PROTOCOL: Bun.SQL.__internal.Adapter = "postgres";

const env = Bun.env;
// Same object as Bun.env, but reading it through process keeps this module's
// evaluation from reifying a property on the Bun object: it runs inside the
// Bun.sql / Bun.SQL lazy initializers, and a structure transition there
// followed by a throw later in the tree breaks the in-progress lookup.
const env = process.env;

/**
* Reads environment variables to try and find a connnection string
Expand Down
6 changes: 0 additions & 6 deletions src/jsc/bindings/BunObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,9 +320,6 @@ static JSValue defaultBunSQLObject(VM& vm, JSObject* bunObject)
auto scope = DECLARE_THROW_SCOPE(vm);
auto* globalObject = defaultGlobalObject(bunObject->globalObject());
JSValue sqlValue = globalObject->internalModuleRegistry()->requireId(globalObject, vm, InternalModuleRegistry::BunSql);
#if BUN_DEBUG
if (scope.exception()) globalObject->reportUncaughtExceptionAtEventLoop(globalObject, scope.exception());
#endif
RETURN_IF_EXCEPTION(scope, {});

@alii alii Aug 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the report gone this returns to the static lookup with the exception pending, and the sql tree has the shape fix 4 removes for the shell: internal/sql/shared.ts reads Bun.env at module scope before errors.ts evaluates, so if the tree throws (globalThis.Error = -6; Bun.sql) the Bun object has transitioned under the lookup and debug builds hit the storedPrototype assertion. Still asserts on main via the frame this deletes; once it is gone the outer lookup takes the same path. Changing that read to process.env plus a spawned test closes it on every OS; otherwise say in the body that it is left.

@robobun robobun Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b70bae7: shared.ts reads process.env now, which was the only module-scope Bun property read in the sql tree, plus a spawned test in test/js/sql/adapter-env-var-precedence.test.ts that aborts the debug build without the shared.ts change.

RELEASE_AND_RETURN(scope, sqlValue.getObject()->get(globalObject, vm.propertyNames->defaultKeyword));
}
Expand All @@ -332,9 +329,6 @@ static JSValue constructBunSQLObject(VM& vm, JSObject* bunObject)
auto scope = DECLARE_THROW_SCOPE(vm);
auto* globalObject = defaultGlobalObject(bunObject->globalObject());
JSValue sqlValue = globalObject->internalModuleRegistry()->requireId(globalObject, vm, InternalModuleRegistry::BunSql);
#if BUN_DEBUG
if (scope.exception()) globalObject->reportUncaughtExceptionAtEventLoop(globalObject, scope.exception());
#endif
RETURN_IF_EXCEPTION(scope, {});
auto clientData = WebCore::clientData(vm);
RELEASE_AND_RETURN(scope, sqlValue.getObject()->get(globalObject, clientData->builtinNames().SQLPublicName()));
Expand Down
2 changes: 2 additions & 0 deletions src/jsc/bindings/JSEnvironmentVariableMap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <JavaScriptCore/StructureInlines.h>
#include <JavaScriptCore/PropertyNameArray.h>
#include <JavaScriptCore/PropertyDescriptor.h>
#include <JavaScriptCore/Symbol.h>
#include "BunProcess.h"
#include "ScriptExecutionContext.h"
#include "SharedEnvStore.h"
Expand Down Expand Up @@ -1142,6 +1143,7 @@ JSValue createEnvironmentVariablesMap(Zig::GlobalObject* globalObject)
args.append(editWindowsEnvVar);
args.append(JSC::JSFunction::create(vm, globalObject, 2, "coerceForWrite"_s, jsProcessEnvCoerceForWrite, ImplementationVisibility::Private));
args.append(JSC::JSFunction::create(vm, globalObject, 1, "resetForDelete"_s, jsProcessEnvResetForDelete, ImplementationVisibility::Private));
args.append(JSC::Symbol::create(vm, vm.symbolRegistry().symbolForKey("nodejs.util.inspect.custom"_s).get()));
auto clientData = WebCore::clientData(vm);
JSC::CallData callData = JSC::getCallData(getSourceEvent);
NakedPtr<JSC::Exception> returnedException = nullptr;
Expand Down
16 changes: 12 additions & 4 deletions src/jsc/bindings/UtilInspect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,19 @@ extern "C" JSC::EncodedJSValue JSC__JSValue__callCustomInspectFunction(
auto& vm = JSC::getVM(globalObject);
auto scope = DECLARE_THROW_SCOPE(vm);

JSObject* options = Bun::createInspectOptionsObject(vm, globalObject, max_depth, colors);
RETURN_IF_EXCEPTION(scope, {});

// If node:util fails to evaluate (e.g. a clobbered global), returning the
// receiver makes the formatter fall back to default formatting for this
// value; nothing is cached, so a later inspect retries the load.
JSFunction* inspectFn = globalObject->utilInspectFunction();
RETURN_IF_EXCEPTION(scope, {});
if (scope.exception() || !inspectFn) [[unlikely]] {
(void)scope.tryClearException();
return JSValue::encode(thisValue);
}
JSObject* options = Bun::createInspectOptionsObject(vm, globalObject, max_depth, colors);
if (scope.exception() || !options) [[unlikely]] {
(void)scope.tryClearException();
return JSValue::encode(thisValue);
}
auto callData = JSC::getCallData(functionToCall);
MarkedArgumentBuffer arguments;
arguments.append(jsNumber(depth));
Expand Down
76 changes: 42 additions & 34 deletions src/jsc/bindings/ZigGlobalObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2303,18 +2303,6 @@ void GlobalObject::finishCreation(VM& vm)
init.set(JSFunction::create(init.vm, init.owner, 4, "performMicrotaskVariadic"_s, jsFunctionPerformMicrotaskVariadic, ImplementationVisibility::Public));
});

m_utilInspectFunction.initLater(
[](const Initializer<JSFunction>& init) {
auto scope = DECLARE_THROW_SCOPE(init.vm);
JSValue nodeUtilValue = uncheckedDowncast<Zig::GlobalObject>(init.owner)->internalModuleRegistry()->requireId(init.owner, init.vm, Bun::InternalModuleRegistry::Field::NodeUtil);
RETURN_IF_EXCEPTION(scope, );
RELEASE_ASSERT(nodeUtilValue.isObject());
auto prop = nodeUtilValue.getObject()->getIfPropertyExists(init.owner, Identifier::fromString(init.vm, "inspect"_s));
RETURN_IF_EXCEPTION(scope, );
ASSERT(prop);
init.set(uncheckedDowncast<JSFunction>(prop));
});

m_utilInspectOptionsStructure.initLater(
[](const Initializer<Structure>& init) {
init.set(Bun::createUtilInspectOptionsStructure(init.vm, init.owner));
Expand All @@ -2329,28 +2317,6 @@ void GlobalObject::finishCreation(VM& vm)
init.set(ErrorCodeCache::create(init.vm, structure));
});

m_utilInspectStylizeColorFunction.initLater(
[](const Initializer<JSFunction>& init) {
auto scope = DECLARE_THROW_SCOPE(init.vm);
JSC::MarkedArgumentBuffer args;
args.append(uncheckedDowncast<Zig::GlobalObject>(init.owner)->utilInspectFunction());
RETURN_IF_EXCEPTION(scope, );

JSC::JSFunction* getStylize = JSC::JSFunction::create(init.vm, init.owner, utilInspectGetStylizeWithColorCodeGenerator(init.vm), init.owner);
RETURN_IF_EXCEPTION(scope, );

JSC::CallData callData = JSC::getCallData(getStylize);
NakedPtr<JSC::Exception> returnedException = nullptr;
auto result = JSC::profiledCall(init.owner, ProfilingReason::API, getStylize, callData, jsNull(), args, returnedException);
RETURN_IF_EXCEPTION(scope, );

if (returnedException) {
throwException(init.owner, scope, returnedException.get());
}
RETURN_IF_EXCEPTION(scope, );
init.set(uncheckedDowncast<JSFunction>(result));
});

m_utilInspectStylizeNoColorFunction.initLater(
[](const Initializer<JSFunction>& init) {
init.set(JSC::JSFunction::create(init.vm, init.owner, utilInspectStylizeWithNoColorCodeGenerator(init.vm), init.owner));
Expand Down Expand Up @@ -3079,6 +3045,48 @@ JSC::JSObject* GlobalObject::navigatorObject()
return this->m_navigatorObject.get(this);
}

// Not a LazyProperty: those are set-once, and evaluating node:util can fail
// transiently (stack overflow, a clobbered global the script restores later).
// On failure this returns null with the exception pending and nothing is
// cached, so the next caller retries the load.
JSC::JSFunction* GlobalObject::utilInspectFunction()
{
if (auto* cached = m_utilInspectFunction.get())
return cached;

auto& vm = this->vm();
auto scope = DECLARE_THROW_SCOPE(vm);
JSValue nodeUtilValue = internalModuleRegistry()->requireId(this, vm, Bun::InternalModuleRegistry::Field::NodeUtil);
RETURN_IF_EXCEPTION(scope, nullptr);
RELEASE_ASSERT(nodeUtilValue.isObject());
JSValue inspect = nodeUtilValue.getObject()->getIfPropertyExists(this, Identifier::fromString(vm, "inspect"_s));
RETURN_IF_EXCEPTION(scope, nullptr);
ASSERT(inspect);
auto* inspectFunction = uncheckedDowncast<JSFunction>(inspect);
m_utilInspectFunction.set(vm, this, inspectFunction);
return inspectFunction;
}

JSC::JSFunction* GlobalObject::utilInspectStylizeColorFunction()
{
if (auto* cached = m_utilInspectStylizeColorFunction.get())
return cached;

auto& vm = this->vm();
auto scope = DECLARE_THROW_SCOPE(vm);
JSFunction* inspect = utilInspectFunction();
RETURN_IF_EXCEPTION(scope, nullptr);

JSFunction* getStylize = JSFunction::create(vm, this, utilInspectGetStylizeWithColorCodeGenerator(vm), this);
MarkedArgumentBuffer args;
args.append(inspect);
JSValue stylize = JSC::profiledCall(this, ProfilingReason::API, getStylize, JSC::getCallData(getStylize), jsNull(), args);
RETURN_IF_EXCEPTION(scope, nullptr);
auto* stylizeFunction = uncheckedDowncast<JSFunction>(stylize);
m_utilInspectStylizeColorFunction.set(vm, this, stylizeFunction);
return stylizeFunction;
}

JSC_DEFINE_CUSTOM_GETTER(functionLazyNavigatorGetter,
(JSC::JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue,
JSC::PropertyName))
Expand Down
9 changes: 5 additions & 4 deletions src/jsc/bindings/ZigGlobalObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,9 @@ class GlobalObject : public Bun::GlobalScope {
JSC::JSFunction* performMicrotaskVariadicFunction() const { return m_performMicrotaskVariadicFunction.getInitializedOnMainThread(this); }

JSC::Structure* utilInspectOptionsStructure() const { return m_utilInspectOptionsStructure.getInitializedOnMainThread(this); }
JSC::JSFunction* utilInspectFunction() const { return m_utilInspectFunction.getInitializedOnMainThread(this); }
JSC::JSFunction* utilInspectStylizeColorFunction() const { return m_utilInspectStylizeColorFunction.getInitializedOnMainThread(this); }
// These two return null with an exception pending if node:util fails to evaluate.
JSC::JSFunction* utilInspectFunction();
JSC::JSFunction* utilInspectStylizeColorFunction();
JSC::JSFunction* utilInspectStylizeNoColorFunction() const { return m_utilInspectStylizeNoColorFunction.getInitializedOnMainThread(this); }

JSC::JSFunction* wasmStreamingConsumeStreamFunction() const { return m_wasmStreamingConsumeStreamFunction.getInitializedOnMainThread(this); }
Expand Down Expand Up @@ -635,9 +636,9 @@ class GlobalObject : public Bun::GlobalScope {
V(private, LazyPropertyOfGlobalObject<Structure>, m_JSSocketHandlersStructure) \
V(private, LazyPropertyOfGlobalObject<JSFunction>, m_nativeMicrotaskTrampoline) \
V(private, LazyPropertyOfGlobalObject<JSFunction>, m_performMicrotaskVariadicFunction) \
V(private, LazyPropertyOfGlobalObject<JSFunction>, m_utilInspectFunction) \
V(private, WriteBarrier<JSFunction>, m_utilInspectFunction) \
V(private, LazyPropertyOfGlobalObject<Structure>, m_utilInspectOptionsStructure) \
V(private, LazyPropertyOfGlobalObject<JSFunction>, m_utilInspectStylizeColorFunction) \
V(private, WriteBarrier<JSFunction>, m_utilInspectStylizeColorFunction) \
V(private, LazyPropertyOfGlobalObject<JSFunction>, m_utilInspectStylizeNoColorFunction) \
V(private, LazyPropertyOfGlobalObject<JSFunction>, m_wasmStreamingConsumeStreamFunction) \
V(private, WebCore::JSStreamsRuntime, m_streamsRuntime) \
Expand Down
53 changes: 10 additions & 43 deletions src/jsc/bindings/bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5627,10 +5627,11 @@ static void JSC__JSValue__forEachPropertyImpl(JSC::EncodedJSValue JSValue0, JSC:
}

JSC::PropertySlot slot(object, PropertySlot::InternalMethodType::Get);
if (!object->getPropertySlot(globalObject, property, slot))
continue;
bool hasProperty = object->getPropertySlot(globalObject, property, slot);
// Ignore exceptions from "Get" proxy traps.
CLEAR_IF_EXCEPTION(scope);
if (!hasProperty)
continue;

if ((slot.attributes() & PropertyAttribute::DontEnum) != 0) {
if (property == propertyNames->underscoreProto
Expand Down Expand Up @@ -5702,7 +5703,13 @@ static void JSC__JSValue__forEachPropertyImpl(JSC::EncodedJSValue JSValue0, JSC:
break;
if (iterating == globalObject)
break;
iterating = iterating->getPrototype(globalObject).getObject();
JSValue prototype = iterating->getPrototype(globalObject);
// Ignore exceptions from "getPrototypeOf" proxy traps.
if (scope.exception()) [[unlikely]] {
(void)scope.tryClearException();
break;
}
iterating = prototype.getObject();
Comment thread
claude[bot] marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.
}
}

Expand Down Expand Up @@ -7029,46 +7036,6 @@ extern "C" JSC::EncodedJSValue Bun__REPL__getProperty(
return JSC::JSValue::encode(result ? result : JSC::jsUndefined());
}

// Format a value for REPL output using util.inspect style
extern "C" JSC::EncodedJSValue Bun__REPL__formatValue(
JSC::JSGlobalObject* globalObject,
JSC::EncodedJSValue valueEncoded,
int32_t depth,
bool colors)
{
auto& vm = JSC::getVM(globalObject);
auto scope = DECLARE_THROW_SCOPE(vm);

// Get the util.inspect function from the global object
auto* bunGlobal = uncheckedDowncast<Zig::GlobalObject>(globalObject);
JSC::JSValue inspectFn = bunGlobal->utilInspectFunction();

if (!inspectFn || !inspectFn.isCallable()) {
// Fallback to toString if util.inspect is not available
JSC::JSValue value = JSC::JSValue::decode(valueEncoded);
JSString* str = value.toString(globalObject);
RETURN_IF_EXCEPTION(scope, JSC::JSValue::encode(JSC::jsUndefined()));
return JSC::JSValue::encode(str);
}

// Create options object
JSC::JSObject* options = JSC::constructEmptyObject(globalObject);
options->putDirect(vm, JSC::Identifier::fromString(vm, "depth"_s), JSC::jsNumber(depth));
options->putDirect(vm, JSC::Identifier::fromString(vm, "colors"_s), JSC::jsBoolean(colors));
options->putDirect(vm, JSC::Identifier::fromString(vm, "maxArrayLength"_s), JSC::jsNumber(100));
options->putDirect(vm, JSC::Identifier::fromString(vm, "maxStringLength"_s), JSC::jsNumber(10000));
options->putDirect(vm, JSC::Identifier::fromString(vm, "breakLength"_s), JSC::jsNumber(80));

JSC::MarkedArgumentBuffer args;
args.append(JSC::JSValue::decode(valueEncoded));
args.append(options);

JSC::JSValue result = JSC::call(globalObject, inspectFn, JSC::ArgList(args), "util.inspect"_s);
RETURN_IF_EXCEPTION(scope, JSC::JSValue::encode(JSC::jsUndefined()));

return JSC::JSValue::encode(result);
}

// Collects every ArrayBufferView in a JSArray and the (data, byteLength) span
// of each. Two passes, mirroring Buffer.concat: the first reads every element
// into a MarkedArgumentBuffer, so any user code an indexed read can run
Expand Down
1 change: 0 additions & 1 deletion src/jsc/bindings/headers.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 8 additions & 1 deletion src/jsc/bindings/napi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2077,14 +2077,21 @@
if (key_mode == napi_key_include_prototypes) {
// Climb up the prototype chain to find inherited properties
while (!owner->getOwnPropertyDescriptor(globalObject, propKey, desc)) {
JSObject* proto = owner->getPrototype(globalObject).getObject();
// A throwing "getOwnPropertyDescriptor" proxy trap must stop the walk
// before getPrototype runs more JS with the exception pending.
NAPI_RETURN_IF_EXCEPTION(env);
JSValue protoValue = owner->getPrototype(globalObject);
// A throwing proxy trap leaves protoValue empty; getObject() on it is a null deref.
NAPI_RETURN_IF_EXCEPTION(env);

Check warning on line 2085 in src/jsc/bindings/napi.cpp

View check run for this annotation

Claude / Claude Code Review

napi_get_prototype (Rust) missed in throwing-getPrototypeOf sibling sweep 

The sibling sweep of `getPrototype` sites ("I also swept the remaining getPrototype(globalObject) call sites in src/jsc") was C++-only; the Rust N-API `napi_get_prototype` at `src/runtime/napi/napi_body.rs:979-980` has the same bug class fixed here — `object.get_prototype()` on a Proxy with a throwing `getPrototypeOf` trap returns `napi_ok` with an empty result and a pending exception, whereas Node returns `napi_pending_exception`. Per REVIEW.md ("Fix the whole class… If a site is intentionally
Comment thread
claude[bot] marked this conversation as resolved.
Comment on lines +2083 to +2085

@claude claude Bot Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The sibling sweep of getPrototype sites ("I also swept the remaining getPrototype(globalObject) call sites in src/jsc") was C++-only; the Rust N-API napi_get_prototype at src/runtime/napi/napi_body.rs:979-980 has the same bug class fixed here — object.get_prototype() on a Proxy with a throwing getPrototypeOf trap returns napi_ok with an empty result and a pending exception, whereas Node returns napi_pending_exception. Per REVIEW.md ("Fix the whole class… If a site is intentionally excluded, say so in the PR"), consider either checking env.has_pending_exception() after the call, or noting in the PR body that Rust-side get_prototype callers are excluded (the API returns JSValue, not JsResult<JSValue>; five sites across napi_body.rs / ConsoleObject.rs / pretty_format.rs). Nit: pre-existing, no release crash, narrow trigger.

Extended reasoning...

What the finding is

The PR's sibling sweep of throwing-getPrototype sites — stated in the review response as "I also swept the remaining getPrototype(globalObject) call sites in src/jsc for the widened pattern" — covered only C++ files under src/jsc/. The Rust N-API implementation napi_get_prototype at src/runtime/napi/napi_body.rs:979-980 has the identical bug class this PR fixes in napi_get_all_property_names (napi.cpp:2080-2086):

result.set(env, object.get_prototype(env.to_js()));
env.ok()

JSValue::get_prototype (JSValue.rs:2322) returns bare JSValue (not JsResult<JSValue>) and calls JSC__JSValue__getPrototype directly, with no from_js_host_call wrapper. JSC__JSValue__getPrototype (bindings.cpp:4163-4167) has no ThrowScope and no exception check — it just encodes value.getPrototype(arg1). env.ok() (napi_body.rs:124-126) unconditionally sets NapiStatus::ok without checking for a pending exception. So a throwing getPrototypeOf trap leaves the exception pending, writes an empty JSValue to *result, and returns napi_ok.

Step-by-step proof

  1. A native addon calls napi_get_prototype(env, proxy, &out) where proxy is new Proxy({}, { getPrototypeOf() { throw new Error('trap') } }).
  2. preamble! (napi_body.rs:477-485) checks for a pending exception at entry only — none is pending yet, so it passes.
  3. object.is_empty() and object.is_undefined_or_null() are both false (a Proxy is a non-empty object), so control reaches line 979.
  4. object.get_prototype(env.to_js()) → JSC__JSValue__getPrototype → value.getPrototype(globalObject) → ProxyObject::getPrototype, which calls the user's trap. The trap throws; getPrototype returns an empty JSValue (encoded 0) with the exception pending on the VM.
  5. result.set(env, <empty>) writes 0 to *out. This does not dereference a cell, so no release crash — this differs from the napi.cpp site, which called .getObject() on the empty value.
  6. env.ok() returns napi_ok. The addon receives napi_ok + an encoded-0 napi_value + a pending exception it did not expect.

Node.js returns napi_pending_exception here: its GetPrototypeV2() returns an empty MaybeLocal, and CHECK_MAYBE_EMPTY bails with the pending-exception status.

Why existing code doesn't prevent it

preamble! only checks exceptions at entry. The Rust JSValue::get_prototype type signature (fn get_prototype(&self, global: *mut JSGlobalObject) -> JSValue) does not surface the exception — the same shape as the C++ getPrototype(globalObject).getObject() before this PR fixed it, minus the null-deref. There is no post-body exception check between line 979 and the env.ok() return.

Impact

No release crash (the empty JSValue is stored, not dereferenced). The addon receives napi_ok with an invalid result and a pending exception, diverging from Node. The addon may then pass the empty napi_value to another N-API call and get napi_invalid_arg for no obvious reason, or the pending exception surfaces one N-API call late. Under BUN_JSC_validateExceptionChecks=1, the addon's next N-API call that declares a ThrowScope (via preamble! → NAPI_PREAMBLE) would observe and return napi_pending_exception, so the exception is caught at the next call rather than this one — one call late.

Why this belongs in scope, and why it's still a nit

REVIEW.md, Correctness: the bug class, not the bug: "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep). Grep for every sibling site sharing the pattern… If a site is intentionally excluded, say so in the PR." This is a direct sibling of the napi.cpp site the PR fixes and adds a test for, in the same N-API surface (napi_get_prototype vs napi_get_all_property_names), missed because the sweep was scoped to src/jsc C++ files. The PR does not touch napi_body.rs, and the trigger requires a native addon plus a Proxy with a throwing getPrototypeOf trap — narrow. It does not block merge.

How to fix

Either:

result.set(env, object.get_prototype(env.to_js()));
if env.has_pending_exception() {
    return env.pending_exception();
}
env.ok()

…or note in the PR body that Rust-side get_prototype callers are intentionally excluded from this sweep. There are five: napi_body.rs:979, ConsoleObject.rs:3242/3972/4038, pretty_format.rs:877 — none wrap the call in from_js_host_call, and get_prototype itself does not return JsResult. Fixing the API signature to return JsResult<JSValue> would cover all five at once but is a larger change than this PR's scope.

JSObject* proto = protoValue.getObject();
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.
if (!proto) {
break;
}
owner = proto;
}
} else {
owner->getOwnPropertyDescriptor(globalObject, propKey, desc);
NAPI_RETURN_IF_EXCEPTION(env);
}

// V8 never applies ONLY_WRITABLE/ONLY_CONFIGURABLE to Proxy keys
Expand Down
29 changes: 18 additions & 11 deletions src/jsc/modules/NodeUtilTypesModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,22 +898,29 @@ JSC_DEFINE_HOST_FUNCTION(jsFunctionIsError,
// node util.isError relies on toString
// https://github.com/nodejs/node/blob/cf8c6994e0f764af02da4fa70bc5962142181bf3/doc/api/util.md#L2923
// util.isError is deprecated and removed in node 23
PropertySlot slot(object, PropertySlot::InternalMethodType::VMInquiry, &vm);
bool has = object->getPropertySlot(globalObject, vm.propertyNames->toStringTagSymbol, slot);
scope.assertNoException();
if (has) {
if (slot.isValue()) {
JSValue value = slot.getValue(globalObject, vm.propertyNames->toStringTagSymbol);
if (value.isString()) {
String tag = asString(value)->value(globalObject);
CLEAR_IF_EXCEPTION(scope);
if (tag == "Error"_s)
return JSValue::encode(jsBoolean(true));
{
// Scoped: a live VMInquiry slot forbids VM entry, and getPrototype
// below can enter JS through a "getPrototypeOf" proxy trap.
PropertySlot slot(object, PropertySlot::InternalMethodType::VMInquiry, &vm);
bool has = object->getPropertySlot(globalObject, vm.propertyNames->toStringTagSymbol, slot);
scope.assertNoException();
if (has) {
if (slot.isValue()) {
JSValue value = slot.getValue(globalObject, vm.propertyNames->toStringTagSymbol);
if (value.isString()) {
String tag = asString(value)->value(globalObject);
CLEAR_IF_EXCEPTION(scope);
if (tag == "Error"_s)
return JSValue::encode(jsBoolean(true));
}
}
}
}

JSValue proto = object->getPrototype(globalObject);
// A throwing "getPrototypeOf" proxy trap leaves proto empty, and isCell()
// is true for the empty value, so the checks below would deref null.
RETURN_IF_EXCEPTION(scope, {});
if (proto.isCell() && (proto.inherits<JSC::ErrorInstance>() || proto.asCell()->type() == ErrorInstanceType || proto.inherits<JSC::ErrorPrototype>()))
return JSValue::encode(jsBoolean(true));
}
Expand Down
Loading