Fix null deref in forEachProperty when Proxy getPrototype throws#30457
Fix null deref in forEachProperty when Proxy getPrototype throws#30457robobun wants to merge 3 commits into
Conversation
When walking the prototype chain during property enumeration for formatting, getPrototype() on a ProxyObject can throw (via a getPrototypeOf trap) or return early due to a pending exception, returning an empty JSValue. Calling .getObject() on an empty JSValue dereferences a null JSCell*. Guard the result of getPrototype() the same way the fast path already does: clear any pending exception and treat an empty result as the end of the chain.
|
Updated 12:10 PM PT - May 10th, 2026
❌ @robobun, your commit 87ac0b8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30457That installs a local version of the PR into your bun-30457 --bun |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughHardens prototype-chain iteration in ChangesProxy Exception Safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mirrors the existing fast-path guard for the same crash, with regression tests.
Extended reasoning...
Overview
This PR fixes a null-pointer dereference in JSC__JSValue__forEachPropertyImpl (src/jsc/bindings/bindings.cpp), the property enumerator backing Bun.inspect / console.log. The slow-path prototype walk called .getObject() on the result of getPrototype() without checking for an empty JSValue, which occurs when a Proxy getPrototypeOf trap throws. The fix stores the result in a local, clears the pending exception, and null-checks before dereferencing — exactly the pattern already used by the fast path of the same function at lines 5327–5337. Two regression tests are added to test/js/bun/util/inspect.test.js.
Security risks
None introduced. This is a strict hardening: it turns a segfault into graceful loop termination. Exceptions are intentionally swallowed here (consistent with every other CLEAR_IF_EXCEPTION in this function, which is best-effort formatting code). No new attack surface, no behavior change beyond "don't crash".
Level of scrutiny
Low. The diff is 4 lines of C++ that copy an existing, adjacent idiom verbatim. The test additions are straightforward "does not crash" assertions. There are no design decisions, no API changes, and the function's contract (best-effort, swallow user-code exceptions during inspection) is preserved.
Other factors
- The one inline comment from the bug-hunting system is explicitly pre-existing (same pattern in
napi.cpp), not introduced or affected by this PR — it's a follow-up suggestion, not a blocker. - No CODEOWNERS for this path.
- Found by Fuzzilli with a clear repro; the tests directly encode that repro.
| 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 vulnerable pattern this PR fixes still exists at src/jsc/bindings/napi.cpp:1837 (current_object->getPrototype(globalObject).getObject()), reachable via napi_get_all_property_names with napi_key_include_prototypes when the chain contains a Proxy with a throwing getPrototypeOf or getOwnPropertyDescriptor trap. This is pre-existing and not touched by this PR — just flagging since it's the identical root cause and may be worth the same one-line guard as a follow-up.
Extended reasoning...
Summary
This PR correctly fixes a null deref in JSC__JSValue__forEachPropertyImpl where iterating->getPrototype(globalObject).getObject() is called on a potentially-empty JSValue. However, the identical pattern remains unfixed 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();
if (!proto) {
break;
}
current_object = proto;
}There is no exception check or empty-JSValue guard between the getOwnPropertyDescriptor call and the .getObject() call on the next line.
How it manifests
As the PR description explains, when getPrototype() is called on a ProxyObject and the getPrototypeOf trap throws (or there is already a pending exception, causing ProxyObject::performGetPrototype to early-return {} via RETURN_IF_EXCEPTION), it returns an empty JSValue. An empty JSValue reports isCell() == true but asCell() == nullptr, so .getObject() calls JSCell::getObject() on nullptr and segfaults — exactly the crash this PR fixes in bindings.cpp.
Why the existing guard at line 1823 doesn't prevent it
allPropertyKeys() at line 1818 also walks the prototype chain, and NAPI_RETURN_IF_EXCEPTION(env) at line 1823 would catch an unconditionally-throwing getPrototypeOf trap on that first walk. However, the descriptor-filtering loop at lines 1833–1842 walks the chain again per key, and is reachable past line 1823 in at least two ways:
- Throwing
getOwnPropertyDescriptortrap. Line 1836 callsgetOwnPropertyDescriptoron the Proxy. If that trap throws, it returnsfalse(loop body entered) and leaves a pending exception. Line 1837 then callsgetPrototype()on the same Proxy with the exception pending → emptyJSValue→ null deref. TheownKeys/getPrototypeOftraps used byallPropertyKeysneed not throw for this path. - Stateful
getPrototypeOftrap. A trap that succeeds duringallPropertyKeysbut throws on a subsequent invocation (e.g. throws on the Nth call) reaches line 1837 directly.
Step-by-step proof (path 1)
- Native addon calls
napi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_writable, ...)whereobj's prototype isnew Proxy(target, { getOwnPropertyDescriptor() { throw new Error('x') } }). - Line 1818:
allPropertyKeyswalks the chain viagetPrototype/ownKeys— nogetOwnPropertyDescriptortrap invoked, no throw. Line 1823 passes. - Line 1827:
key_filter & filter_by_any_descriptoris truthy (napi_key_writable), so we enter the per-key filter loop. - For some key, line 1836 calls
getOwnPropertyDescriptoronobj→ not own → loop iterates,current_objectadvances to the Proxy. - Line 1836 (next iteration):
getOwnPropertyDescriptoron the Proxy invokes the trap → throws → returnsfalse, pending exception set. - Line 1837:
current_object->getPrototype(globalObject)on the Proxy with a pending exception returnsJSValue{}. .getObject()on the empty value:isCell()→ true,asCell()→nullptr,nullptr->getObject()→ segfault.
Impact
A native addon calling napi_get_all_property_names with napi_key_include_prototypes and any of napi_key_enumerable | napi_key_writable | napi_key_configurable on user-supplied objects can be crashed by a hostile Proxy in the prototype chain. Same severity class as the bug this PR fixes (process crash from JS-observable input), just gated behind N-API rather than Bun.inspect.
Suggested fix
Apply the same guard the PR uses in bindings.cpp:
JSValue protoVal = current_object->getPrototype(globalObject);
NAPI_RETURN_IF_EXCEPTION(env); // or CLEAR_IF_EXCEPTION + break
JSObject* proto = protoVal ? protoVal.getObject() : nullptr;(and ideally also check for an exception after getOwnPropertyDescriptor at line 1836).
Severity
Pre-existing. This PR does not touch napi.cpp, add callers to it, or change its behavior in any way. Flagging only because it is the identical root cause and the fix is the same one-liner — fine to defer to a follow-up.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
When getPropertySlot returns false with a pending exception (Proxy trap throws), the continue skipped CLEAR_IF_EXCEPTION, leaving the exception pending into the next getPropertySlot call and tripping JSC's exception-scope verification.
There was a problem hiding this comment.
LGTM — mirrors the existing fast-path guard in the same function, with regression tests.
Extended reasoning...
Overview
Two small edits to JSC__JSValue__forEachPropertyImpl in src/jsc/bindings/bindings.cpp plus two regression tests in test/js/bun/util/inspect.test.js. The first hunk reorders CLEAR_IF_EXCEPTION to run before continue so a throwing getPropertySlot doesn't leak a pending exception into the next iteration. The second hunk captures getPrototype() into a local JSValue, clears any Proxy-trap exception, and only calls .getObject() when the value is non-empty — preventing the null deref that Fuzzilli found.
Security risks
None introduced. This strictly hardens an existing code path against a JS-triggerable segfault (DoS-class). No new attack surface, no auth/crypto/permissions involvement.
Level of scrutiny
Low. The slow-path fix is a copy of the guard already present in the fast path of the same function (lines ~5327–5337: if (JSValue proto = ...) + CLEAR_IF_EXCEPTION). When proto is empty, iterating becomes nullptr and the while condition exits cleanly; when proto is a non-object (e.g. null), getObject() returns nullptr and the loop also exits — identical to prior semantics minus the crash. The getPropertySlot reorder is a pure defensive fix with no behavioral change in the non-exception case.
Other factors
Regression tests cover both repros from the PR description. The duplicate-PR bot lists 8 other PRs proposing the identical fix, which corroborates that this is the canonical patch for this fuzzer fingerprint. My earlier inline note about napi.cpp:1837 is pre-existing and explicitly out of scope for this PR. No CODEOWNERS apply to these paths.
There was a problem hiding this comment.
LGTM — mirrors the existing fast-path guard in the same function, with regression tests.
Extended reasoning...
Overview
Two small defensive changes in JSC__JSValue__forEachPropertyImpl (src/jsc/bindings/bindings.cpp) plus two regression tests in test/js/bun/util/inspect.test.js:
- Reorder
CLEAR_IF_EXCEPTION(scope)to run before thecontinuewhengetPropertySlotreturns false, so a throwing Proxygettrap doesn't leave a pending exception that poisons the subsequentgetPrototype()call. - Capture
iterating->getPrototype(globalObject)into aJSValue, clear anygetPrototypeOf-trap exception, and only call.getObject()when the value is non-empty — preventing the null deref on an emptyJSValue.
Both changes exactly mirror the pre-existing fast-path handling at lines ~5327–5337 of the same function.
Security risks
None introduced. This is a robustness fix that turns a JS-triggerable segfault (DoS) into a graceful no-op during property enumeration for Bun.inspect/console.log. No new attack surface, no auth/crypto/permissions involved.
Level of scrutiny
Low-to-medium. The change is ~7 lines of native C++ in a hot inspection path, but it's purely defensive, follows the established pattern already used a few lines above in the same function, and is covered by two direct regression tests. The Fuzzilli-found crash and repro are well-characterized in the PR description.
Other factors
- Bug-hunting system found no issues in the diff itself.
- My earlier inline note about
napi.cpp:1837is a pre-existing sibling issue explicitly flagged as fine for a follow-up; it doesn't block this PR. - CI failures (musl build,
bun-install-registry,s3-storage-class, http2/abort flakes) are unrelated to this change. - The duplicate-PR bot lists several prior attempts at the same fix, which is a repo workflow matter rather than a correctness concern here.
|
CI failures are pre-existing flakes unrelated to this change:
|
1 participant
Found by Fuzzilli. Fingerprint:
f363b9d446e2156fWhat
JSC__JSValue__forEachPropertyImplwalks the prototype chain to enumerate properties forBun.inspect/console.logformatting. At the end of each iteration it advances via:iterating = iterating->getPrototype(globalObject).getObject();When
iteratingis aProxyObject,getPrototype()can throw (via agetPrototypeOftrap) or return early due to an already-pending exception, returning an emptyJSValue. An emptyJSValuereportsisCell() == truebutasCell() == nullptr, so.getObject()dereferences a null pointer.The fast path in the same function already guards against this (checking
if (JSValue proto = ...)and clearing the exception). This applies the same guard to the slow path.Repro
Also reproduces with an empty Proxy handler wrapping a native prototype that has throwing accessors (e.g.
expect()instances).