Skip to content

Fix crash in forEachProperty when Proxy getPrototypeOf trap throws#29386

Closed
robobun wants to merge 1 commit into
mainfrom
farm/8b93594e/fix-foreach-property-proto-throw
Closed

Fix crash in forEachProperty when Proxy getPrototypeOf trap throws#29386
robobun wants to merge 1 commit into
mainfrom
farm/8b93594e/fix-foreach-property-proto-throw

Conversation

@robobun

@robobun robobun commented Apr 17, 2026

Copy link
Copy Markdown
Collaborator

When walking the prototype chain during property enumeration for the console formatter (Bun.inspect / console.log), getPrototype() can throw — e.g. via a Proxy getPrototypeOf trap — returning an empty JSValue. Calling .getObject() on that empty value dereferences a null cell pointer and crashes.

Clear the exception and stop walking the chain instead, matching the behavior of the other getPrototype() call sites in the same function.

Repro

const obj = {};
Object.setPrototypeOf(obj, new Proxy({}, {
  getPrototypeOf() { throw new Error("nope"); }
}));
console.log(obj);

Before: Segmentation fault at address 0x5
After: {}

Fuzzer fingerprint: 6ba473c2ca8e03e1

@robobun
Fix crash in forEachProperty when Proxy getPrototypeOf trap throws
f647c8e 
When walking the prototype chain during property enumeration for the
console formatter, getPrototype() can throw (e.g. via a Proxy trap),
returning an empty JSValue. Calling .getObject() on that empty value
dereferences a null cell pointer.

Clear the exception and stop walking the chain instead, matching the
behavior of the other getPrototype() call sites in this function.
@robobun

robobun commented Apr 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author
Updated 10:48 PM PT - Apr 16th, 2026

❌ @robobun, your commit f647c8e has 4 failures in Build #45974 (All Failures):

🧪   To try this PR locally:

bunx bun-pr 29386

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

bun-29386 --bun

@coderabbitai

coderabbitai Bot commented Apr 17, 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: b331c5b7-1446-4347-badc-b38902f3d725

📥 Commits

Reviewing files that changed from the base of the PR and between b2d3f5d and f647c8e.

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

Walkthrough

Updated prototype-chain iteration in JSC value inspection to handle exceptions from Proxy getPrototypeOf traps, and added a test verifying the functionality doesn't crash under these conditions.

Changes

Cohort / File(s) Summary
Prototype Chain Exception Handling
src/bun.js/bindings/bindings.cpp		 Updated JSC__JSValue__forEachPropertyImpl to capture and validate prototype values before advancing iteration, preventing crashes when Proxy getPrototypeOf traps throw exceptions.
Exception Handling Test Coverage
test/js/bun/util/inspect.test.js		 Added test case verifying Bun.inspect() handles objects with Proxy prototypes that throw in getPrototypeOf traps without crashing.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main fix: preventing a crash in forEachProperty when a Proxy getPrototypeOf trap throws an exception.
Description check ✅ Passed The description includes both required sections: a detailed explanation of what the PR does (the problem and fix) and verification via a code reproduction case that demonstrates the issue and expected behavior.

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

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

@github-actions

github-actions Bot commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Fix null deref in forEachProperty when prototype is a Proxy #28325 - Fixes same null deref in forEachProperty prototype walk when Proxy getPrototypeOf throws
  2. Fix crash in forEachProperty when prototype chain contains a Proxy #28854 - Fixes same crash in forEachProperty with CLEAR_IF_EXCEPTION and null check after getPrototype
  3. Fix null deref in forEachProperty when Proxy prototype throws #28882 - Fixes same null deref in forEachProperty prototype walk, also fixes napi.cpp
  4. Handle exception from getPrototype in forEachProperty #28918 - Handles getPrototype exception in both fast-path and slow-path of forEachPropertyImpl
  5. Fix use-after-poison in forEachProperty prototype chain walk #28919 - Fixes same use-after-poison in forEachProperty prototype chain walk with additional EnsureStillAliveScope
  6. fix null deref in forEachProperty prototype walk #28991 - Fixes same null deref in forEachProperty prototype walk
  7. Fix null deref in Bun.inspect when Proxy getPrototypeOf throws #29071 - Fixes same null deref in Bun.inspect Proxy getPrototypeOf, also fixes getPropertySlot and napi.cpp
  8. getPrototype exception checks #24985 - Broader fix that propagates getPrototype exceptions at the Zig binding level across all call sites

🤖 Generated with Claude Code

@robobun

robobun commented Apr 17, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as duplicate of #28325 (and several others: #28854, #28882, #28918, #28919, #28991, #29071, #24985) which all fix the same null deref in forEachProperty when a Proxy getPrototypeOf trap throws.

@robobun robobun closed this Apr 17, 2026
@robobun robobun deleted the farm/8b93594e/fix-foreach-property-proto-throw branch April 17, 2026 00:37
claude[bot]
claude Bot reviewed Apr 17, 2026
View reviewed changes

@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 — targeted crash fix with matching test.

Extended reasoning...

Overview

Fixes a segfault in JSC__JSValue__forEachPropertyImpl (bindings.cpp) that occurred when walking the prototype chain during property enumeration and a Proxy getPrototypeOf trap throws. The fix adds null-check and exception clearing before dereferencing the returned JSValue, plus a regression test in inspect.test.js.

Security Risks

None. The change only adds defensive handling for an exception path, preventing a crash. It does not affect auth, crypto, or permissions.

Level of Scrutiny

Low — this is a single-site, surgical crash fix with a clear repro. The CLEAR_IF_EXCEPTION pattern is already used in two other nearby call sites in the same function (lines ~5223 and ~5258), making this straightforwardly consistent.

Other Factors

No bugs were found by the automated bug hunter. The test directly reproduces the crash scenario. The change is 5 lines of C++ and 16 lines of test code.

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

Reviewers

@claude claude[bot] claude[bot] left review comments

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