Skip to content

Fix crash in Bun.inspect when Proxy getPrototypeOf trap throws#30240

Closed
robobun wants to merge 1 commit into
mainfrom
farm/8928bc04/fix-inspect-proxy-getprototypeof-throw
Closed

Fix crash in Bun.inspect when Proxy getPrototypeOf trap throws#30240
robobun wants to merge 1 commit into
mainfrom
farm/8928bc04/fix-inspect-proxy-getprototypeof-throw

Conversation

@robobun

@robobun robobun commented May 4, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a null pointer dereference crash found by Fuzzilli in Bun.inspect / console.log when printing an object whose prototype chain contains a Proxy with a throwing getPrototypeOf trap.

const p = new Proxy({}, { getPrototypeOf() { throw 0; } });
const obj = Object.create(p);
obj[0] = 1;
console.log(obj); // crash

When walking the prototype chain in the slow path of forEachProperty, JSObject::getPrototype() invokes the Proxy getPrototypeOf trap. If the trap throws, getPrototype() returns an empty JSValue, and calling .getObject() on it dereferences a null cell pointer.

The fix checks for an empty return value before calling .getObject() and clears the exception, matching the existing handling in the fast-path branch of the same function.

Also applies the same fix to napi_get_all_property_names which has the identical pattern.

How did you verify your code works?

Added regression tests to test/js/bun/util/inspect.test.js that crash before this change and pass after.

When walking the prototype chain during property enumeration,
JSObject::getPrototype() can invoke a Proxy getPrototypeOf trap which
may throw. In that case it returns an empty JSValue, and calling
.getObject() on it dereferences a null cell pointer.

Check for the empty value before calling getObject(), matching the
existing handling in the fast-path branch of the same function. Apply
the same fix to napi_get_all_property_names which has the identical
pattern.
@coderabbitai

coderabbitai Bot commented May 4, 2026

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@robobun has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes 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: c5ea89a7-9441-4970-a77c-47a63763266b

📥 Commits

Reviewing files that changed from the base of the PR and between 0a7bed5 and cd520b3.

📒 Files selected for processing (3)
  • src/jsc/bindings/bindings.cpp
  • src/jsc/bindings/napi.cpp
  • test/js/bun/util/inspect.test.js

Review rate limit: 0/5 reviews remaining, refill in 2 minutes.

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

@github-actions github-actions Bot added the claude label May 4, 2026
@robobun

robobun commented May 4, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 7:33 AM PT - May 4th, 2026

@robobun, your commit cd520b3 has 1 failures in Build #51296 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 30240

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

bun-30240 --bun

@github-actions

github-actions Bot commented May 4, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Fix null deref in Bun.inspect when Proxy prototype throws #30200 - Fixes the same forEachPropertyImpl crash in bindings.cpp when Proxy getPrototypeOf throws during prototype chain walk in Bun.inspect
  2. fix(inspect): handle Proxy trap exceptions when walking prototype chain #30099 - Fixes the same forEachPropertyImpl crash from Proxy getPrototypeOf throwing during Bun.inspect
  3. inspect: handle throwing Proxy getPrototypeOf in forEachProperty #29814 - Fixes the exact same forEachPropertyImpl getPrototype() crash for Proxy with throwing getPrototypeOf
  4. inspect: clear exceptions when walking Proxy prototype chain #29845 - Fixes the same crash by clearing exceptions after getPrototype() on Proxy objects in forEachPropertyImpl
  5. fix(inspect): don't crash when a Proxy in the prototype chain throws #29642 - Fixes the same crash in both bindings.cpp and napi.cpp — identical file scope to this PR
  6. Fix null JSCell deref in Bun lazy property callbacks and forEachProperty #29671 - Partially overlapping — fixes the same forEachPropertyImpl getPrototype() crash, plus an unrelated lazy property issue
  7. getPrototype exception checks #24985 - Broader getPrototype exception checks covering the same crash in bindings.cpp and napi.cpp

🤖 Generated with Claude Code

@robobun

robobun commented May 4, 2026

Copy link
Copy Markdown
Collaborator Author

Duplicate of #29642 / #24985.

@robobun robobun closed this May 4, 2026
@robobun robobun deleted the farm/8928bc04/fix-inspect-proxy-getprototypeof-throw branch May 4, 2026 14:31

@claude claude Bot left a comment

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.

LGTM — minimal null-check + exception handling for throwing Proxy getPrototypeOf traps, with regression tests.

Extended reasoning...

Overview

This PR fixes a Fuzzilli-discovered null-pointer-dereference crash in two prototype-chain walks: JSC__JSValue__forEachPropertyImpl in bindings.cpp (the slow path used by Bun.inspect/console.log) and napi_get_all_property_names in napi.cpp. In both, JSObject::getPrototype() can invoke a Proxy getPrototypeOf trap; if the trap throws, an empty JSValue is returned and the previous code unconditionally called .getObject() on it. The fix stores the result, handles the pending exception (CLEAR_IF_EXCEPTION for inspect, NAPI_RETURN_IF_EXCEPTION for N-API), and null-checks before dereferencing. Two regression cases are added to the existing "crash testing" fixture in inspect.test.js.

Security risks

None. This is purely defensive error handling — it converts a hard crash into graceful termination of the prototype walk (inspect) or a propagated pending-exception status (N-API). No new user-controlled data flows, auth, or privilege boundaries are touched.

Level of scrutiny

Low. The change is ~6 lines of C++ across two sites, each a localized null/exception guard. It mirrors the existing fast-path handling in the same function (bindings.cpp:5250, 5327), which already uses if (JSValue proto = ...) to guard against empty returns, and uses the standard NAPI_RETURN_IF_EXCEPTION macro idiom in napi.cpp. The behavior choice (swallow the exception in inspect, propagate in N-API) is consistent with how each function already treats exceptions elsewhere in its body.

Other factors

  • No CODEOWNERS apply to the modified files.
  • The bug-hunting system found no issues.
  • Regression tests are added to the existing crash-test fixture loop, so they're verified not to crash rather than asserting specific output — appropriate for a Fuzzilli fix.
  • No prior human or bot reviews to address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant