Skip to content

Fix null deref in forEachProperty when Proxy getPrototype throws #30457

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
+30 −3
Closed

Fix null deref in forEachProperty when Proxy getPrototype throws #30457

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
0931891
Fix null deref in forEachProperty when Proxy getPrototype throws
robobun May 10, 2026
b03795d
Clear exception before continue in property slot lookup
robobun May 10, 2026
87ac0b8
ci: retrigger
robobun May 10, 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
10 changes: 7 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,10 @@ static void JSC__JSValue__forEachPropertyImpl(JSC::EncodedJSValue JSValue0, JSC:
break;
if (iterating == globalObject)
break;
iterating = iterating->getPrototype(globalObject).getObject();
JSValue proto = iterating->getPrototype(globalObject);
// Ignore exceptions from Proxy "getPrototypeOf" trap.
CLEAR_IF_EXCEPTION(scope);
iterating = proto ? proto.getObject() : nullptr;
Comment on lines +5448 to +5451

@claude claude Bot May 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.

🟣 Heads up: the same vulnerable pattern this PR fixes still exists at src/jsc/bindings/napi.cpp:1837 (current_object->getPrototype(globalObject).getObject()), reachable via napi_get_all_property_names with napi_key_include_prototypes when the chain contains a Proxy with a throwing getPrototypeOf or getOwnPropertyDescriptor trap. This is pre-existing and not touched by this PR — just flagging since it's the identical root cause and may be worth the same one-line guard as a follow-up.

Extended reasoning...

Summary

This PR correctly fixes a null deref in JSC__JSValue__forEachPropertyImpl where iterating->getPrototype(globalObject).getObject() is called on a potentially-empty JSValue. However, the identical pattern remains unfixed at src/jsc/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;
}

There is no exception check or empty-JSValue guard between the getOwnPropertyDescriptor call and the .getObject() call on the next line.

How it manifests

As the PR description explains, when getPrototype() is called on a ProxyObject and the getPrototypeOf trap throws (or there is already a pending exception, causing ProxyObject::performGetPrototype to early-return {} via RETURN_IF_EXCEPTION), it returns an empty JSValue. An empty JSValue reports isCell() == true but asCell() == nullptr, so .getObject() calls JSCell::getObject() on nullptr and segfaults — exactly the crash this PR fixes in bindings.cpp.

Why the existing guard at line 1823 doesn't prevent it

allPropertyKeys() at line 1818 also walks the prototype chain, and NAPI_RETURN_IF_EXCEPTION(env) at line 1823 would catch an unconditionally-throwing getPrototypeOf trap on that first walk. However, the descriptor-filtering loop at lines 1833–1842 walks the chain again per key, and is reachable past line 1823 in at least two ways:

  1. Throwing getOwnPropertyDescriptor trap. Line 1836 calls getOwnPropertyDescriptor on the Proxy. If that trap throws, it returns false (loop body entered) and leaves a pending exception. Line 1837 then calls getPrototype() on the same Proxy with the exception pending → empty JSValue → null deref. The ownKeys/getPrototypeOf traps used by allPropertyKeys need not throw for this path.
  2. Stateful getPrototypeOf trap. A trap that succeeds during allPropertyKeys but throws on a subsequent invocation (e.g. throws on the Nth call) reaches line 1837 directly.

Step-by-step proof (path 1)

  1. Native addon calls napi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_writable, ...) where obj's prototype is new Proxy(target, { getOwnPropertyDescriptor() { throw new Error('x') } }).
  2. Line 1818: allPropertyKeys walks the chain via getPrototype/ownKeys — no getOwnPropertyDescriptor trap invoked, no throw. Line 1823 passes.
  3. Line 1827: key_filter & filter_by_any_descriptor is truthy (napi_key_writable), so we enter the per-key filter loop.
  4. For some key, line 1836 calls getOwnPropertyDescriptor on obj → not own → loop iterates, current_object advances to the Proxy.
  5. Line 1836 (next iteration): getOwnPropertyDescriptor on the Proxy invokes the trap → throws → returns false, pending exception set.
  6. Line 1837: current_object->getPrototype(globalObject) on the Proxy with a pending exception returns JSValue{}.
  7. .getObject() on the empty value: isCell() → true, asCell() → nullptr, nullptr->getObject() → segfault.

Impact

A native addon calling napi_get_all_property_names with napi_key_include_prototypes and any of napi_key_enumerable | napi_key_writable | napi_key_configurable on user-supplied objects can be crashed by a hostile Proxy in the prototype chain. Same severity class as the bug this PR fixes (process crash from JS-observable input), just gated behind N-API rather than Bun.inspect.

Suggested fix

Apply the same guard the PR uses in bindings.cpp:

JSValue protoVal = current_object->getPrototype(globalObject);
NAPI_RETURN_IF_EXCEPTION(env); // or CLEAR_IF_EXCEPTION + break
JSObject* proto = protoVal ? protoVal.getObject() : nullptr;

(and ideally also check for an exception after getOwnPropertyDescriptor at line 1836).

Severity

Pre-existing. This PR does not touch napi.cpp, add callers to it, or change its behavior in any way. Flagging only because it is the identical root cause and the fix is the same one-liner — fine to defer to a follow-up.

}
}

Expand Down
23 changes: 23 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 @@ -453,6 +453,29 @@ const fixture = [
),
];

describe("Proxy in prototype chain", () => {
it("inspecting an object whose prototype is a Proxy with a throwing getPrototypeOf trap does not crash", () => {
const obj = {};
const proto = Object.getPrototypeOf(obj);
Object.setPrototypeOf(
obj,
new Proxy(proto, {
getPrototypeOf() {
throw new Error("nope");
},
}),
);
expect(() => Bun.inspect(obj)).not.toThrow();
});

it("inspecting an object whose prototype is a Proxy wrapping a native prototype does not crash", () => {
const e = expect({});
const proto = Object.getPrototypeOf(e);
Object.setPrototypeOf(e, new Proxy(proto, {}));
expect(() => Bun.inspect(e)).not.toThrow();
});
});

describe("crash testing", () => {
for (let input of fixture) {
it(`inspecting "${input.toString().slice(0, 20).replaceAll("\n", "\\n")}" doesn't crash`, async () => {
Expand Down
Loading