Fix null deref in Bun.inspect when Proxy getPrototypeOf trap throws#30517
Fix null deref in Bun.inspect when Proxy getPrototypeOf trap throws#30517robobun wants to merge 1 commit into
Conversation
When walking the prototype chain in forEachPropertyImpl, getPrototype() can throw (e.g. via a Proxy getPrototypeOf trap) and return an empty JSValue. Calling .getObject() on an empty JSValue dereferences null.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR hardens exception handling during prototype chain traversal in JSValue property enumeration. The core fix in ChangesPrototype Walking Exception Handling
🚥 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-check + exception-clear matching the fast path's existing handling; the napi.cpp note is pre-existing and non-blocking.
Extended reasoning...
Overview
This PR fixes a fuzzer-found null dereference in JSC__JSValue__forEachPropertyImpl (src/jsc/bindings/bindings.cpp). When walking the prototype chain during Bun.inspect(), iterating->getPrototype(globalObject) can throw via a Proxy getPrototypeOf trap and return an empty JSValue; calling .getObject() on that segfaults. The fix stores the result, clears the exception with CLEAR_IF_EXCEPTION(scope), and null-checks before .getObject(). Two regression cases are added to the existing crash-testing fixture in test/js/bun/util/inspect.test.js.
Security risks
None. This is a defensive null check in the property-enumeration path used for object inspection/formatting. It swallows an exception in a context where the surrounding code already swallows exceptions from getters and other proxy traps, so behavior is consistent. No auth, crypto, permissions, or untrusted input parsing is involved.
Level of scrutiny
Low. The change is 4 lines, mechanical, and mirrors the identical handling already present in the fast path of the same function (lines ~5336–5337: CLEAR_IF_EXCEPTION(scope) after a getPrototype call). The loop already terminates when iterating becomes nullptr, so treating a thrown trap as end-of-chain is the natural semantics here. The affected surface is Bun.inspect/console.log formatting, where best-effort output is expected.
Other factors
The only finding from the bug-hunting pass is a pre-existing instance of the same pattern in napi.cpp (napi_get_all_property_names), which this PR does not touch — flagged as a non-blocking follow-up suggestion, not a defect in this change. No CODEOWNERS cover this path. Regression tests are added to the existing crash fixture, which wraps Bun.inspect in try/catch and only fails on a process crash, so they exercise exactly the segfault case.
| 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: the same unsafe pattern exists at src/jsc/bindings/napi.cpp:1837 in napi_get_all_property_names — current_object->getPrototype(globalObject).getObject() with no exception check, so a Proxy getPrototypeOf trap that throws will null-deref there too. This is pre-existing and not touched by this PR, but since a grep for getPrototype(globalObject).getObject() finds it, you may want to apply the same CLEAR_IF_EXCEPTION + empty-check fix there in this PR or a follow-up. Non-blocking.
Extended reasoning...
What the bug is
This PR correctly fixes a null dereference in JSC__JSValue__forEachPropertyImpl (bindings.cpp:5447) where iterating->getPrototype(globalObject).getObject() crashes when a Proxy's getPrototypeOf trap throws. However, the identical pattern remains at src/jsc/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(); // <-- same unsafe pattern
if (!proto) {
break;
}
current_object = proto;
}There is no RETURN_IF_EXCEPTION / CLEAR_IF_EXCEPTION between the getPrototype() call and .getObject(), so the exact crash class fixed by this PR still exists in the napi path.
Code path that triggers it
A native addon calls napi_get_all_property_names with:
key_mode = napi_key_include_prototypes(so the inner while-loop walks the prototype chain), andkey_filtercontaining any ofnapi_key_enumerable | napi_key_writable | napi_key_configurable(so the descriptor-filtering block at line 1827 is entered),
on an object whose prototype chain contains a Proxy with a throwing getPrototypeOf trap.
Why existing code doesn't prevent it
There's a NAPI_RETURN_IF_EXCEPTION(env) at line 1823 after the initial allPropertyKeys() walk, so a trivially-always-throwing trap would be caught there. But a stateful trap — e.g., one that returns normally for the first N calls (during allPropertyKeys's prototype walk) and throws on a subsequent call (during the per-key descriptor-filtering walk at line 1837) — passes the line-1823 check and reaches line 1837. At that point getPrototype() throws and returns an empty JSValue; per the PR description's own analysis, an empty JSValue passes isCell() but asCell() is nullptr, so .getObject() invokes JSCell::getObject() on null → segfault.
Step-by-step proof
- JS sets up:
let n = 0; const p = new Proxy({}, { getPrototypeOf() { if (n++ > 3) throw new Error('nope'); return null; } }); const obj = Object.create(p); obj.x = 1; - Native addon calls
napi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_enumerable, napi_key_numbers_to_strings, &result). - Line 1818-ish:
allPropertyKeys()walks the chain, calling the trap a few times;nincrements but stays under the threshold; no exception → line 1823 passes. - Line 1827:
key_filter & filter_by_any_descriptoris true → enter filtering loop. - For key
"x", line 1836:getOwnPropertyDescriptoronobjsucceeds, loop body skipped. But for any key contributed by the prototype (or if the descriptor lookup misses onobj), the while-loop at 1836 runs and line 1837 callscurrent_object->getPrototype(globalObject). - The trap now throws (
n > 3).getPrototype()returnsJSValue()(empty). JSValue().getObject()→isCell()is true for empty (bits == 0 has no number/undefined/null tag) →asCell()returnsnullptr→nullptr->getObject()readstype()through a nullthis→ crash.
Impact
Process crash (SIGSEGV) reachable from any native addon that uses napi_get_all_property_names with prototype inclusion + descriptor filtering on user-controlled objects. Lower exposure than the Bun.inspect path since it requires a native addon, but it's the same crash class.
How to fix
Apply the same pattern as this PR:
JSValue protoVal = current_object->getPrototype(globalObject);
CLEAR_IF_EXCEPTION(scope); // or NAPI_RETURN_IF_EXCEPTION(env) if propagating is preferred
JSObject* proto = protoVal ? protoVal.getObject() : nullptr;
if (!proto) break;Severity
Pre-existing — this PR does not touch napi.cpp, add callers to it, or otherwise interact with this code path. Mentioning it only because the PR's stated goal is fixing exactly this crash class and a grep for getPrototype(globalObject).getObject() finds this remaining instance. Non-blocking; fine to address in a follow-up.
1 participant
Fuzzer found a crash (fingerprint
b01f1338ddbd3ba7) whereBun.inspect()/console.log()dereferences null when the inspected object's prototype chain contains a Proxy whosegetPrototypeOftrap throws.Repro
Root cause
In
JSC__JSValue__forEachPropertyImpl, when walking the prototype chain:iterating = iterating->getPrototype(globalObject).getObject();getPrototype()can throw (via a Proxy'sgetPrototypeOftrap) and return an emptyJSValue. An emptyJSValuepassesisCell()butasCell()returnsnullptr, so.getObject()calls a method on null.Fix
Clear the exception and treat an empty result as end of the prototype chain, matching how the fast path in the same function already handles this case.