Skip to content

Fix null deref in forEachProperty when Proxy prototype getter throws #30541

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 null deref in forEachProperty when Proxy prototype getter throws #30541

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
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 hasSlot = object->getPropertySlot(globalObject, property, slot);
// Ignore exceptions from "Get" proxy traps.
CLEAR_IF_EXCEPTION(scope);
if (!hasSlot)
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 "getPrototype" trap.
CLEAR_IF_EXCEPTION(scope);
iterating = proto ? proto.getObject() : nullptr;
}
}

Expand Down
52 changes: 52 additions & 0 deletions test/js/bun/util/inspect-proxy-prototype.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { describe, expect, test } from "bun:test";

describe("Bun.inspect with Proxy in prototype chain", () => {
test("does not crash when a getter reached through a Proxy prototype throws", () => {
// The Expect prototype has getters (.rejects / .resolves) that mutate state and can
// throw when invoked in sequence. Going through a Proxy forces the slow property
// iteration path which invokes those getters.
const e = expect(1);
const proto = Object.getPrototypeOf(e);
Object.setPrototypeOf(e, new Proxy(proto, {}));
const out = Bun.inspect(e);
expect(out).toContain("toBe");
});

test("does not crash when a Proxy getPrototypeOf trap throws during iteration", () => {
class Foo {
bar() {}
}
const obj = new Foo();
const proto = Object.getPrototypeOf(obj);
Object.setPrototypeOf(
obj,
new Proxy(proto, {
getPrototypeOf() {
throw new Error("nope");
},
}),
);
const out = Bun.inspect(obj);
expect(out).toContain("bar");
});

test("does not crash when a Proxy get trap throws during iteration", () => {
class Foo {
bar() {}
baz() {}
}
const obj = new Foo();
const proto = Object.getPrototypeOf(obj);
let count = 0;
Object.setPrototypeOf(
obj,
new Proxy(proto, {
get(t, k, r) {
if (++count > 3) throw new Error("get fail");
return Reflect.get(t, k, r);

Check warning on line 46 in test/js/bun/util/inspect-proxy-prototype.test.ts

View check run for this annotation

Claude / Claude Code Review

Test 'Proxy get trap throws' never actually triggers the throw 

This test never actually triggers the throw: the `get` trap is invoked exactly 3 times during `Bun.inspect(obj)` (once for `Symbol(nodejs.util.inspect.custom)`, then once each for `bar` and `baz`), so `++count > 3` is never true and the test passes even without the bindings.cpp fix. Lower the threshold to e.g. `> 1` so the trap actually throws inside `getPropertySlot` and exercises the `CLEAR_IF_EXCEPTION`-before-`continue` path. (Nit — tests 1 and 2 already cover the actual fixes, this one is j
Comment on lines +44 to +46

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

🟡 This test never actually triggers the throw: the get trap is invoked exactly 3 times during Bun.inspect(obj) (once for Symbol(nodejs.util.inspect.custom), then once each for bar and baz), so ++count > 3 is never true and the test passes even without the bindings.cpp fix. Lower the threshold to e.g. > 1 so the trap actually throws inside getPropertySlot and exercises the CLEAR_IF_EXCEPTION-before-continue path. (Nit — tests 1 and 2 already cover the actual fixes, this one is just currently vacuous.)

Extended reasoning...

What the test claims vs. what it does

The third test is titled "does not crash when a Proxy get trap throws during iteration" and asserts expect(() => Bun.inspect(obj)).not.toThrow(). For that to be a meaningful regression test, the get trap must actually throw at some point during forEachPropertyImpl's slow-path iteration so that the new CLEAR_IF_EXCEPTION(scope) before continue (bindings.cpp:5372–5376) is exercised. As written, the trap never throws.

Step-by-step trace of the get-trap invocations

Running this scenario on bun and counting trap invocations gives exactly 3:

  1. Symbol(nodejs.util.inspect.custom) lookup — Before forEachProperty runs, Tag.getAdvanced (ConsoleObject.zig:1246) calls fastGet(.inspectCustom), which walks obj → proxy and invokes ProxyObject::getOwnPropertySlot with InternalMethodType::Get, calling the trap once. count = 1, 1 > 3 is false.
  2. bar — In the slow path, getOwnPropertyNames on the proxy (no ownKeys trap) returns [constructor, bar, baz]; constructor is filtered at bindings.cpp:5362. For bar, object->getPropertySlot(globalObject, property, slot) walks obj → proxy and invokes performGet, calling the trap. count = 2, 2 > 3 is false. The slot is a plain value slot, so slot.getValue() later returns the cached value with no second trap call.
  3. baz — Same as above. count = 3, 3 > 3 is false.
  4. proxy->getPrototype() (no getPrototypeOf trap) returns Object.prototype, the loop terminates.

The condition if (++count > 3) throw first fires on the 4th invocation, which never happens. No exception is ever raised inside forEachPropertyImpl, so neither the new CLEAR_IF_EXCEPTION at 5374 nor the getPrototype guard at 5448–5451 is reached via this test.

Why this matters

This was verified empirically on a bun build without this PR's fix: the test body runs to completion with count == 3, no throw, no crash — so the test would pass even on the broken code it's meant to guard against. It currently provides no regression coverage for the throwing-get-trap path described in its title.

Why this is only a nit

The first test (the expect(1) + Proxy-prototype repro from the fuzzer) does exercise the throwing-getter path that crashed pre-fix and covers bindings.cpp:5372–5376, and the second test covers the getPrototype guard at 5448–5451. So overall regression coverage for the fix exists; this third test is supplementary and just ineffective as written.

Fix

Lower the threshold so the trap throws during the getPropertySlot loop:

get(t, k, r) {
  if (++count > 1) throw new Error("get fail");
  return Reflect.get(t, k, r);
},

With > 1, the inspectCustom lookup succeeds (count=1), then bar throws inside getPropertySlot (count=2) — exercising exactly the code path this PR fixed. > 2 would also work (throws on baz).

},
}),
);
expect(() => Bun.inspect(obj)).not.toThrow();
});
});
Loading