Skip to content

fix(inspect): don't keep enumerating properties with an exception pending #37330

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 1 commit into from
Closed

fix(inspect): don't keep enumerating properties with an exception pending #37330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
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 @@ -319,9 +319,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, {});
RELEASE_AND_RETURN(scope, sqlValue.getObject()->get(globalObject, vm.propertyNames->defaultKeyword));
}
Expand All @@ -331,9 +328,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
14 changes: 10 additions & 4 deletions src/jsc/bindings/bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5596,10 +5596,11 @@
}

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

if ((slot.attributes() & PropertyAttribute::DontEnum) != 0) {
if (property == propertyNames->underscoreProto
Expand Down Expand Up @@ -5671,7 +5672,12 @@
break;
if (iterating == globalObject)
break;
iterating = iterating->getPrototype(globalObject).getObject();
JSValue prototype = iterating->getPrototype(globalObject);
// Ignore exceptions from Proxy "getPrototypeOf" traps.
CLEAR_IF_EXCEPTION(scope);
if (!prototype) [[unlikely]]
break;
iterating = prototype.getObject();

Check warning on line 5680 in src/jsc/bindings/bindings.cpp

View check run for this annotation

Claude / Claude Code Review

Same getPrototype().getObject() pattern remains at napi.cpp:2072 

nit: The same `->getPrototype(globalObject).getObject()` pattern remains at `src/jsc/bindings/napi.cpp:2072` (in `napi_get_all_property_names`'s filter loop, with no exception check after `getOwnPropertyDescriptor`) — a Proxy prototype whose trap throws produces the same segfault-at-0x5. It's the sole greppable sibling of this pattern in `src/`; pre-existing N-API code so non-blocking, but per REVIEW.md's "grep for every sibling site" it may be worth including the one-line fix here or noting it'
Comment on lines +5675 to +5680

@claude claude Bot Aug 10, 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.

🟡 nit: The same ->getPrototype(globalObject).getObject() pattern remains at src/jsc/bindings/napi.cpp:2072 (in napi_get_all_property_names's filter loop, with no exception check after getOwnPropertyDescriptor) — a Proxy prototype whose trap throws produces the same segfault-at-0x5. It's the sole greppable sibling of this pattern in src/; pre-existing N-API code so non-blocking, but per REVIEW.md's "grep for every sibling site" it may be worth including the one-line fix here or noting it's intentionally out of scope.

Extended reasoning...

What this is

This PR fixes iterating->getPrototype(globalObject).getObject() in JSC__JSValue__forEachPropertyImpl (bindings.cpp:5675-5680) — when ProxyObject::getPrototype throws, it returns an empty JSValue, and calling .getObject() on an empty JSValue dereferences a null cell pointer and segfaults at address 0x5. A grep for this exact pattern across src/ finds one remaining instance at src/jsc/bindings/napi.cpp:2072, inside napi_get_all_property_names:

while (!owner->getOwnPropertyDescriptor(globalObject, propKey, desc)) {
    JSObject* proto = owner->getPrototype(globalObject).getObject();
    if (!proto) break;
    owner = proto;
}

Code path that triggers it

Reachable via napi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_enumerable | ..., ...) on an object with a Proxy in its prototype chain. The filter loop at napi.cpp:2061-2080 runs when key_filter includes any of napi_key_enumerable/writable/configurable and key_mode == napi_key_include_prototypes. Inside that loop, getOwnPropertyDescriptor (line 2071) invokes the Proxy's getOwnPropertyDescriptor trap; if the trap throws, there is no exception check before line 2072, so owner->getPrototype(globalObject) on the same Proxy hits RETURN_IF_EXCEPTION inside ProxyObject::performGetPrototype and returns {}. A stateful getPrototypeOf trap that succeeds during getPropertyNames (line 1999) but throws when re-called here produces the same empty return.

Why the existing code doesn't prevent it

The if (!proto) break; on line 2073 checks the result of .getObject(), but .getObject() itself is what crashes: on JSVALUE64 an empty JSValue encodes as 0, which satisfies isCell(), so asCell() returns nullptr and JSCell::getObject() reads m_type at offset ~5 → SIGSEGV at 0x5. This is the identical mechanism the PR description documents for the release-build segfault in forEachPropertyImpl.

Step-by-step proof

  1. Native addon calls napi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_enumerable, napi_key_numbers_to_strings, &result) where obj's prototype is new Proxy({}, { ownKeys: () => ['x'], getOwnPropertyDescriptor() { throw new Error('boom'); } }).
  2. getPropertyNames at line 1999 collects 'x' via the ownKeys trap (uses jsc_key_mode = Include, so it does not call getOwnPropertyDescriptor); NAPI_RETURN_IF_EXCEPTION at 2057 passes.
  3. The filter loop reaches propKey = 'x'; owner = object; object->getOwnPropertyDescriptor(...) returns false (not own), so the while body runs and owner becomes the Proxy.
  4. Next iteration: owner->getOwnPropertyDescriptor(globalObject, 'x', desc) invokes the Proxy trap, which throws. Return value is false → loop body runs again with the exception still pending.
  5. owner->getPrototype(globalObject) on the Proxy enters ProxyObject::performGetPrototype, hits RETURN_IF_EXCEPTION, returns JSValue().
  6. JSValue().getObject() → asCell() = nullptr → nullptr->isObject() reads offset 5 → segfault.

Impact and fix

This is pre-existing N-API code the PR does not touch, only reachable through a native addon calling napi_get_all_property_names on an object with an adversarial Proxy in its chain, so it should not block this PR — merging is strictly an improvement over main. Flagging per REVIEW.md's "Fix the whole class in the same PR — grep for every sibling site sharing the pattern... If a site is intentionally excluded, say so in the PR" since this is the sole greppable sibling of the exact pattern being fixed. The fix is the same shape as this PR's hunk: capture getPrototype(globalObject) into a JSValue, check/clear the exception, break if empty, then call .getObject() — plus an exception check after getOwnPropertyDescriptor.

}
}

Expand Down
48 changes: 48 additions & 0 deletions test/js/bun/util/inspect.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,54 @@ describe("crash testing", () => {
}
});

// An exception thrown while looking up a property (Proxy trap, lazily initialized
// property) used to stay pending while the next property was looked up, and a
// throwing getPrototypeOf trap crashed the walk up the prototype chain.
// Run in a child so a regression fails this test instead of killing the runner.
it("Bun.inspect ignores exceptions thrown while enumerating properties", async () => {
const code = `
{
const target = { a: 1, b: 2 };
const proto = new Proxy(target, {
get(t, key, receiver) {
if (key in target) throw new Error("get " + String(key));
return Reflect.get(t, key, receiver);
},
});
const obj = Object.create(proto);
obj.own = 1;
console.log(Bun.inspect(obj));
}
{
const proto = new Proxy({ fromProto: 2 }, {
getPrototypeOf() { throw new Error("getPrototypeOf"); },
});
const obj = Object.create(proto);
obj.own = 1;
console.log(Bun.inspect(obj));
}
{
// Several of Bun's lazily initialized properties (Bun.$ among them) call
// Symbol() when first accessed, so this makes their initializers throw.
globalThis.Symbol = undefined;
const out = Bun.inspect(Bun);
console.log(typeof out, out.includes("inspect"));
}
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", code],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout, stderr, exitCode }).toEqual({
stdout: "{\n own: 1,\n}\n" + "{\n own: 1,\n fromProto: 2,\n}\n" + "string true\n",
stderr: "",
exitCode: 0,
});
});

it("possibly formatted emojis log", () => {
expect(Bun.inspect("✔")).toBe('"✔"');
});
Expand Down