Fix null deref in forEachProperty with Proxy in prototype chain - #29816
Fix null deref in forEachProperty with Proxy in prototype chain#29816robobun wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughUpdates exception handling in property enumeration logic to record property slot discovery and clear proxy trap exceptions before continuing traversal. Adds test cases to verify that throwing getters and proxy getPrototypeOf traps don't cause crashes during inspection. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
CI failures are unrelated pre-existing flakes that appear on every recent PR build: Build 48472 / 48777:
None of these touch the For comparison, the same shard pattern fails on other concurrent PRs (48661, 48653, 48647, 48644, 48643, 48640, 48635, 48630, ...) — it's the current baseline. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped crash fix that mirrors the existing exception-clearing patterns in forEachPropertyOrdered and the fast-path branch just above.
Extended reasoning...
Overview
This PR fixes a null pointer dereference in JSC__JSValue__forEachPropertyImpl (bindings.cpp), the C++ backing for Bun.inspect / console.log property enumeration. Two tightly-scoped changes: (1) reorder CLEAR_IF_EXCEPTION to run before the continue when getPropertySlot fails, and (2) null-check the result of getPrototype() before calling .getObject() on it, clearing any exception from a Proxy getPrototypeOf trap. Two regression fixtures are added to the existing crash-test loop in inspect.test.js.
Security risks
None. This is output-formatting code (Bun.inspect). The change adds defensive null/exception handling and does not introduce new data flow, parsing, or trust boundaries. Clearing exceptions here is intentional and matches surrounding behavior — inspect is designed to swallow getter/trap errors rather than propagate them.
Level of scrutiny
Low-to-moderate. The diff is ~7 lines of logic in a single function. Both changes are direct copies of patterns already present in the same file: the hasSlot reordering matches forEachPropertyOrdered at lines 5456-5461, and the nextProto ? nextProto.getObject() : nullptr + CLEAR_IF_EXCEPTION matches the fast-path prototype walk at lines 5243-5253. There is no novel design here, just bringing the slow path into parity with sibling code.
Other factors
- Bug hunter found no issues.
- Root-cause analysis in the PR description is precise and the Fuzzilli crash signature confirms the failure mode.
- Regression tests cover both the throwing-getter-via-Proxy and throwing-
getPrototypeOf-trap cases; CI confirms the inspect test shard passed on all platforms (remaining failures are documented pre-existing flakes unrelated to this code path). - No outstanding reviewer comments.
When inspecting an object whose prototype chain contains a Proxy, getPropertySlot() may throw (e.g. a throwing getter invoked through the Proxy's default [[Get]]). If it returned false, we would continue to the next property without clearing the pending exception, and a later getPrototype() call on the Proxy would bail out early and return an empty JSValue, on which .getObject() dereferences a null JSCell. Clear the exception before skipping the property (matching forEachPropertyOrdered), and guard the prototype walk against an empty return from getPrototype() so a throwing getPrototypeOf trap cannot crash either.
83239a1 to
278af2f
Compare
There was a problem hiding this comment.
LGTM — targeted crash fix that mirrors the existing fast-path exception handling in the same function.
Extended reasoning...
Overview
This PR touches two small hunks in src/jsc/bindings/bindings.cpp inside JSC__JSValue__forEachPropertyImpl (the property iterator backing Bun.inspect / console.log), plus two new crash-test fixtures appended to the existing fixture array in test/js/bun/util/inspect.test.js.
The first hunk reorders the getPropertySlot call so that CLEAR_IF_EXCEPTION(scope) runs before the continue, instead of being skipped when the slot lookup fails. The second hunk stores the result of iterating->getPrototype(globalObject) in a local, clears any pending exception from a Proxy getPrototypeOf trap, and null-checks before calling .getObject(). The enclosing while (iterating && ...) loop already handles nullptr as a clean termination condition.
Security risks
None. This is purely defensive exception/null handling in the object-inspection formatter. The change makes the code strictly more robust — it swallows exceptions that were already intended to be swallowed (the CLEAR_IF_EXCEPTION was already present, just unreachable on the early-continue path) and guards a null deref. No new user-controlled data flows, auth, crypto, or permissions are involved.
Level of scrutiny
Low-to-medium. While bindings.cpp is core C++ glue, this specific change is a mechanical defensive fix that exactly mirrors the pattern already used in the fast-path branch of the same function (lines ~5327–5337, which already does if (JSValue proto = ...getPrototype(...)) followed by CLEAR_IF_EXCEPTION). The PR description's root-cause analysis is precise and verifiable against the JSC semantics of getPropertySlot with InternalMethodType::Get on a Proxy.
Other factors
- The bug hunting system found no issues.
- Regression tests are added as crash-test fixtures and CI confirms the
inspect.test.jsshard passes on all platforms; remaining CI failures are documented pre-existing flakes unrelated to this code path. - The diff is ~10 lines of logic with no behavioral change beyond preventing the crash — successful paths are unaffected.
- The duplicate-PR bot flagged three other PRs fixing the same root cause; that's a coordination question for maintainers but doesn't affect the correctness of this change.
1 participant
What does this PR do?
Fixes a null pointer dereference in
JSC__JSValue__forEachPropertyImpl(used byBun.inspect/console.log) when the object being inspected has aProxyin its prototype chain.Repro
Root cause
Proxysits in the prototype chain,getPropertySlot()withInternalMethodType::Getinvokes the proxy's[[Get]], which (with the default trap) evaluates getters on the target. If a getter throws,getPropertySlot()returnsfalsewith the exception still pending. The code didif (!getPropertySlot(...)) continue;, skipping theCLEAR_IF_EXCEPTIONthat follows, so the exception stayed pending into the next prototype-walk step.iterating->getPrototype(globalObject)on aProxyObjectthen bails out early due to the pending exception (or because its owngetPrototypeOftrap throws) and returns an emptyJSValue. Calling.getObject()on an emptyJSValuepassesisCell()and dereferences a nullJSCell.Fix
continue, matching whatforEachPropertyOrderedalready does.getPrototype()and clear any exception from thegetPrototypeOftrap, matching the fast-path branch just above.How did you verify your code works?
Added inspect crash-test fixtures covering both a throwing getter behind a
Proxyprototype and a throwinggetPrototypeOftrap.Found by Fuzzilli (
JSCJSValueCell.h:92:33: member call on null pointer of type 'JSC::JSCell').