Skip to content

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

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
+53 −4
Closed

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

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
12 changes: 9 additions & 3 deletions src/bun.js/bindings/bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5369,10 +5369,11 @@
}

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 @@
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();

Check notice on line 5453 in src/bun.js/bindings/bindings.cpp

View check run for this annotation

Claude / Claude Code Review

Same getPrototype().getObject() null-deref pattern remains in napi.cpp (pre-existing) 

Heads-up (pre-existing, not introduced here): the same `getPrototype(globalObject).getObject()` pattern without an exception check still exists at `src/bun.js/bindings/napi.cpp:1837` in `napi_get_all_property_names`. A throwing Proxy `getPrototypeOf`/`getOwnPropertyDescriptor` trap on the prototype chain will cause the same null deref there when called with `napi_key_include_prototypes` + a descriptor filter — might be worth a follow-up since it's the identical bug class.
Comment on lines +5448 to +5453

@claude claude Bot May 4, 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 here): the same getPrototype(globalObject).getObject() pattern without an exception check still exists at src/bun.js/bindings/napi.cpp:1837 in napi_get_all_property_names. A throwing Proxy getPrototypeOf/getOwnPropertyDescriptor trap on the prototype chain will cause the same null deref there when called with napi_key_include_prototypes + a descriptor filter — might be worth a follow-up since it's the identical bug class.

Extended reasoning...

What the bug is

This PR fixes a null deref in JSC__JSValue__forEachPropertyImpl where iterating->getPrototype(globalObject).getObject() was called without checking for a pending exception. The exact same vulnerable pattern still exists in napi_get_all_property_names at src/bun.js/bindings/napi.cpp:1837:

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 between getPrototype(globalObject) and .getObject(), and the preceding getOwnPropertyDescriptor (also a Proxy trap) is similarly unchecked.

Code path that triggers it

A native addon calls napi_get_all_property_names with:

  • key_mode = napi_key_include_prototypes
  • key_filter containing any of napi_key_enumerable | napi_key_writable | napi_key_configurable (so the descriptor-filtering branch at line 1827 runs)
  • An object whose prototype chain contains a Proxy with a throwing getPrototypeOf trap, or a throwing getOwnPropertyDescriptor trap.

The earlier allPropertyKeys call (line 1818) goes through the Proxy ownKeys/getPrototypeOf traps and is guarded by NAPI_RETURN_IF_EXCEPTION at line 1823. However, the inner loop at line 1836 invokes getOwnPropertyDescriptor — a different trap that allPropertyKeys does not exercise — so a Proxy whose ownKeys/getPrototypeOf succeed but whose getOwnPropertyDescriptor throws will sail past line 1823 and blow up here.

Why the existing guard doesn't help

When a Proxy trap throws (or there is already a pending exception), JSObject::getPrototype / ProxyObject::getPrototype returns an empty JSValue. On JSVALUE64, an empty JSValue is encoded as 0, which passes the isCell() check (0 & NotCellMask == 0). getObject() therefore evaluates asCell()->isObject() on a null JSCell* and segfaults before reaching the if (!proto) break; on line 1838 — that null check never gets a chance to run.

Step-by-step proof

  1. JS sets up obj whose prototype is new Proxy(target, { getOwnPropertyDescriptor() { throw new Error('nope'); } }) and target has a key foo.
  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. allPropertyKeys walks the chain via ownKeys/getPrototypeOf (no throw), returns ['foo']. NAPI_RETURN_IF_EXCEPTION passes.
  4. Filter loop runs for 'foo'. object->getOwnPropertyDescriptor on obj itself returns false (own miss), enters loop body.
  5. obj->getPrototype(globalObject) returns the Proxy; .getObject() returns the Proxy object; current_object = proxy.
  6. Next iteration: proxy->getOwnPropertyDescriptor(...) invokes the trap, which throws. Returns false → loop body again with a pending exception.
  7. proxy->getPrototype(globalObject) runs ProxyObject::getPrototype, which begins with RETURN_IF_EXCEPTION(scope, { }) → returns empty JSValue.
  8. .getObject() on empty → asCell() is nullptr → nullptr->isObject() → SIGSEGV.

(The simpler variant — a Proxy whose getPrototypeOf throws only on the second call — also reaches step 7 directly.)

Impact

Same as the bug this PR fixes: a hard crash (segfault) of the Bun process, but gated behind a native addon calling this specific N-API with prototype-walking + descriptor filtering on adversarial input. Less reachable than the Bun.inspect case, but still the same crash class.

Fix

Mirror what this PR does in bindings.cpp:

JSValue protoVal = current_object->getPrototype(globalObject);
NAPI_RETURN_IF_EXCEPTION(env);   // or break + report
JSObject* proto = protoVal.getObject();
if (!proto) break;

and add an exception check after getOwnPropertyDescriptor in the loop condition. This is pre-existing and untouched by this PR — flagging only because it's the identical bug class and a natural follow-up.

}
}

Expand Down
45 changes: 44 additions & 1 deletion test/js/bun/util/inspect.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, expect, it } from "bun:test";
import { normalizeBunSnapshot, tmpdirSync } from "harness";
import { bunEnv, bunExe, normalizeBunSnapshot, tmpdirSync } from "harness";
import { join } from "path";
import util from "util";
it("prototype", () => {
Expand Down Expand Up @@ -465,6 +465,49 @@ describe("crash testing", () => {
}
});
}

it.concurrent.each([
[
"throwing get trap",
`
const o = {};
Object.setPrototypeOf(o, new Proxy({ foo: 1 }, {
get(t, k) { if (typeof k === "symbol") return undefined; throw new Error("nope"); },
}));
Bun.inspect(o);
`,
],
[
"throwing getPrototypeOf trap",
`
const o = {};
Object.setPrototypeOf(o, new Proxy({ foo: 1 }, {
getPrototypeOf() { throw new Error("nope"); },
}));
Bun.inspect(o);
`,
],
[
"getter throws after side-effect from prior getter",
`
const { expect } = Bun.jest("");
const e = expect(1);
Object.setPrototypeOf(e, new Proxy(Object.getPrototypeOf(e), {}));
Bun.inspect(e);
`,
],
])("Proxy prototype with %s doesn't crash", async (_, code) => {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", code + '\nconsole.log("OK");'],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(stdout).toBe("OK\n");
expect(exitCode).toBe(0);
Comment on lines +506 to +509

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Filter known ASAN startup noise before asserting empty stderr.

With bunEnv, ASAN can emit a known warning line on stderr; asserting stderr === "" can make this flaky on ASAN CI even when behavior is correct.

Suggested patch 
-    expect(stderr).toBe("");
+    const stderrLines = stderr
+      .split("\n")
+      .filter(line => !line.startsWith("WARNING: ASAN interferes"))
+      .filter(Boolean);
+    expect(stderrLines).toEqual([]);

Based on learnings: In oven-sh/bun test files that spawn subprocesses using bunEnv, suppress known ASAN startup noise by filtering lines starting with "WARNING: ASAN interferes" before asserting stderr is empty.

🤖 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 506 - 509, The test currently
asserts stderr is exactly empty which flakes under ASAN; update the assertion to
filter out lines beginning with "WARNING: ASAN interferes" from the
proc.stderr.text() result before asserting emptiness. Concretely, after awaiting
proc.stderr.text() (variable stderr) split into lines, remove any lines that
startWith "WARNING: ASAN interferes", rejoin and then assert the filtered stderr
is "". Keep the existing checks for stdout ("OK\n") and exitCode (0) unchanged.

});
});

it("possibly formatted emojis log", () => {
Expand Down
Loading