Skip to content

Fix null deref in Bun.inspect when Proxy getPrototypeOf trap throws #30517

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
+27 −1
Closed

Fix null deref in Bun.inspect when Proxy getPrototypeOf trap throws #30517

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
5 changes: 4 additions & 1 deletion src/jsc/bindings/bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5444,7 +5444,10 @@
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;

Check notice on line 5450 in src/jsc/bindings/bindings.cpp

View check run for this annotation

Claude / Claude Code Review

Same null-deref pattern remains in napi.cpp 

Heads-up: the same unsafe pattern exists at `src/jsc/bindings/napi.cpp:1837` in `napi_get_all_property_names` — `current_object->getPrototype(globalObject).getObject()` with no exception check, so a Proxy `getPrototypeOf` trap that throws will null-deref there too. This is pre-existing and not touched by this PR, but since a grep for `getPrototype(globalObject).getObject()` finds it, you may want to apply the same `CLEAR_IF_EXCEPTION` + empty-check fix there in this PR or a follow-up. Non-blocki
Comment on lines +5447 to +5450

@claude claude Bot May 11, 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 unsafe pattern exists at src/jsc/bindings/napi.cpp:1837 in napi_get_all_property_names — current_object->getPrototype(globalObject).getObject() with no exception check, so a Proxy getPrototypeOf trap that throws will null-deref there too. This is pre-existing and not touched by this PR, but since a grep for getPrototype(globalObject).getObject() finds it, you may want to apply the same CLEAR_IF_EXCEPTION + empty-check fix there in this PR or a follow-up. Non-blocking.

Extended reasoning...

What the bug is

This PR correctly fixes a null dereference in JSC__JSValue__forEachPropertyImpl (bindings.cpp:5447) where iterating->getPrototype(globalObject).getObject() crashes when a Proxy's getPrototypeOf trap throws. However, the identical pattern remains 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();  // <-- same unsafe pattern
    if (!proto) {
        break;
    }
    current_object = proto;
}

There is no RETURN_IF_EXCEPTION / CLEAR_IF_EXCEPTION between the getPrototype() call and .getObject(), so the exact crash class fixed by this PR still exists in the napi path.

Code path that triggers it

A native addon calls napi_get_all_property_names with:

  • key_mode = napi_key_include_prototypes (so the inner while-loop walks the prototype chain), and
  • key_filter containing any of napi_key_enumerable | napi_key_writable | napi_key_configurable (so the descriptor-filtering block at line 1827 is entered),

on an object whose prototype chain contains a Proxy with a throwing getPrototypeOf trap.

Why existing code doesn't prevent it

There's a NAPI_RETURN_IF_EXCEPTION(env) at line 1823 after the initial allPropertyKeys() walk, so a trivially-always-throwing trap would be caught there. But a stateful trap — e.g., one that returns normally for the first N calls (during allPropertyKeys's prototype walk) and throws on a subsequent call (during the per-key descriptor-filtering walk at line 1837) — passes the line-1823 check and reaches line 1837. At that point getPrototype() throws and returns an empty JSValue; per the PR description's own analysis, an empty JSValue passes isCell() but asCell() is nullptr, so .getObject() invokes JSCell::getObject() on null → segfault.

Step-by-step proof

  1. JS sets up: let n = 0; const p = new Proxy({}, { getPrototypeOf() { if (n++ > 3) throw new Error('nope'); return null; } }); const obj = Object.create(p); obj.x = 1;
  2. Native addon calls napi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_enumerable, napi_key_numbers_to_strings, &result).
  3. Line 1818-ish: allPropertyKeys() walks the chain, calling the trap a few times; n increments but stays under the threshold; no exception → line 1823 passes.
  4. Line 1827: key_filter & filter_by_any_descriptor is true → enter filtering loop.
  5. For key "x", line 1836: getOwnPropertyDescriptor on obj succeeds, loop body skipped. But for any key contributed by the prototype (or if the descriptor lookup misses on obj), the while-loop at 1836 runs and line 1837 calls current_object->getPrototype(globalObject).
  6. The trap now throws (n > 3). getPrototype() returns JSValue() (empty).
  7. JSValue().getObject() → isCell() is true for empty (bits == 0 has no number/undefined/null tag) → asCell() returns nullptr → nullptr->getObject() reads type() through a null this → crash.

Impact

Process crash (SIGSEGV) reachable from any native addon that uses napi_get_all_property_names with prototype inclusion + descriptor filtering on user-controlled objects. Lower exposure than the Bun.inspect path since it requires a native addon, but it's the same crash class.

How to fix

Apply the same pattern as this PR:

JSValue protoVal = current_object->getPrototype(globalObject);
CLEAR_IF_EXCEPTION(scope);  // or NAPI_RETURN_IF_EXCEPTION(env) if propagating is preferred
JSObject* proto = protoVal ? protoVal.getObject() : nullptr;
if (!proto) break;

Severity

Pre-existing — this PR does not touch napi.cpp, add callers to it, or otherwise interact with this code path. Mentioning it only because the PR's stated goal is fixing exactly this crash class and a grep for getPrototype(globalObject).getObject() finds this remaining instance. Non-blocking; fine to address in 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 @@ -451,6 +451,29 @@ const fixture = [
},
},
),
() =>
Object.setPrototypeOf(
{ yolo: 1 },
new Proxy(
{},
{
getPrototypeOf() {
throw new Error("nope");
},
},
),
),
() =>
Object.create(
new Proxy(
{ yolo: 1 },
{
getPrototypeOf() {
throw new Error("nope");
},
},
),
),
];

describe("crash testing", () => {
Expand Down
Loading