Fix null deref in forEachProperty when Proxy prototype getter throws - #30541
Fix null deref in forEachProperty when Proxy prototype getter throws#30541robobun wants to merge 1 commit into
Conversation
When inspecting an object whose prototype chain contains a Proxy, the slow path in forEachPropertyImpl uses InternalMethodType::Get which invokes getters through the Proxy. If a getter throws, getPropertySlot returns false but the CLEAR_IF_EXCEPTION was placed after the early continue, so the exception stayed pending. On the next prototype hop, ProxyObject::getPrototype would observe the pending exception and return an empty JSValue, and .getObject() on an empty value dereferenced a null cell. Clear the exception before the continue, and also guard the getPrototype result so a throwing getPrototypeOf trap cannot crash the formatter either.
|
I am sure we already fixed this? @robobun ? |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughException handling in JSC property enumeration is refactored to capture property-slot lookup and prototype-chain access results into temporary variables, clear exceptions immediately after each operation, and then decide iteration flow. A new test suite validates that Bun.inspect handles Proxy traps that throw without crashing. ChangesProxy exception handling in property enumeration
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
You're right — this is a dupe. The bug is still present on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward exception-handling reorder and null guard, matching the existing fast-path pattern.
Extended reasoning...
Overview
This PR fixes a null-pointer deref in JSC__JSValue__forEachPropertyImpl (bindings.cpp) when Bun.inspect / console.log formats an object with a Proxy in its prototype chain and a getter or getPrototypeOf trap throws. Two changes:
- Reorder
CLEAR_IF_EXCEPTION(scope)to run before thecontinueaftergetPropertySlot, so a throwing Proxygettrap doesn't leave a pending exception that poisons the subsequentgetPrototypecall. - Guard the result of
iterating->getPrototype(globalObject)and clear any exception from a throwinggetPrototypeOftrap before calling.getObject()on it.
Both changes mirror what the fast path in the same function already does at lines ~5336–5337, so this is bringing the slow path to parity rather than introducing new behavior.
Security risks
None. This is purely defensive — replacing a crash with graceful exception swallowing in a debug/inspection code path. No new attack surface, no auth/crypto/permissions involved.
Level of scrutiny
Low-to-moderate. While bindings.cpp is core JSC integration code, the diff is ~10 lines and mechanical: a statement reorder and a null check. The CLEAR_IF_EXCEPTION + proto ? proto.getObject() : nullptr pattern is idiomatic and already used elsewhere in this function. The loop correctly terminates when iterating becomes nullptr (the while condition checks it).
Other factors
- Fuzzer-found crash with a clear repro and root-cause analysis in the PR description.
- Three regression tests added; tests 1 and 2 directly exercise the two fixed code paths. The inline nit notes test 3's threshold is too high to actually trigger the throw, but that's supplementary coverage — the fix itself is well-tested by tests 1 and 2.
- No CODEOWNERS for this path.
- No outstanding reviewer comments.
| get(t, k, r) { | ||
| if (++count > 3) throw new Error("get fail"); | ||
| return Reflect.get(t, k, r); |
There was a problem hiding this comment.
🟡 This test never actually triggers the throw: the get trap is invoked exactly 3 times during Bun.inspect(obj) (once for Symbol(nodejs.util.inspect.custom), then once each for bar and baz), so ++count > 3 is never true and the test passes even without the bindings.cpp fix. Lower the threshold to e.g. > 1 so the trap actually throws inside getPropertySlot and exercises the CLEAR_IF_EXCEPTION-before-continue path. (Nit — tests 1 and 2 already cover the actual fixes, this one is just currently vacuous.)
Extended reasoning...
What the test claims vs. what it does
The third test is titled "does not crash when a Proxy get trap throws during iteration" and asserts expect(() => Bun.inspect(obj)).not.toThrow(). For that to be a meaningful regression test, the get trap must actually throw at some point during forEachPropertyImpl's slow-path iteration so that the new CLEAR_IF_EXCEPTION(scope) before continue (bindings.cpp:5372–5376) is exercised. As written, the trap never throws.
Step-by-step trace of the get-trap invocations
Running this scenario on bun and counting trap invocations gives exactly 3:
Symbol(nodejs.util.inspect.custom)lookup — BeforeforEachPropertyruns,Tag.getAdvanced(ConsoleObject.zig:1246) callsfastGet(.inspectCustom), which walksobj → proxyand invokesProxyObject::getOwnPropertySlotwithInternalMethodType::Get, calling the trap once.count = 1,1 > 3is false.bar— In the slow path,getOwnPropertyNameson the proxy (noownKeystrap) returns[constructor, bar, baz];constructoris filtered at bindings.cpp:5362. Forbar,object->getPropertySlot(globalObject, property, slot)walksobj → proxyand invokesperformGet, calling the trap.count = 2,2 > 3is false. The slot is a plain value slot, soslot.getValue()later returns the cached value with no second trap call.baz— Same as above.count = 3,3 > 3is false.proxy->getPrototype()(nogetPrototypeOftrap) returnsObject.prototype, the loop terminates.
The condition if (++count > 3) throw first fires on the 4th invocation, which never happens. No exception is ever raised inside forEachPropertyImpl, so neither the new CLEAR_IF_EXCEPTION at 5374 nor the getPrototype guard at 5448–5451 is reached via this test.
Why this matters
This was verified empirically on a bun build without this PR's fix: the test body runs to completion with count == 3, no throw, no crash — so the test would pass even on the broken code it's meant to guard against. It currently provides no regression coverage for the throwing-get-trap path described in its title.
Why this is only a nit
The first test (the expect(1) + Proxy-prototype repro from the fuzzer) does exercise the throwing-getter path that crashed pre-fix and covers bindings.cpp:5372–5376, and the second test covers the getPrototype guard at 5448–5451. So overall regression coverage for the fix exists; this third test is supplementary and just ineffective as written.
Fix
Lower the threshold so the trap throws during the getPropertySlot loop:
get(t, k, r) {
if (++count > 1) throw new Error("get fail");
return Reflect.get(t, k, r);
},With > 1, the inspectCustom lookup succeeds (count=1), then bar throws inside getPropertySlot (count=2) — exercising exactly the code path this PR fixed. > 2 would also work (throws on baz).
2 participants
Fuzzer fingerprint:
b8b8543bb6fc12e8What
Bun.inspect/console.logcrashed with a null-pointer deref when formatting an object whose prototype chain contains a Proxy and a getter reached through that Proxy throws.Minimal repro:
Why
The slow path of
JSC__JSValue__forEachPropertyImplwalks the prototype chain usingPropertySlot::InternalMethodType::Get, which means going through a Proxy actually invokes getters. TheExpectprototype has stateful getters (.rejectsthen.resolves) where the second one throws.When
getPropertySlotthrows it returnsfalse, but the existing code did:The
continueruns before the exception is cleared, so it stays pending for the rest of the loop. At the end of the loop body we doiterating->getPrototype(globalObject).getObject(); for a Proxy,getPrototypesees the pending exception and returns an emptyJSValue, and.getObject()on an empty value dereferences a nullJSCell*.Fix
getPropertySlotbefore thecontinue.getPrototypeand clear any exception from a throwinggetPrototypeOftrap (matching what the fast path already does).Added regression tests covering both a throwing getter and a throwing
getPrototypeOftrap.