inspect: clear exceptions when walking Proxy prototype chain

[robobun]

Fixes a null-pointer crash in Bun.inspect / console.log when an object's prototype is a Proxy and property access through that proxy throws.

Root cause

In the slow-path property enumeration loop of forEachProperty:

1. getPropertySlot() invokes the Proxy [[Get]] internal method, which eagerly calls getters on the target. If a getter throws, getPropertySlot() returns false with a pending exception.
2. The code did if (!getPropertySlot(...)) continue; — skipping past the CLEAR_IF_EXCEPTION that follows, leaving the exception pending.
3. At the end of the iteration, iterating->getPrototype(globalObject) is called with the exception still pending. On a ProxyObject this short-circuits and returns an empty JSValue, and .getObject() on an empty value dereferences a null JSCell.

Additionally, getPrototype() itself can throw when a Proxy has a throwing getPrototypeOf trap, with the same result.

Fix

• Clear the exception before continue when getPropertySlot() fails.
• Guard the getPrototype() result: clear any exception and break on an empty value before calling .getObject().

Repro

const obj = {};
const proto = {
  get ok() { return 1; },
  get bad() { throw new Error("nope"); },
  get ok2() { return 2; },
};
Object.setPrototypeOf(obj, new Proxy(proto, {}));
Bun.inspect(obj); // used to crash, now prints { ok: 1, ok2: 2 }

[robobun]

^{Updated 7:05 PM PT - May 4th, 2026}

❌ @robobun, your commit 2e35e3a has 3 failures in Build #51398 (All Failures):

• test/js/bun/s3/s3-storage-class.test.ts - code 1 on 🍎 14 aarch64
• Failed to create agent for 🪟 x64 - build-bun
• test/js/bun/test/parallel/test-http-should-emit-close-when-connection-is-aborted.ts - timeout on 🪟 11 aarch64
• test/js/bun/test/parallel/test-http-should-emit-close-when-connection-is-aborted.ts - timeout on 🪟 2019 x64-baseline

---

🧪   To try this PR locally:

bunx bun-pr 29845

That installs a local version of the PR into your bun-29845 executable, so you can run:

bun-29845 --bun

[coderabbitai]

Warning

Rate limit exceeded

@dylan-conway has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 21 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6463d9b1-f73e-481b-b79f-b2d0ed663ca8

📥 Commits

Reviewing files that changed from the base of the PR and between 38c93aa and 19d9992.

📒 Files selected for processing (1)

• test/js/bun/util/inspect.test.js

Walkthrough

Improves exception-safety in property/prototype enumeration when proxies are involved by explicitly checking getPropertySlot success and clearing proxy-related exceptions. Adds regression tests for Bun.inspect robustness with prototype proxies.

Changes

Cohort / File(s)	Summary
Enumeration Safety src/bun.js/bindings/bindings.cpp	Enhanced property/prototype enumeration to store getPropertySlot success in a hasProperty flag, evaluate getPrototype into a JSValue nextProto, and clear exceptions from proxy Get and getPrototypeOf traps while stopping chain traversal if nextProto is falsy.
Proxy Robustness Tests test/js/bun/util/inspect.test.js	Added three crash-regression tests for Bun.inspect with proxies: prototype proxy with throwing getters, prototype proxy with throwing getPrototypeOf trap, and prototype swapping with expect({}) verification.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main fix: clearing exceptions when walking a Proxy prototype chain in the inspect functionality.
Description check	✅ Passed	The description provides comprehensive information including root cause, detailed fix explanation, and a working repro example, though it exceeds the template's minimal structure.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Review rate limit: 0/5 reviews remaining, refill in 4 minutes and 21 seconds.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/js/bun/util/inspect.test.js`:
- Around line 777-793: The current assertion in the test "does not crash when a
Proxy prototype has a throwing getter" is too loose (it only checks for "ok");
update the assertion after calling Bun.inspect(obj) to explicitly assert that
both non-throwing prototype properties are present (e.g., assert presence of
"ok" and "ok2") so the test verifies traversal continued past the throwing
getter; locate the test by its description and the call to Bun.inspect and
replace the single toContain("ok") check with assertions that require both "ok"
and "ok2".

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2eeb350d-a6b5-428e-9721-e9532c4dacaf

📥 Commits

Reviewing files that changed from the base of the PR and between 4d615e8 and 38c93aa.

📒 Files selected for processing (2)

• src/bun.js/bindings/bindings.cpp
• test/js/bun/util/inspect.test.js

[github-actions]

This PR may be a duplicate of:

1. Fix null deref in forEachProperty with Proxy in prototype chain #29816 - Fixes the same null deref in forEachPropertyImpl by clearing exceptions after getPropertySlot and guarding getPrototype() on Proxy
2. inspect: handle throwing Proxy getPrototypeOf in forEachProperty #29814 - Fixes the same forEachPropertyImpl Proxy prototype chain crash with the same two-part fix (exception clearing + getPrototype guard)
3. fix(inspect): don't crash when a Proxy in the prototype chain throws #29642 - Fixes the same forEachPropertyImpl crash plus extends the fix to napi_get_all_property_names
4. Fix null JSCell deref in Bun lazy property callbacks and forEachProperty #29671 - Includes the same forEachPropertyImpl getPrototype() guard fix alongside lazy-property fixes
5. getPrototype exception checks #24985 - Fixes getPrototype exception handling in forEachPropertyImpl and other call sites addressing the same crash

🤖 Generated with Claude Code

[claude]

LGTM — small defensive fix that swaps the order of exception-clearing vs. continue and guards an empty getPrototype() result, with three regression tests.

Extended reasoning...

Overview

This PR touches two files: a ~10-line change in forEachPropertyImpl in src/bun.js/bindings/bindings.cpp and three new regression tests in test/js/bun/util/inspect.test.js. The C++ change has two parts: (1) store the result of getPropertySlot() so CLEAR_IF_EXCEPTION(scope) runs before the continue, and (2) capture getPrototype() into a local JSValue, clear any exception, and break on an empty value before calling .getObject(). Both are pure hardening — they convert a null-deref crash path into a graceful skip/break, with no change to the non-error path.

Security risks

None. This is a crash fix in the property-enumeration path used by Bun.inspect / console.log. It does not touch auth, crypto, permissions, network, or filesystem code. Clearing the exception and bailing out is consistent with the existing pattern in this function (which already swallows getter exceptions a few lines below).

Level of scrutiny

Low-to-moderate. The change is mechanical and strictly defensive — the previous code would either work identically (when no exception is pending) or crash (when one is). The new code preserves the former and fixes the latter. The outer for loop already terminates on iterating == nullptr, so nextProto.getObject() returning null for jsNull() is handled exactly as before.

Other factors

• Three targeted regression tests cover both the throwing-getter and throwing-getPrototypeOf trap cases, plus an expect({}) repro. CodeRabbit's nit about loose assertions was addressed in 532c54c.
• The only bug-hunter finding is an explicitly pre-existing identical pattern in napi.cpp (napi_get_all_property_names), which is out of scope here and reportedly handled by #29642 — left as an FYI inline comment.
• The build-zig CI failures on 38c93aa are in scripts/build/ci.ts across all platforms; this PR touches no Zig code, so those are unrelated infra issues.
• The duplicate-PR bot lists several overlapping PRs — a maintainer will want to pick one to land, but that's a coordination question, not a correctness one for this diff.

[claude]

🟣 Heads-up (pre-existing, not introduced by this PR): the same getPrototype(globalObject).getObject() null-deref pattern this PR fixes still exists at src/bun.js/bindings/napi.cpp:1837 in napi_get_all_property_names. A Proxy with a throwing getPrototypeOf/getOwnPropertyDescriptor trap will crash there the same way; PR #29642 reportedly extends the fix to napi, so you may want to apply the same CLEAR_IF_EXCEPTION + empty-value guard there or land that PR alongside.

Extended reasoning...

Summary

This PR correctly fixes a null-pointer crash in forEachPropertyImpl (bindings.cpp:5364-5369) where iterating->getPrototype(globalObject).getObject() dereferences a null JSCell when a Proxy's getPrototypeOf trap throws (or a prior trap leaves an exception pending). However, the identical vulnerable pattern remains untouched at src/bun.js/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;
}

This is pre-existing — the PR doesn't touch napi.cpp, add callers to it, or change its trigger likelihood. It is flagged here only because it is the exact same bug class being fixed, and the duplicate-PR bot on this PR already notes that #29642 covers the napi side.

Code path

napi_get_all_property_names is reachable from any native addon. When called with key_mode == napi_key_include_prototypes and a non-trivial key_filter (e.g. napi_key_writable / napi_key_enumerable / napi_key_configurable), the function enters the descriptor-filtering branch at napi.cpp:1833. For each key it walks the prototype chain:

1. Line 1836 calls current_object->getOwnPropertyDescriptor(...). If current_object is a ProxyObject, this invokes the JS getOwnPropertyDescriptor trap, which can throw. On throw, getOwnPropertyDescriptor returns false with a pending exception, and the loop body executes.
2. Line 1837 calls current_object->getPrototype(globalObject). With an exception already pending — or if current_object is a Proxy whose getPrototypeOf trap throws — ProxyObject::getPrototype short-circuits via RETURN_IF_EXCEPTION and returns an empty JSValue ({ }).
3. .getObject() is called on that empty value. On JSVALUE64, an empty JSValue is encoded as 0, which passes the isCell() check ((0 & NotCellMask) == 0), so asCell() returns nullptr and asCell()->getObject() dereferences null — crashing before the if (!proto) check on line 1838 ever runs.

Why existing code doesn't prevent it

There is no ThrowScope/CatchScope exception clearing between the getOwnPropertyDescriptor call and the getPrototype call, and the result of getPrototype() is chained directly into .getObject() without an empty-value check. The if (!proto) break; guard on line 1838 only handles the case where getPrototype() legitimately returns jsNull()/jsUndefined() (for which .getObject() safely returns nullptr); it cannot help when .getObject() itself segfaults on an empty value.

Step-by-step proof

1. JS creates const p = new Proxy({}, { getPrototypeOf() { throw new Error('boom'); } }); and passes p to a native addon.
2. The addon calls napi_get_all_property_names(env, p, napi_key_include_prototypes, napi_key_writable, napi_key_numbers_to_strings, &result).
3. Enumeration finds at least one key (e.g. inherited from Object.prototype since getOwnPropertyNames includes prototypes here), enters the filter loop, and getOwnPropertyDescriptor on the proxy returns false (no own descriptor on the empty target).
4. current_object->getPrototype(globalObject) invokes the getPrototypeOf trap → throws → returns empty JSValue.
5. emptyJSValue.getObject() → isCell() true → asCell() = nullptr → nullptr->getObject() → SIGSEGV.

The same crash occurs without a getPrototypeOf trap if the getOwnPropertyDescriptor trap throws at step 3, because step 4 then short-circuits on the pending exception and still returns empty.

Impact

Process crash (null-pointer dereference) in any native addon that calls napi_get_all_property_names with napi_key_include_prototypes + a descriptor filter on an object whose prototype chain contains a hostile/buggy Proxy. Lower exposure than the Bun.inspect path fixed in this PR (requires a native addon and a specific key-mode/filter combo), but identical mechanism.

Suggested fix

Apply the same pattern this PR uses in bindings.cpp:

while (!current_object->getOwnPropertyDescriptor(globalObject, key.toPropertyKey(globalObject), desc)) {
    CLEAR_IF_EXCEPTION(scope); // or RETURN_IF_EXCEPTION as appropriate for napi
    JSValue protoValue = current_object->getPrototype(globalObject);
    CLEAR_IF_EXCEPTION(scope);
    if (!protoValue)
        break;
    JSObject* proto = protoValue.getObject();
    if (!proto)
        break;
    current_object = proto;
}

Since #29642 reportedly already does this, the simplest path is to land that PR alongside this one rather than expanding scope here.

[claude]

LGTM — straightforward exception-clearing fix with good regression coverage.

Extended reasoning...

Overview

Two small hunks in src/jsc/bindings/bindings.cpp inside JSC__JSValue__forEachPropertyImpl: (1) reorder getPropertySlot so CLEAR_IF_EXCEPTION runs before the continue (the clearing macro and its comment were already present but were being skipped on the false path), and (2) store getPrototype() into a local, clear any exception, and break on empty before calling .getObject(). Three regression tests added to inspect.test.js covering throwing getters, throwing getPrototypeOf traps, and an expect() object with a Proxy prototype.

Security risks

None. This is a defensive crash fix in the inspect/formatting path — it strictly reduces attack surface by preventing a null-pointer dereference reachable from console.log/Bun.inspect on adversarial Proxy objects. No new code paths, no auth/crypto/permissions involved.

Level of scrutiny

Low-to-moderate. The change is ~10 lines, follows the established CLEAR_IF_EXCEPTION(scope) + empty-JSValue-guard pattern already used throughout this function and JSC bindings generally, and the first hunk simply moves an existing macro call before an early continue. The behavior change is purely additive safety: previously-crashing inputs now produce partial output and continue.

Other factors

• CodeRabbit's only nitpick (loose test assertion) was addressed in a follow-up commit; the test now asserts both ok: 1 and ok2: 2.
• My earlier inline comment flagged a pre-existing identical pattern in napi.cpp — that is out of scope and tracked by other PRs (#29642), not a blocker here.
• The duplicate-PR bot noted several overlapping PRs; that's a maintainer merge-ordering concern, not a correctness issue with this change.
• No CODEOWNERS apply to the touched paths.

[claude]

LGTM — minimal, purely-defensive crash fix with regression tests; the napi.cpp note from my earlier comment is pre-existing and out of scope here.

Extended reasoning...

Overview

This PR fixes a null-pointer crash in JSC__JSValue__forEachPropertyImpl (src/jsc/bindings/bindings.cpp) that occurs when Bun.inspect / console.log walks an object whose prototype chain contains a Proxy with throwing traps. Two hunks: (1) reorder so CLEAR_IF_EXCEPTION runs before the continue when getPropertySlot fails, and (2) store getPrototype() into a local, clear any exception, and break on an empty value before calling .getObject(). Three regression tests are added to test/js/bun/util/inspect.test.js.

Security risks

None. This is strictly defensive hardening of an inspection/formatting code path. No new inputs are accepted, no auth/crypto/permissions are touched, and the change only adds exception-clearing and an empty-value guard — it cannot expose data or enable injection.

Level of scrutiny

Low-to-medium. While bindings.cpp is core JSC interop, the diff is ~10 lines and purely additive: the happy path (no exception, non-empty prototype) is byte-for-byte equivalent to the old behavior. The only semantic change is that previously-crashing inputs now gracefully skip the property / stop walking the chain. The pattern (CLEAR_IF_EXCEPTION + empty-JSValue guard before .getObject()) matches existing idioms in this file.

Other factors

• The CodeRabbit nit (loose toContain("ok") assertion) was addressed; tests now assert ok: 1 and ok2: 2 explicitly.
• Since my prior review the only commits are the assertion tightening and a CI retrigger — no substantive code changes.
• My earlier inline note about the identical pattern in napi.cpp is explicitly pre-existing and tracked by other PRs (#29642); it does not block this fix.
• The duplicate-PR bot flagged several overlapping PRs; that is a merge-coordination question for maintainers, not a correctness concern with this diff.
• No bugs found by the bug-hunting system on this run.