Skip to content

Fix null deref in Bun.inspect when Proxy prototype throws #30200

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

Fix null deref in Bun.inspect when Proxy prototype throws #30200

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
984985c
Fix null deref in Bun.inspect when Proxy prototype throws
robobun May 3, 2026
8d5e31f
Remove redundant revoked-proxy test case
robobun May 3, 2026
671b57b
[autofix.ci] apply automated fixes
autofix-ci[bot] May 3, 2026
bc4e2fc
Retrigger CI (previous build had 117 expired jobs due to agent outage)
robobun May 4, 2026
216d8b3
Retrigger CI
robobun May 5, 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 3, 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 unguarded getPrototype(globalObject).getObject() pattern still exists in napi_get_all_property_names at src/bun.js/bindings/napi.cpp:1836-1842. A native addon enumerating with napi_key_include_prototypes + a writable/enumerable/configurable filter on an object whose prototype chain has a Proxy with a throwing getOwnPropertyDescriptor/getPrototypeOf trap can hit the identical null deref — might be worth applying the same empty-value + exception guard there in a follow-up.

Extended reasoning...

What the bug is

This PR fixes a null deref in forEachProperty where getPrototype() on a Proxy returns an empty JSValue (not jsNull()) when the trap throws, and .getObject() on an empty value passes isCell() (0 & NotCellMask == 0) and then dereferences a null JSCell*. The exact same pattern exists, untouched and unguarded, in napi_get_all_property_names at src/bun.js/bindings/napi.cpp:1836-1842:

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 RETURN_IF_EXCEPTION / CLEAR_IF_EXCEPTION between getOwnPropertyDescriptor / getPrototype and .getObject(), and no check that the prototype value is non-empty before calling .getObject() on it.

How it manifests

A native addon calls:

napi_get_all_property_names(env, obj, napi_key_include_prototypes,
                             napi_key_writable /* or enumerable/configurable */,
                             napi_key_numbers_to_strings, &result);

on a JS object whose prototype chain contains a Proxy with hostile traps. Once the per-key filter loop is entered (line 1833), each iteration calls getOwnPropertyDescriptor and then getPrototype on current_object. If either of those throws while current_object is a Proxy, getPrototype() returns an empty JSValue and empty.getObject() segfaults reading m_type from a null JSCell* — the same crash mechanism described in this PR's description.

Why existing code doesn't prevent it

There is a NAPI_RETURN_IF_EXCEPTION at line 1823 after allPropertyKeys, which catches a trivially throwing getPrototypeOf trap (since allPropertyKeys walks the chain once). However:

  • The inner loop at 1833-1842 re-walks the chain once per key with no exception checks at all, so a stateful trap that succeeds the first time and throws on a later call slips past line 1823.
  • More directly, a Proxy getOwnPropertyDescriptor trap that throws makes the while condition return false with a pending exception; the very next line then calls getPrototype() on the Proxy, and ProxyObject::performGetPrototype bails out early with {} because an exception is already pending.

In both cases the empty value reaches .getObject() unchecked.

Step-by-step proof

  1. JS side: 
    const target = { foo: 1 };
const proxy = new Proxy(target, {
  getOwnPropertyDescriptor() { throw new Error("boom"); },
});
const obj = Object.setPrototypeOf({}, proxy);
addon.enumerate(obj); // -> napi_get_all_property_names(..., napi_key_include_prototypes, napi_key_writable, ...)
  2. allPropertyKeys (line 1818) walks the chain via [[OwnPropertyKeys]] + [[GetPrototypeOf]]; neither trap is overridden to throw here, so it succeeds and returns ["foo"]. NAPI_RETURN_IF_EXCEPTION at 1823 passes.
  3. key_filter & filter_by_any_descriptor is true (napi_key_writable), so we enter the per-key loop with key "foo".
  4. key_mode == napi_key_include_prototypes, so current_object = obj and we evaluate the while condition: obj->getOwnPropertyDescriptor(globalObject, "foo", desc) → obj has no own "foo", returns false, no throw.
  5. Loop body: obj->getPrototype(globalObject) returns the Proxy (no trap, target's proto), .getObject() → proxy. current_object = proxy.
  6. while condition again: proxy->getOwnPropertyDescriptor(globalObject, "foo", desc) invokes the getOwnPropertyDescriptor trap, which throws. getOwnPropertyDescriptor returns false with the exception left pending in scope.
  7. Loop body: proxy->getPrototype(globalObject) → ProxyObject::performGetPrototype sees the pending exception (or re-enters JS with it pending) and returns JSValue() (empty).
  8. JSValue().getObject(): isCell() checks (0 & NotCellMask) == 0 → true; asCell() returns nullptr; JSCell::getObject() reads this->m_type → null deref / segfault.

(An equivalent path exists with a stateful getPrototypeOf trap that returns null during allPropertyKeys and throws inside the per-key loop.)

Impact

Process crash (SIGSEGV) reachable from a native addon that calls napi_get_all_property_names with napi_key_include_prototypes and any descriptor-filter bit on an attacker-/user-controlled object. Narrower than the Bun.inspect case (requires a native addon and a specific flag combination) but the same crash class.

How to fix

Apply the same guard this PR adds at bindings.cpp:5448-5451:

while (!current_object->getOwnPropertyDescriptor(globalObject, key.toPropertyKey(globalObject), desc)) {
    NAPI_RETURN_IF_EXCEPTION(env);              // or CLEAR_IF_EXCEPTION + break
    JSValue protoVal = current_object->getPrototype(globalObject);
    NAPI_RETURN_IF_EXCEPTION(env);
    JSObject* proto = protoVal ? protoVal.getObject() : nullptr;
    if (!proto) break;
    current_object = proto;
}

Scope

This is pre-existing — the PR does not touch napi.cpp, add callers, or otherwise interact with napi_get_all_property_names. Flagging it only because it's the identical pattern being fixed here and is a natural follow-up.

}
}

Expand Down
33 changes: 33 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,36 @@ it("CustomEvent", () => {
}"
`);
});

describe("Proxy in prototype chain", () => {
it("does not crash when a getter throws through a Proxy prototype", () => {
const proto = {};
Object.defineProperty(proto, "thrower", {
get() {
throw new Error("getter threw");
},
enumerable: true,
configurable: true,
});
proto.foo = function () {};
proto.bar = function () {};

const obj = Object.create(proto);
Object.setPrototypeOf(obj, new Proxy(proto, {}));
expect(Bun.inspect(obj)).toContain("foo");
});
Comment on lines +777 to +792

@coderabbitai coderabbitai Bot May 3, 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.

🧹 Nitpick | 🔵 Trivial | 💤 Low value

Optional: Simplify test setup for clarity.

Line 789 creates obj with proto as its prototype, but line 790 immediately replaces that prototype with a Proxy wrapping the same proto. The initial Object.create(proto) is redundant. Consider simplifying to const obj = {}; to match test 2's clearer pattern.

♻️ Suggested simplification 
-    const obj = Object.create(proto);
-    Object.setPrototypeOf(obj, new Proxy(proto, {}));
+    const obj = Object.create(new Proxy(proto, {}));

or

-    const obj = Object.create(proto);
+    const obj = {};
     Object.setPrototypeOf(obj, new Proxy(proto, {}));
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/js/bun/util/inspect.test.js` around lines 777 - 792, The test creates
obj via Object.create(proto) and then immediately replaces its prototype with a
Proxy, which is redundant; simplify by creating a plain object (const obj = {})
and then set its prototype to the Proxy of proto (use Object.setPrototypeOf(obj,
new Proxy(proto, {}))). Update the test "does not crash when a getter throws
through a Proxy prototype" to use proto and obj as described so the behavior is
unchanged but the setup is clearer.


it("does not crash when a Proxy getPrototypeOf trap throws", () => {
const proto = { foo: 1, bar: 2 };
const obj = {};
Object.setPrototypeOf(
obj,
new Proxy(proto, {
getPrototypeOf() {
throw new Error("trap threw");
},
}),
);
expect(Bun.inspect(obj)).toContain("foo");
});
});
Loading