Skip to content

inspect: clear exceptions when walking Proxy prototype chain #29845

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 3 commits into from
Closed

inspect: clear exceptions when walking Proxy prototype chain #29845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb1caa9
inspect: clear exceptions when walking Proxy prototype chain
robobun Apr 28, 2026
97c45b0
test: tighten assertion for throwing getter in Proxy prototype
robobun Apr 28, 2026
2e35e3a
ci: retrigger build
robobun May 4, 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
12 changes: 9 additions & 3 deletions src/jsc/bindings/bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5369,10 +5369,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 @@ -5444,7 +5445,12 @@ static void JSC__JSValue__forEachPropertyImpl(JSC::EncodedJSValue JSValue0, JSC:
break;
if (iterating == globalObject)
break;
iterating = iterating->getPrototype(globalObject).getObject();
JSValue nextProto = iterating->getPrototype(globalObject);
// Ignore exceptions from Proxy "getPrototypeOf" trap.
CLEAR_IF_EXCEPTION(scope);
if (!nextProto)
break;
iterating = nextProto.getObject();
Comment on lines +5448 to +5453

@claude claude Bot Apr 28, 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.

🟣 Heads-up (pre-existing, not introduced by this PR): the same getPrototype(globalObject).getObject() null-deref pattern this PR fixes still exists at src/bun.js/bindings/napi.cpp:1837 in napi_get_all_property_names. A Proxy with a throwing getPrototypeOf/getOwnPropertyDescriptor trap will crash there the same way; PR #29642 reportedly extends the fix to napi, so you may want to apply the same CLEAR_IF_EXCEPTION + empty-value guard there or land that PR alongside.

Extended reasoning...

Summary

This PR correctly fixes a null-pointer crash in forEachPropertyImpl (bindings.cpp:5364-5369) where iterating->getPrototype(globalObject).getObject() dereferences a null JSCell when a Proxy's getPrototypeOf trap throws (or a prior trap leaves an exception pending). However, the identical vulnerable pattern remains untouched at src/bun.js/bindings/napi.cpp:1837 inside napi_get_all_property_names:

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

This is pre-existing — the PR doesn't touch napi.cpp, add callers to it, or change its trigger likelihood. It is flagged here only because it is the exact same bug class being fixed, and the duplicate-PR bot on this PR already notes that #29642 covers the napi side.

Code path

napi_get_all_property_names is reachable from any native addon. When called with key_mode == napi_key_include_prototypes and a non-trivial key_filter (e.g. napi_key_writable / napi_key_enumerable / napi_key_configurable), the function enters the descriptor-filtering branch at napi.cpp:1833. For each key it walks the prototype chain:

  1. Line 1836 calls current_object->getOwnPropertyDescriptor(...). If current_object is a ProxyObject, this invokes the JS getOwnPropertyDescriptor trap, which can throw. On throw, getOwnPropertyDescriptor returns false with a pending exception, and the loop body executes.
  2. Line 1837 calls current_object->getPrototype(globalObject). With an exception already pending — or if current_object is a Proxy whose getPrototypeOf trap throws — ProxyObject::getPrototype short-circuits via RETURN_IF_EXCEPTION and returns an empty JSValue ({ }).
  3. .getObject() is called on that empty value. On JSVALUE64, an empty JSValue is encoded as 0, which passes the isCell() check ((0 & NotCellMask) == 0), so asCell() returns nullptr and asCell()->getObject() dereferences null — crashing before the if (!proto) check on line 1838 ever runs.

Why existing code doesn't prevent it

There is no ThrowScope/CatchScope exception clearing between the getOwnPropertyDescriptor call and the getPrototype call, and the result of getPrototype() is chained directly into .getObject() without an empty-value check. The if (!proto) break; guard on line 1838 only handles the case where getPrototype() legitimately returns jsNull()/jsUndefined() (for which .getObject() safely returns nullptr); it cannot help when .getObject() itself segfaults on an empty value.

Step-by-step proof

  1. JS creates const p = new Proxy({}, { getPrototypeOf() { throw new Error('boom'); } }); and passes p to a native addon.
  2. The addon calls napi_get_all_property_names(env, p, napi_key_include_prototypes, napi_key_writable, napi_key_numbers_to_strings, &result).
  3. Enumeration finds at least one key (e.g. inherited from Object.prototype since getOwnPropertyNames includes prototypes here), enters the filter loop, and getOwnPropertyDescriptor on the proxy returns false (no own descriptor on the empty target).
  4. current_object->getPrototype(globalObject) invokes the getPrototypeOf trap → throws → returns empty JSValue.
  5. emptyJSValue.getObject() → isCell() true → asCell() = nullptr → nullptr->getObject() → SIGSEGV.

The same crash occurs without a getPrototypeOf trap if the getOwnPropertyDescriptor trap throws at step 3, because step 4 then short-circuits on the pending exception and still returns empty.

Impact

Process crash (null-pointer dereference) in any native addon that calls napi_get_all_property_names with napi_key_include_prototypes + a descriptor filter on an object whose prototype chain contains a hostile/buggy Proxy. Lower exposure than the Bun.inspect path fixed in this PR (requires a native addon and a specific key-mode/filter combo), but identical mechanism.

Suggested fix

Apply the same pattern this PR uses in bindings.cpp:

while (!current_object->getOwnPropertyDescriptor(globalObject, key.toPropertyKey(globalObject), desc)) {
    CLEAR_IF_EXCEPTION(scope); // or RETURN_IF_EXCEPTION as appropriate for napi
    JSValue protoValue = current_object->getPrototype(globalObject);
    CLEAR_IF_EXCEPTION(scope);
    if (!protoValue)
        break;
    JSObject* proto = protoValue.getObject();
    if (!proto)
        break;
    current_object = proto;
}

Since #29642 reportedly already does this, the simplest path is to land that PR alongside this one rather than expanding scope here.

}
}

Expand Down
50 changes: 50 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 @@ -772,3 +772,53 @@ it("CustomEvent", () => {
}"
`);
});

describe("Proxy in prototype chain", () => {
it("does not crash when a Proxy prototype has a throwing getter", () => {
const obj = {};
const proto = {
get ok() {
return 1;
},
get bad() {
throw new Error("nope");
},
get ok2() {
return 2;
},
};
Object.setPrototypeOf(obj, new Proxy(proto, {}));
const out = Bun.inspect(obj);
expect(out).toContain("ok: 1");
expect(out).toContain("ok2: 2");
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

it("does not crash when a Proxy prototype has a throwing getPrototypeOf trap", () => {
class Foo {
bar() {}
baz() {}
}
const obj = new Foo();
const originalPrototype = Object.getPrototypeOf(obj);
const newPrototype = new Proxy(originalPrototype, {
getPrototypeOf() {
throw new Error("boom");
},
});
Object.setPrototypeOf(obj, newPrototype);
const out = Bun.inspect(obj);
expect(out).toContain("bar");
expect(out).toContain("baz");
});

it("does not crash when formatting an expect() object with a Proxy prototype", () => {
const received = expect({});
const originalPrototype = Object.getPrototypeOf(received);
Object.setPrototypeOf(received, new Proxy(originalPrototype, {}));
try {
expect(typeof Bun.inspect(received)).toBe("string");
} finally {
Object.setPrototypeOf(received, originalPrototype);
}
});
});
Loading