Fix null deref in Bun.inspect when Proxy prototype traps throw#30030
Fix null deref in Bun.inspect when Proxy prototype traps throw#30030robobun wants to merge 1 commit into
Conversation
When an object's prototype chain contains a Proxy with a throwing getPrototypeOf trap, or a throwing get trap, forEachProperty would crash while walking the prototype chain. - getPropertySlot can throw via a Proxy get trap and return false; the continue would skip CLEAR_IF_EXCEPTION, leaving the exception pending for the next iteration. - getPrototype can throw via a Proxy getPrototypeOf trap and return an empty JSValue; calling .getObject() on an empty value dereferences a null JSCell.
|
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)
WalkthroughThe changes enhance exception handling in property enumeration during object inspection. The implementation now properly handles exceptions from Proxy traps (both Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 4/5 reviews remaining, refill in 12 minutes. 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}
There was a problem hiding this comment.
LGTM — straightforward crash fix that mirrors the existing exception-handling patterns in the same function, with regression tests.
Extended reasoning...
Overview
This PR fixes a null-pointer dereference in JSC__JSValue__forEachPropertyImpl (the slow-path property iterator used by Bun.inspect / console.log). Two small changes in src/bun.js/bindings/bindings.cpp:
- Reorder
getPropertySlotsoCLEAR_IF_EXCEPTION(scope)runs before the earlycontinue(a Proxygettrap can throw and returnfalse). - Split
iterating->getPrototype(globalObject).getObject()into two steps with an exception check in between, since a throwing ProxygetPrototypeOftrap returns an emptyJSValueand.getObject()on that dereferences null.
Two regression tests added to test/js/bun/util/inspect.test.js covering both traps.
Security risks
None introduced. This is defensive hardening that removes a segfault triggerable from JS via a hostile Proxy. No new attack surface; exceptions are swallowed (consistent with the rest of this best-effort inspection routine, which already clears getter exceptions at lines 5337-5340 and has a catch-all clear at 5376-5379).
Level of scrutiny
Low-to-moderate. It's C++ in JSC bindings, but the change is purely mechanical and copies idioms already used a few lines away in the same function:
- The
hasProperty = ...; CLEAR_IF_EXCEPTION; if (!hasProperty) continue;shape now matches lines 5218 and 5253 exactly. - The
if (scope.exception()) { tryClearException(); ... }shape matches lines 5337-5340.
There are no new control-flow paths beyond "stop walking the prototype chain on error," which is the obvious correct behavior for an inspection routine.
Other factors
- Regression tests included; PR description states they segfault on
mainand pass with the fix, and the fullinspect.test.jssuite passes. - The one inline finding is explicitly pre-existing (same pattern in
napi.cpp, not touched by this PR) and is a follow-up suggestion, not a blocker. - ~15 lines of C++ changed, no API or behavior changes beyond not crashing.
| JSValue proto = iterating->getPrototype(globalObject); | ||
| // Ignore exceptions from Proxy "getPrototypeOf" trap. | ||
| if (scope.exception()) [[unlikely]] { | ||
| (void)scope.tryClearException(); | ||
| break; | ||
| } | ||
| iterating = proto.getObject(); |
There was a problem hiding this comment.
🟣 Heads up (pre-existing, not introduced by this PR): the same getPrototype(globalObject).getObject() null-deref pattern fixed here also exists at src/bun.js/bindings/napi.cpp:1837 inside napi_get_all_property_names. A native addon calling that API with napi_key_include_prototypes + an enumerable/writable/configurable filter on an object whose prototype chain has a Proxy with a throwing getPrototypeOf (or getOwnPropertyDescriptor) trap will crash the same way. Might be worth applying the same guard there as a follow-up.
Extended reasoning...
What the bug is
This PR correctly fixes a null dereference in JSC__JSValue__forEachPropertyImpl where iterating->getPrototype(globalObject).getObject() crashes when a Proxy getPrototypeOf trap throws. The identical vulnerable pattern exists, untouched, at src/bun.js/bindings/napi.cpp:1837 inside napi_get_all_property_names:
while (!current_object->getOwnPropertyDescriptor(globalObject, key.toPropertyKey(globalObject), desc)) {
JSObject* proto = current_object->getPrototype(globalObject).getObject();
if (!proto) {
break;
}
current_object = proto;
}There is no exception check between getPrototype() and .getObject(), and none after getOwnPropertyDescriptor() either (lines 1829–1861 contain zero RETURN_IF_EXCEPTION / CLEAR_IF_EXCEPTION).
Why it crashes (same mechanism as this PR)
When a Proxy getPrototypeOf trap throws, JSObject::getPrototype() returns an empty JSValue (encoded bits = 0). On JSVALUE64 an empty value reports isCell() == true with asCell() == nullptr, so JSValue::getObject() evaluates isCell() && asCell()->isObject() and dereferences a null JSCell* in ->isObject(). The if (!proto) break; at line 1838 cannot help — the segfault happens inside .getObject() before proto is ever assigned.
Code path that triggers it
- A native addon calls
napi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_enumerable /* or writable/configurable */, ...). obj's prototype chain contains aProxywhose handler has a throwinggetPrototypeOftrap (or a throwinggetOwnPropertyDescriptortrap, which makes thewhilecondition returnfalsewith a pending exception and thengetPrototyperuns under that exception).allPropertyKeysat line 1818 succeeds (it usesownKeys, notgetPrototypeOf/getOwnPropertyDescriptor), soNAPI_RETURN_IF_EXCEPTIONat 1823 passes.- Because
key_filter & (enumerable|writable|configurable)is set, the inner loop runs. - For a key not present as an own property on
object,getOwnPropertyDescriptorreturnsfalseand the body executescurrent_object->getPrototype(globalObject). - The Proxy trap throws →
getPrototype()returns emptyJSValue→.getObject()dereferences null → segfault.
Step-by-step proof
// JS side passed to a native addon:
const target = {};
const obj = Object.create(new Proxy(target, {
getPrototypeOf() { throw new Error("nope"); },
ownKeys() { return ["x"]; }, // so allPropertyKeys yields a key
getOwnPropertyDescriptor() { return { value: 1, enumerable: true, configurable: true }; },
}));
// Native addon:
// napi_get_all_property_names(env, obj, napi_key_include_prototypes,
// napi_key_enumerable, napi_key_numbers_to_strings, &result);allPropertyKeyswalks own keys ofobj(none) then the proxy'sownKeys→["x"]. No throw, line 1823 passes.- Loop iteration for
"x":object(=obj) has no own"x"→getOwnPropertyDescriptorreturnsfalse→ enter loop body. current_objectisobj;obj->getPrototype(globalObject)returns the Proxy (ordinary [[GetPrototypeOf]] onobj, no throw).proto= Proxy,current_object= Proxy.- Next
whiletest:getOwnPropertyDescriptoron the Proxy for"x"returns the descriptor → loop exits. Fine so far. - But change the handler to
getOwnPropertyDescriptor() { throw new Error("gOPD"); }: now the secondwhiletest throws and returnsfalse, body runs again, andcurrent_object->getPrototype(globalObject)invokes the Proxy'sgetPrototypeOftrap → throws → emptyJSValue→.getObject()→ null deref. - Alternatively, with a deeper chain (
obj → plain → Proxy), the second body iteration callsgetPrototypedirectly on the Proxy and crashes without needing the gOPD trap.
Either way the vulnerable line is reached with a throwing trap and no guard.
Why existing code doesn't prevent it
The only exception check in this function is NAPI_RETURN_IF_EXCEPTION(env) at line 1823, which runs before the descriptor-filtering loop. Inside the loop (1829–1861) there are no exception checks at all, so neither the throwing getOwnPropertyDescriptor nor the throwing getPrototype is caught.
Impact
Segfault of the Bun process when a native addon enumerates properties (with prototype walk + descriptor filter) on user-controlled objects whose prototype chain contains a hostile/broken Proxy. Lower exposure than the Bun.inspect case since it requires a native addon using this specific N-API call, but it's the same crash class this PR is hardening against.
Suggested fix (follow-up, not blocking)
Mirror this PR's fix in napi.cpp: after getOwnPropertyDescriptor and after getPrototype, check scope.exception() (or split out JSValue proto = current_object->getPrototype(globalObject); if (scope.exception() || !proto.isObject()) break;). Returning napi_pending_exception via NAPI_RETURN_IF_EXCEPTION inside the loop would also be reasonable.
This is pre-existing — napi.cpp is not touched by this PR and no new callers are added. Mentioning only because the PR is specifically about hardening this exact pattern.
1 participant
What does this PR do?
Fixes a crash in
Bun.inspect/console.logwhen formatting an object whose prototype chain contains a Proxy with throwing traps.In
JSC__JSValue__forEachPropertyImpl, the slow-path prototype walk had two issues:getPropertySlotcan throw via a Proxygettrap and returnfalse. The earlycontinueskippedCLEAR_IF_EXCEPTION, leaving the exception pending for subsequent operations.getPrototype(globalObject)can throw via a ProxygetPrototypeOftrap and return an emptyJSValue. Calling.getObject()on an empty value dereferences a nullJSCell.Both now clear the exception and stop walking the chain gracefully.
Repro
How did you verify your code works?
test/js/bun/util/inspect.test.jscovering both throwinggetPrototypeOfand throwinggettraps in the prototype chain.mainand pass with this fix.inspect.test.jssuite passes (74 tests).Found by fuzzer (fingerprint
8d8ced8f16c8c4dd).