Fix null deref in Bun.inspect when Proxy getPrototypeOf trap throws#29943
Fix null deref in Bun.inspect when Proxy getPrototypeOf trap throws#29943robobun wants to merge 1 commit into
Conversation
When walking the prototype chain in JSC__JSValue__forEachPropertyImpl, getPrototype() can throw (e.g. via a Proxy getPrototypeOf trap) and return an empty JSValue. Calling .getObject() on that empty value dereferences a null pointer. Handle the empty value and clear the exception, matching the existing handling in the fast-path branch above.
|
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 modify prototype-chain traversal in 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: 3/5 reviews remaining, refill in 15 minutes and 48 seconds. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
robobun
deleted the
farm/60be592e/fix-foreachproperty-proxy-getprototype
branch
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward null-check that mirrors the existing fast-path handling in the same function, with a regression test.
Extended reasoning...
Overview
This PR fixes a null pointer dereference in JSC__JSValue__forEachPropertyImpl (bindings.cpp) when getPrototype() throws via a Proxy getPrototypeOf trap and returns an empty JSValue. The fix replaces a chained .getObject() call with an empty-value check and a CLEAR_IF_EXCEPTION, then adds a regression test in inspect.test.js. Total change is 4 lines of C++ and one test case.
Security risks
None. This is a defensive fix that prevents a crash in the property-enumeration path used by Bun.inspect / console.log. No new attack surface; it strictly hardens against hostile Proxy traps.
Level of scrutiny
Low. The change is mechanical and directly mirrors two existing patterns in the same function (lines ~5166 and ~5243) that already guard getPrototype() with if (JSValue proto = ...), and the CLEAR_IF_EXCEPTION matches the surrounding exception-swallowing semantics of this best-effort inspection routine (e.g. lines 5291, 5336–5338, 5372–5374). Setting iterating = nullptr cleanly terminates the enclosing for (; iterating && ...) loop.
Other factors
The only finding from the bug-hunting pass is a pre-existing instance of the same pattern in napi.cpp — explicitly flagged as out-of-scope follow-up, not a defect in this PR. The fix was found via Fuzzilli, has a clear repro, and ships with a test that exercises the exact crash path. No outstanding reviewer comments.
| JSValue proto = iterating->getPrototype(globalObject); | ||
| // Ignore exceptions from Proxy "getPrototypeOf" trap. | ||
| CLEAR_IF_EXCEPTION(scope); | ||
| iterating = proto ? proto.getObject() : nullptr; |
There was a problem hiding this comment.
🟣 Heads up (pre-existing, not introduced by this PR): the identical getPrototype(globalObject).getObject() pattern exists at src/bun.js/bindings/napi.cpp:1837 in the napi_key_include_prototypes filtering loop of napi_get_all_property_names. A Proxy whose getPrototypeOf/getOwnPropertyDescriptor trap throws during that per-key prototype re-walk will null-deref the same way — might be worth applying the same fix there in a follow-up.
Extended reasoning...
Summary
This PR correctly fixes a null deref in JSC__JSValue__forEachPropertyImpl where iterating->getPrototype(globalObject).getObject() crashes when getPrototype throws (Proxy getPrototypeOf trap). The same pattern exists, unfixed, at src/bun.js/bindings/napi.cpp:1837 inside napi_get_all_property_names. This is a pre-existing issue — the PR does not touch napi.cpp — but it's the exact bug class being addressed and is worth flagging for a follow-up.
The code path
In napi_get_all_property_names, when the caller passes napi_key_include_prototypes together with a descriptor-based filter (napi_key_writable / napi_key_enumerable / napi_key_configurable), the code first collects all keys via allPropertyKeys() and then, for each collected key, re-walks the prototype chain to find the owning descriptor:
JSObject* current_object = object;
while (!current_object->getOwnPropertyDescriptor(globalObject, key.toPropertyKey(globalObject), desc)) {
JSObject* proto = current_object->getPrototype(globalObject).getObject(); // ← line 1837
if (!proto) {
break;
}
current_object = proto;
}If getPrototype throws, it returns an empty JSValue. On JSVALUE64 an empty value is encoded as 0, so isCell() returns true, asCell() returns nullptr, and JSCell::getObject() dereferences m_type on a null pointer — exactly the crash this PR fixes in bindings.cpp. The if (!proto) break; on the next line never runs because the deref happens inside .getObject().
Why the existing guard doesn't help
There is a NAPI_RETURN_IF_EXCEPTION at line 1823 after allPropertyKeys(), but it only guards the initial property collection. The per-key filtering loop that follows performs its own prototype walk with no exception checks between getOwnPropertyDescriptor and getPrototype, nor after getPrototype itself. Two routes reach the crash:
- Throwing
getOwnPropertyDescriptortrap — if a Proxy in the chain throws from itsgetOwnPropertyDescriptortrap, the call returnsfalsewith a pending exception, thewhilebody executes, andgetPrototypeimmediately returns empty because of the pending exception. - Stateful
getPrototypeOftrap — a trap that succeeds duringallPropertyKeys()(so the line-1823 guard is passed) but throws on a later invocation during the per-key re-walk returns empty directly.
Step-by-step proof
- Native addon calls
napi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_enumerable, napi_key_numbers_to_strings, &result)whereobj's prototype chain containsnew Proxy(target, { getOwnPropertyDescriptor() { throw new Error('boom'); } }). allPropertyKeys()collects keys (usingownKeys, notgetOwnPropertyDescriptor), no exception is pending → passes the line-1823 check.- Filtering loop iterates the first key;
current_object->getOwnPropertyDescriptor(...)walks down to the Proxy, invokes the trap, which throws → returnsfalsewith a pending exception. - Loop body runs:
current_object->getPrototype(globalObject)sees the pending exception and returns an emptyJSValue(encoded0). .getObject()→isCell()is!isNotCellMask(0)= true →asCell()=nullptr→nullptr->m_type→ SIGSEGV.
Impact and suggested fix
Any native addon that enumerates prototype-chain properties with a descriptor filter on user-controlled objects can be crashed by a hostile Proxy. The fix is the same as in this PR:
JSValue protoVal = current_object->getPrototype(globalObject);
if (scope.exception()) [[unlikely]] { ... } // or CLEAR_IF_EXCEPTION + break
JSObject* proto = protoVal ? protoVal.getObject() : nullptr;
if (!proto) break;(and ideally an exception check after getOwnPropertyDescriptor as well). Since this PR doesn't otherwise touch N-API, it's reasonable to land this fix as-is and address napi.cpp in a follow-up.
1 participant
What
Fixes a null pointer dereference in
JSC__JSValue__forEachPropertyImpl(used byBun.inspect,console.log, and expect matcher error formatting) when walking the prototype chain of an object whose prototype is aProxyand[[GetPrototypeOf]]throws.Why
JSObject::getPrototype()can throw — either directly via a ProxygetPrototypeOftrap, or indirectly when there's a pending exception from a prior property access through the Proxy. When it throws, it returns an emptyJSValue, and calling.getObject()on that empty value dereferences a null cell:iterating = iterating->getPrototype(globalObject).getObject();The fast-path branch a few lines above already handles this correctly by checking the return value and clearing the exception; the slow-path loop did not.
Repro
Found by Fuzzilli (fingerprint
72a7516faba947a1).