Skip to content

Fix crash in Bun.inspect when Proxy prototype trap throws - #29965

Closed
robobun wants to merge 1 commit into
mainfrom
farm/3bbc9a77/fix-foreach-property-proxy-proto
Closed

Fix crash in Bun.inspect when Proxy prototype trap throws#29965
robobun wants to merge 1 commit into
mainfrom
farm/3bbc9a77/fix-foreach-property-proxy-proto

Conversation

@robobun

@robobun robobun commented Apr 30, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a null pointer dereference in JSC__JSValue__forEachPropertyImpl that crashed the process when inspecting / console.log-ing an object whose prototype chain contains a Proxy with a throwing trap.

Repro

class Foo {
  get bar() { throw new Error("bar throws"); }
}
const obj = new Foo();
Object.setPrototypeOf(obj, new Proxy(Object.getPrototypeOf(obj), {}));
console.log(obj); // crashes

or

const obj = { x: 1 };
Object.setPrototypeOf(obj, new Proxy({ y: 2 }, {
  getPrototypeOf() { throw new Error("nope"); },
}));
console.log(obj); // crashes

Root cause

Two related issues in the slow-path prototype walk of forEachPropertyImpl:

  1. When getPropertySlot with InternalMethodType::Get goes through a Proxy and a getter on the target throws, getPropertySlot returns false. The code then did continue before the CLEAR_IF_EXCEPTION call, leaking the exception into subsequent iterations.
  2. iterating = iterating->getPrototype(globalObject).getObject() calls .getObject() unconditionally on the result. If getPrototype throws (either directly via a Proxy getPrototypeOf trap, or because an exception was already pending from (1)), it returns an empty JSValue, and .getObject() on an empty JSValue calls a method on a null JSCell*.

The fast path directly above already handled both of these cases; this brings the slow path to parity.

How did you verify your code works?

  • Original fuzzer repro no longer crashes.
  • Added regression cases to the "crash testing" fixture list in test/js/bun/util/inspect.test.js.
  • bun bd test test/js/bun/util/inspect.test.js — 75 pass, 0 fail.

Found by Fuzzilli (fingerprint 3b2a3201fb5f71c3).

@robobun
Fix crash in forEachProperty when Proxy prototype trap throws
742a449 
When inspecting an object whose prototype chain contains a Proxy,
exceptions from the Proxy's get trap could leak past the
getPropertySlot() check (because the exception was only cleared when
getPropertySlot returned true). Additionally, if a Proxy's
getPrototypeOf trap threw, getPrototype() returned an empty JSValue
and calling .getObject() on it dereferenced a null JSCell.

Both paths now clear the pending exception and handle the empty
return, matching the existing handling in the fast path above.
@robobun

robobun commented Apr 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author
Updated 8:22 PM PT - Apr 29th, 2026

❌ @robobun, your commit 742a449 has 1 failures in Build #49312 (All Failures):

🧪   To try this PR locally:

bunx bun-pr 29965

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

bun-29965 --bun

@coderabbitai

coderabbitai Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b1d54ddf-875f-40e8-bd12-0fcdaa97b083

📥 Commits

Reviewing files that changed from the base of the PR and between 360bbb5 and 742a449.

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

Walkthrough

The changes refactor property enumeration and prototype-chain traversal logic to explicitly handle property resolution state and clear exceptions from proxy traps, with corresponding test additions covering edge cases in prototype access and property lookup.

Changes

Cohort / File(s) Summary
Property enumeration and prototype traversal
src/bun.js/bindings/bindings.cpp		 Refactored property enumeration loop to explicitly capture whether getPropertySlot found a property via a boolean flag. Prototype-chain traversal now fetches prototype as JSValue first, clears exceptions from proxy getPrototypeOf trap, then updates the iterating reference. Exception clearing from proxy Get traps preserved.
Crash test fixtures
test/js/bun/util/inspect.test.js		 Extended crash-testing fixture list with new test cases targeting prototype and property lookup failure modes: throwing accessor on instance with proxied prototype, prototype proxy throwing on getPrototypeOf, and prototype proxy throwing on property get.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: fixing a crash in Bun.inspect when Proxy prototype traps throw exceptions.
Description check ✅ Passed The description comprehensively covers both required template sections: it clearly explains what the PR does and provides thorough verification details including fuzzer repro, test results, and root cause analysis.
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: 1/5 review remaining, refill in 44 minutes and 17 seconds.

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

@github-actions

github-actions Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Found 3 issues this PR may fix:

  1. Crash while using node:net #23911 - Crash in JSC__JSValue__forEachPropertyImpl when console.log triggers property enumeration on a TCPSocket and a getter throws — exactly the exception-leak pattern this PR fixes
  2. bun eslint.config.mjs | more panixs #17381 - Panic in JSC__JSValue__forEachPropertyImpl during console.log piped through | more — crash path goes through the same function with an unhandled exception during property iteration
  3. Defining Object.prototype["1"] setter causes Bun to panic during env access #20514 - Segfault caused by a throwing setter on Object.prototype during getPropertySlot in the property lookup chain — same class of bug where exceptions from traps are not properly cleared

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #23911
Fixes #17381
Fixes #20514

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. inspect: clear exceptions when walking Proxy prototype chain #29845 - Fixes both the same bugs in forEachPropertyImpl: reorders getPropertySlot/CLEAR_IF_EXCEPTION and adds null-check after getPrototype
  2. fix(inspect): don't crash when a Proxy in the prototype chain throws #29642 - Fixes both the same bugs in forEachPropertyImpl with identical core fix, plus additional napi.cpp changes
  3. Fix null deref in forEachProperty with Proxy in prototype chain #29816 - Fixes both the same bugs in forEachPropertyImpl: identical getPropertySlot reorder and getPrototype null-check
  4. inspect: handle throwing Proxy getPrototypeOf in forEachProperty #29814 - Fixes both the same bugs in forEachPropertyImpl: identical getPropertySlot reorder and getPrototype null-check

🤖 Generated with Claude Code

@robobun

robobun commented Apr 30, 2026

Copy link
Copy Markdown
Collaborator Author

Duplicate of #29642 (and #29814, #29816, #29845).

@robobun robobun closed this Apr 30, 2026
@robobun
robobun deleted the farm/3bbc9a77/fix-foreach-property-proxy-proto branch April 30, 2026 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

claude

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@robobun