Fix null deref in forEachProperty when Proxy prototype throws#29668
Fix null deref in forEachProperty when Proxy prototype throws#29668robobun wants to merge 1 commit into
Conversation
When an object's prototype chain contains a Proxy, forEachProperty takes the slow path. Two cases could leave a pending exception or produce an empty JSValue that was then dereferenced: - getPropertySlot() with InternalMethodType::Get on a Proxy invokes the getter immediately. If it throws, getPropertySlot returns false and the early continue skipped over CLEAR_IF_EXCEPTION, leaving a pending exception for the rest of the loop. - getPrototype() on a Proxy can throw (getPrototypeOf trap, or when entered with a pending exception). It returns an empty JSValue, and calling .getObject() on that is a null deref.
|
Updated 8:50 PM PT - Apr 23rd, 2026
❌ @robobun, your commit 9229ff8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29668That installs a local version of the PR into your bun-29668 --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)
WalkthroughUpdates property and prototype chain traversal logic in JSC bindings to properly handle exception clearing and Proxy objects, accompanied by new test cases verifying Bun.inspect behavior with proxied prototypes. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
There was a problem hiding this comment.
LGTM — straightforward null-deref guard that mirrors the existing fast-path pattern, with regression tests.
Extended reasoning...
Overview
This PR touches two files: a ~10-line defensive change in JSC__JSValue__forEachPropertyImpl (src/bun.js/bindings/bindings.cpp) and added regression tests in test/js/bun/util/inspect.test.js. The C++ change does two things: (1) reorders CLEAR_IF_EXCEPTION to run before the early continue so a throwing Proxy get trap doesn't leave a pending exception, and (2) checks getPrototype() for an empty JSValue (and clears the exception) before calling .getObject(), which previously dereferenced null when a Proxy getPrototypeOf trap threw.
Security risks
None. This is a robustness fix — it converts a user-JS-triggerable null deref / UBSAN crash in Bun.inspect/console.log into graceful handling. No auth, crypto, permissions, or untrusted-input parsing is involved.
Level of scrutiny
Low-to-moderate. The change is purely additive guards with no behavior change on the success path. The new pattern (if (JSValue proto = getPrototype(...)) → check empty → .getObject()) is identical to how the fast path in the same function already handles this at lines ~5180 and ~5257, and how the REPL completion code at ~6402 handles it. The CLEAR_IF_EXCEPTION reorder is a textbook fix for an exception-leak-before-continue bug.
Other factors
- Regression tests cover both crash repros (throwing getter through Proxy prototype, throwing
getPrototypeOftrap) plus a positive test that non-throwing getters still produce correct output, and the cases are also added to the existing crash-testing fixture array. - No CODEOWNERS apply to the touched files.
- The inline comment about
napi_get_all_property_nameshaving the same pattern is pre-existing and explicitly flagged as a follow-up — it doesn't block this fix. - No outstanding reviewer comments on the timeline.
| JSValue proto = iterating->getPrototype(globalObject); | ||
| // Ignore exceptions from Proxy "getPrototypeOf" trap. | ||
| CLEAR_IF_EXCEPTION(scope); | ||
| if (!proto) | ||
| break; | ||
| iterating = proto.getObject(); |
There was a problem hiding this comment.
🟣 Pre-existing, but the same getPrototype(globalObject).getObject() null-deref pattern still lives in napi_get_all_property_names at src/bun.js/bindings/napi.cpp:1837. A native addon calling it with napi_key_include_prototypes + an enumerable/writable/configurable filter on an object whose prototype chain contains a Proxy with a throwing getPrototypeOf (or getOwnPropertyDescriptor) trap will crash the same way. Might be worth applying the same empty-value + RETURN_IF_EXCEPTION guard there as a follow-up.
Extended reasoning...
What the bug is
This PR correctly fixes a null deref in forEachPropertyImpl where getPrototype(globalObject).getObject() was called without checking for an empty JSValue after a Proxy trap throws. However, the identical unfixed pattern still exists in napi_get_all_property_names:
// src/bun.js/bindings/napi.cpp:1833-1842
if (key_mode == napi_key_include_prototypes) {
JSObject* current_object = object;
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 getOwnPropertyDescriptor (which can throw via a Proxy trap) and getPrototype (which can also throw via a Proxy getPrototypeOf trap, or short-circuit on a pending exception). When either throws, getPrototype() returns an empty JSValue, and .getObject() on an empty value is the same asCell()->getObject() with asCell() == nullptr that this PR fixes in bindings.cpp.
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 a Proxy — e.g.Object.setPrototypeOf(obj, new Proxy({}, { getPrototypeOf() { throw new Error(); } })).- Because a key filter is set, the code enters the per-key loop at line 1829.
- The inner while-loop walks the prototype chain. When
current_objectreaches the Proxy:- Path A: the Proxy's
getOwnPropertyDescriptortrap throws →getOwnPropertyDescriptorreturnsfalse→ loop body executes →ProxyObject::getPrototypehits an internalRETURN_IF_EXCEPTIONon the pending exception and returnsJSValue(). - Path B:
getOwnPropertyDescriptorreturnsfalsenormally, then the Proxy'sgetPrototypeOftrap throws →getPrototypereturnsJSValue().
- Path A: the Proxy's
.getObject()is called on the empty value. On 64-bit JSC an emptyJSValueis encoded as0, which passes theNotCellMaskcheck, soisCell()istrue,asCell()isnullptr, andnullptr->getObject()is a null member call → UBSAN crash / segfault.
Why existing code doesn't prevent it
The NAPI_RETURN_IF_EXCEPTION(env) at line 1823 only guards the earlier allPropertyKeys call (which exercises ownKeys/getPrototypeOf, not getOwnPropertyDescriptor). There is no exception check inside the per-key loop, and the if (!proto) break; check at line 1838 only runs after .getObject() has already dereferenced null.
Impact
Any N-API addon that enumerates properties (with prototype inclusion + attribute filtering) on a user-supplied object can be crashed by a hostile or buggy Proxy in the prototype chain. Same severity as the bug this PR fixes — process crash rather than a catchable JS exception.
How to fix
Apply the same guard this PR adds to bindings.cpp:
while (!current_object->getOwnPropertyDescriptor(globalObject, key.toPropertyKey(globalObject), desc)) {
NAPI_RETURN_IF_EXCEPTION(env);
JSValue protoValue = current_object->getPrototype(globalObject);
NAPI_RETURN_IF_EXCEPTION(env);
JSObject* proto = protoValue.getObject();
if (!proto) break;
current_object = proto;
}(Or split the empty check from the exception check, whichever matches local convention.)
Step-by-step proof
Concrete walkthrough with Path B:
- JS:
const obj = {}; Object.setPrototypeOf(obj, new Proxy({}, { getPrototypeOf() { throw new Error('nope'); } }));then a native addon callsnapi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_enumerable, napi_key_numbers_to_strings, &result). allPropertyKeysat line ~1821 invokes the Proxy'sgetPrototypeOfonce and throws — but wait, that would be caught by the line-1823 check. So use a trap that succeeds the first time and throws the second:let n=0; getPrototypeOf(){ if(n++) throw new Error(); return {}; }. NowallPropertyKeyssucceeds.- Loop iteration for some key
k:current_object = obj.obj->getOwnPropertyDescriptor(k)→ false (own property not found). Enter loop body. obj->getPrototype(globalObject)returns the Proxy (no throw — ordinary [[GetPrototypeOf]] on a plain object).current_object = proxy.- Next iteration:
proxy->getOwnPropertyDescriptor(k)→ no trap defined, forwards to target{}, returns false. Enter loop body. proxy->getPrototype(globalObject)→ invokesgetPrototypeOftrap → throws → returnsJSValue()(empty).JSValue().getObject():isCell()checks(u.asInt64 & NotCellMask) == 0→0 & mask == 0→ true.asCell()returnsreinterpret_cast<JSCell*>(0).nullptr->getObject()readsthis->structure()->typeInfo()→ null deref.
This is pre-existing — the PR doesn't touch napi.cpp, add callers to it, or change its behavior — so it shouldn't block merge. Flagging because it's the exact same root cause and the author may want to sweep it in the same pass.
|
Duplicate of #29642, which already covers this crash with additional fixes (napi.cpp getPrototype exception handling and GC safety). |
1 participant
What does this PR do?
Fixes a null pointer dereference in
JSC__JSValue__forEachPropertyImpl(used byBun.inspect,console.log, expect error formatting, etc.) when inspecting an object whose prototype chain contains a Proxy.Root cause
When an object's prototype chain contains a Proxy,
forEachPropertytakes the slow path. Two cases were mishandled:getPropertySlot()withInternalMethodType::Geton a Proxy invokesperformProxyGet, which calls the getter immediately (viaperformDefaultGet→slot.getValue()). If the getter throws,getPropertySlotreturnsfalse. The earlycontinuethen skipped overCLEAR_IF_EXCEPTION(scope), leaving a pending exception for the remainder of the loop.iterating->getPrototype(globalObject)on a Proxy can throw (either from agetPrototypeOftrap, or because of the pending exception from (1) hitting an internalRETURN_IF_EXCEPTION). When it throws it returns an emptyJSValue, and calling.getObject()on that isasCell()->getObject()withasCell() == nullptr.Fix
getPropertySlot()before thecontinue.getPrototype()for an empty value and clear the exception before dereferencing, matching how the fast path already handles this.Repro
How did you verify your code works?
expect().toBe()error instead).test/js/bun/util/inspect.test.js.inspect.test.jspasses (77 tests).