Fix null deref in Bun.inspect when Proxy getPrototypeOf trap throws#30318
Fix null deref in Bun.inspect when Proxy getPrototypeOf trap throws#30318robobun wants to merge 2 commits into
Bun.inspect when Proxy getPrototypeOf trap throws#30318Conversation
|
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)
WalkthroughThis change hardens prototype traversal in fast-path JS property enumeration to safely handle Proxy ChangesProxy-Safe Prototype Traversal
🚥 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 |
|
Duplicate of #29642 (and several others). Closing. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 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 same unguarded pattern lives at src/jsc/bindings/napi.cpp:1837 — current_object->getPrototype(globalObject).getObject() — reachable via napi_get_all_property_names with napi_key_include_prototypes + a writable/enumerable/configurable filter. A Proxy whose getPrototypeOf trap throws will cause the same null JSCell* deref there; the if (!proto) on the next line doesn't help because the UB happens inside .getObject(). Since you're fixing the same root cause here, you may want to apply the same JSValue-then-check guard there too.
Extended reasoning...
What the bug is
The exact pattern this PR fixes — calling .getObject() directly on the result of getPrototype(globalObject) without first checking whether the returned JSValue is empty — also exists at src/jsc/bindings/napi.cpp:1837:
while (!current_object->getOwnPropertyDescriptor(globalObject, key.toPropertyKey(globalObject), desc)) {
JSObject* proto = current_object->getPrototype(globalObject).getObject();
if (!proto) {
break;
}
current_object = proto;
}When current_object (or something in its prototype chain) is a ProxyObject whose getPrototypeOf trap throws, getPrototype() returns an empty JSValue. As demonstrated by the UBSAN trace in this PR's description, an empty JSValue (encoded as 0) passes the isCell() check (it doesn't have NotCellMask bits set), so .getObject() proceeds to call asCell()->isObject() on a null JSCell* — undefined behavior / null deref.
Why the existing if (!proto) doesn't help
It's tempting to think the if (!proto) break; on line 1838 covers this, but it doesn't: the null dereference happens inside .getObject() on line 1837, before proto is ever assigned. This is exactly the failure mode the PR fixes in bindings.cpp (the original code there was iterating->getPrototype(globalObject).getObject(), and the fix splits it into JSValue proto = ...; iterating = proto ? proto.getObject() : nullptr;).
How it's reachable
This loop sits inside napi_get_all_property_names and runs when called with key_mode = napi_key_include_prototypes and a key_filter that includes any of napi_key_writable | napi_key_enumerable | napi_key_configurable (so the per-key descriptor-lookup loop at line 1829 executes). The NAPI_RETURN_IF_EXCEPTION at line 1823 catches an exception thrown during the initial allPropertyKeys walk, but the per-key loop calls getPrototype again for every key — so a stateful trap (succeeds the first N calls, throws on call N+1), or a trap that only throws after getOwnPropertyDescriptor has run, will reach line 1837 with a pending exception and an empty return value.
Step-by-step proof
- Native addon calls
napi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_writable, napi_key_numbers_to_strings, &result)whereobjhas a Proxy in its prototype chain whosegetPrototypeOftrap is stateful (e.g., returns{}the first few times, thenthrow new Error("trap")). - Line 1818
allPropertyKeyswalks the chain — trap returns normally, no exception. Line 1823 passes. key_filter & filter_by_any_descriptoris true → enter the per-key loop.- For some key,
getOwnPropertyDescriptorreturns false onobject, so we enter thewhilebody and callcurrent_object->getPrototype(globalObject). - The Proxy trap now throws.
getPrototype()returns an emptyJSValue(encoded0). .getObject()→isCell()on encoded0returns true →asCell()returnsnullptr→nullptr->isObject()→ null deref, identical to the UBSAN trace in this PR.
Impact and fix
Impact: a native module using this N-API combination on a user-controlled object with a hostile Proxy prototype hits UB / a crash. Same severity class as the bug being fixed in this PR, just on a less-traveled path.
Fix: apply the same guard this PR adds in bindings.cpp:
JSValue protoValue = current_object->getPrototype(globalObject);
RETURN_IF_EXCEPTION(scope, ...); // or CLEAR_IF_EXCEPTION + break, matching local conventions
JSObject* proto = protoValue ? protoValue.getObject() : nullptr;
if (!proto) break;This is pre-existing — the PR doesn't touch napi.cpp and doesn't make it any worse — but since it's the same root cause and the same one-line fix, it seemed worth flagging here for completeness.
1 participant
What does this PR do?
Fixes a null pointer dereference in
JSC__JSValue__forEachPropertyImpl(used byBun.inspect,console.log, andexpecterror formatting) when walking the prototype chain of an object whose prototype is aProxy.When
getPrototype()is called on aProxyObject, thegetPrototypeOftrap may throw, causinggetPrototype()to return an emptyJSValue. Calling.getObject()on an emptyJSValuethen dereferences a nullJSCell*.Repro
The fix mirrors the existing handling of the same call earlier in the function: check the returned
JSValuebefore calling.getObject()and clear any pending exception from the trap.How did you verify your code works?
Added regression tests in
test/js/bun/util/inspect.test.jscovering:Proxyprototype with a throwinggetPrototypeOftrapProxyprototype wrapping a prototype with a throwing getterexpect()result with aProxyprototype (the original fuzzer scenario)Found by Fuzzilli. Fingerprint:
0948b210d37148a3