inspect: handle throwing Proxy traps when walking prototype chain#29069
inspect: handle throwing Proxy traps when walking prototype chain#29069robobun wants to merge 1 commit into
Conversation
forEachPropertyImpl walks the prototype chain to enumerate properties. When the chain contains a Proxy, two failure modes were unhandled: - getPrototype() on a Proxy can throw (getPrototypeOf trap) and return an empty JSValue; calling .getObject() on that crashed. - getPropertySlot() going through a Proxy [[Get]] can throw and return false; the exception was only cleared on the true branch, so it remained pending and caused the next getPrototype() to return empty. Clear the exception in both places and stop walking when getPrototype() returns empty, matching the handling already present in the fast path.
|
Updated 10:28 PM PT - Apr 8th, 2026
❌ @robobun, your commit f74a274 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 29069That installs a local version of the PR into your bun-29069 --bun |
|
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)
WalkthroughModified the prototype chain traversal logic in Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| get foo() { | ||
| throw new Error("oops"); | ||
| }, | ||
| bar: 2, | ||
| }, | ||
| {}, | ||
| ), | ||
| ); | ||
| const result = Bun.inspect(o); | ||
| expect(result).toContain("bar: 2"); | ||
| }); |
There was a problem hiding this comment.
🟡 The test for "Proxy prototype with throwing getter on target" only checks that bar: 2 appears in the output, but never asserts that foo (the throwing getter) is absent. If a regression caused foo to appear as [Getter], foo: undefined, or any other representation, the test would silently pass. Adding expect(result).not.toContain("foo") would make the negative intent explicit and catch such regressions.
Extended reasoning...
What the bug is: The test at the bottom of the PR diff verifies the "Proxy prototype with throwing getter on target" scenario by checking expect(result).toContain("bar: 2"). This only checks the positive case — that the non-throwing property appears — but never validates the negative case: that the throwing getter (foo) does not appear in the output.
The specific code path: The C++ fix in bindings.cpp (lines 5292–5296 of the diff) deliberately moves CLEAR_IF_EXCEPTION before hasSlot is checked, so that when getPropertySlot returns false (because the getter threw), the exception is cleared and the property is skipped via continue. The intent is for foo to be absent from the output entirely.
Why existing code does not prevent it: The test exercises crash prevention (running without segfaulting) and confirms bar appears, but there is no assertion guarding the secondary behavior — that foo is skipped. A future change that accidentally emits foo: [Getter] or foo: undefined would still satisfy toContain("bar: 2").
Impact: A regression in property-skipping logic for throwing getters would go undetected by this test. The symptom would be a silent leak of a throwing-getter property into inspect output, which could expose implementation details or confuse users inspecting objects.
How to fix: Add expect(result).not.toContain("foo") immediately after the existing toContain assertion. This makes the test intent explicit and catches any regression where the throwing getter appears under any name or value.
Regarding the refutation: The refuter argues this is a subjective test quality concern rather than a bug. That is fair — the crash-prevention intent is fully covered, and the C++ fix is correct. However, the purpose of the dedicated named test (distinct from the crash-fixture suite) is precisely to assert behavior, not merely absence of crash. Without the negative assertion, the test leaves the specific behavioral invariant — that throwing getters are skipped — entirely unverified. This is a real gap in test coverage, even if minor.
Step-by-step proof: Suppose a regression changes the C++ so that when hasSlot is false the property is emitted as foo: undefined. Then result becomes something like { foo: undefined, bar: 2 }. expect(result).toContain("bar: 2") passes because the string contains bar: 2. The test turns green despite the broken behavior. Adding expect(result).not.toContain("foo") would catch this.
1 participant
Fuzzilli fingerprint:
3ece47c123fdcdb2What
Bun.inspect/console.logcrashed with a null pointer dereference when formatting an object whose prototype chain contains a Proxy that throws from itsgetPrototypeOftrap, or whose target has a throwing getter.Repro
Why
JSC__JSValue__forEachPropertyImplwalks the prototype chain on the slow path via:iterating = iterating->getPrototype(globalObject).getObject();When
iteratingis a Proxy,getPrototype()invokes thegetPrototypeOftrap, which can throw and return an emptyJSValue. Calling.getObject()on an emptyJSValuedereferences a null cell.Additionally, when
getPropertySlot()goes through a Proxy[[Get]]and the underlying getter throws, it returnsfalse— but the existingCLEAR_IF_EXCEPTIONwas placed after thecontinue, so the pending exception leaked into the nextgetPrototype()call and produced the same empty value.Fix
getPropertySlot()before checking its return value.getPrototype()and break out of the loop if it's empty, clearing any exception from the trap.This mirrors the handling already present in the fast path of the same function.