inspect: handle throwing Proxy getPrototypeOf in forEachProperty - #29814
inspect: handle throwing Proxy getPrototypeOf in forEachProperty#29814robobun wants to merge 3 commits into
Conversation
|
Updated 2:05 PM PT - May 4th, 2026
❌ @robobun, your commit f994d1f has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29814That installs a local version of the PR into your bun-29814 --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 (1)
WalkthroughAdjusts property enumeration to preserve control flow around exception-prone proxy operations by explicitly capturing and clearing prototype and property-lookup results. Adds tests ensuring Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/util/inspect.test.js`:
- Around line 483-495: The test "inspecting object with Proxy prototype that
leaves a pending exception during enumeration doesn't crash" currently relies on
a throwing getter (target.second) which Bun.inspect is known not to call; change
it to use a Proxy trap that is actually consulted during property enumeration
(e.g., ownKeys or getOwnPropertyDescriptor) so the exception path is exercised.
Replace Object.setPrototypeOf(obj, new Proxy(target, {})) with a proxy whose
ownKeys or getOwnPropertyDescriptor throws, set a local flag or spy to verify
the trap was invoked, and assert that calling Bun.inspect(obj) does not throw
while also asserting the trap was hit. Ensure references to Bun.inspect, the
test name, and the Proxy trap (ownKeys/getOwnPropertyDescriptor) are updated
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 177758a9-4120-42bb-b5ff-3d05a1932a79
📒 Files selected for processing (2)
src/bun.js/bindings/bindings.cpptest/js/bun/util/inspect.test.js
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Related earlier attempts (both mine, both with stale/red CI):
This PR is the minimal, targeted version of the |
|
CI failures on build 48468 are all unrelated to this change — every failing test is in the experimental HTTP/2 fetch client that landed yesterday (#29766):
Same ASAN shard is red on #29812, #29804, #29795 right now. #29809 and #29812 are hardening the h2 client. All 12 other |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted null-deref fix with regression tests for both trigger paths.
Extended reasoning...
Overview
This PR fixes a fuzzer-found null-pointer dereference in JSC__JSValue__forEachPropertyImpl (src/bun.js/bindings/bindings.cpp), the property enumerator behind Bun.inspect/console.log. Two tightly-scoped changes:
- Reorder
CLEAR_IF_EXCEPTION(scope)to run before theif (!hasProperty) continue;check, so a pending exception from a Proxygettrap (which can causegetPropertySlotto returnfalse) is cleared rather than leaking into the next iteration. - Capture
iterating->getPrototype(globalObject)into a local, clear any exception (ProxygetPrototypeOftrap may throw), andbreakif the result is empty before calling.getObject()— avoidingasCell()on an emptyJSValue.
Two regression tests are added to test/js/bun/util/inspect.test.js covering both trigger paths; the second test includes a getterCalled assertion (added in 7e297bf) so it self-verifies that the slow path is actually exercised.
Security risks
None. The change is purely defensive — adding an exception-clear and an empty-value guard. No new user-controlled data flows, no auth/crypto/permissions surface. The worst-case behavioral change is that Bun.inspect now stops walking the prototype chain (instead of crashing) when a hostile Proxy trap throws, which is the intended and pre-existing design of this function (it already swallows exceptions throughout).
Level of scrutiny
Low-to-moderate. This is ~10 lines of C++ in a non-hot-path slow branch of the inspector, strictly narrowing crash conditions. The no-exception path is byte-for-byte equivalent to the old code. The pattern matches how the fast path in the same function already handles getPrototype, and the surrounding code already uses CLEAR_IF_EXCEPTION/tryClearException liberally to keep Bun.inspect non-throwing.
Other factors
- The one CodeRabbit concern (whether the second test exercises the intended path) was addressed and acknowledged as resolved.
- CI failures on build #48468 are documented as unrelated HTTP/2 fetch-client flakes affecting multiple PRs on the same shards; 12/16
test-bunshards passed. - Related PRs #29642/#29671 overlap but this is the minimal, current-
mainversion; the author has flagged willingness to close redundant ones. - No bugs were found by the bug-hunting system.
There was a problem hiding this comment.
LGTM — defensive null-check matching the existing fast-path pattern in the same function.
Extended reasoning...
Overview
This PR fixes a null-pointer dereference in JSC__JSValue__forEachPropertyImpl (src/bun.js/bindings/bindings.cpp), the property-enumeration helper backing Bun.inspect/console.log. Two small changes: (1) move CLEAR_IF_EXCEPTION to run regardless of getPropertySlot's return value so a pending exception from a Proxy get trap doesn't leak into the next iteration, and (2) capture getPrototype() into a local, clear any exception, and break if empty before calling .getObject(). Two regression tests are added to test/js/bun/util/inspect.test.js.
Security risks
None. The change is purely defensive — it adds an exception-clear and an empty-value guard. Bun.inspect is output formatting; the only effect is that adversarial Proxy traps in the prototype chain now terminate iteration cleanly instead of crashing. No new code paths, no user input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-moderate. The diff is ~10 lines of C++ that mirror the existing fast-path handling at lines 5243–5253 of the same function (if (JSValue proto = ...getPrototype(...)) + CLEAR_IF_EXCEPTION). The reorder of CLEAR_IF_EXCEPTION before continue is semantically obvious — it just ensures the clear happens on both branches. The if (!proto) break; guard is the standard JSC idiom for handling empty JSValue from a throwing internal method.
Other factors
- Fuzzer-found with a concrete repro and fingerprint.
- Two regression tests cover both the
getPrototypeOf-throws case and the pending-exception-from-getter case; the second test was hardened with agetterCalledassertion after CodeRabbit feedback (now resolved). - Bug-hunting system found no issues.
- CI failures on build 48468 are documented as unrelated HTTP/2 client flakes (
fetch-http2-*.test.ts) also red on contemporaneous PRs; all 12 other shards passed. - The author noted overlap with #29642/#29671; this is the minimal targeted version against current main.
|
Build 49024 (after merging latest main): Shard Shard (src/bun.js/event_loop.zig:644) — pre-existing worker-teardown race. Current |
There was a problem hiding this comment.
LGTM — targeted null-deref fix that mirrors the fast-path's existing getPrototype guard, with regression tests for both crash paths.
Extended reasoning...
Overview
This PR fixes a fuzzer-found null-pointer dereference in JSC__JSValue__forEachPropertyImpl (src/bun.js/bindings/bindings.cpp), the property enumerator used by Bun.inspect/console.log. Two changes: (1) reorder CLEAR_IF_EXCEPTION to run regardless of getPropertySlot's return value so a Proxy get trap exception doesn't leak into the next iteration, and (2) capture getPrototype() into a local, clear any exception, and break on empty before calling .getObject() — preventing the asCell()->getObject() null deref when a Proxy getPrototypeOf trap throws. Two regression tests are added to test/js/bun/util/inspect.test.js.
Security risks
None. This is a crash-hardening change in a read-only inspection path. It does not touch auth, crypto, permissions, or any trust boundary. If anything, it reduces a denial-of-service vector (adversarial Proxy crashing the process via console.log).
Level of scrutiny
Low-to-moderate. The diff is ~10 lines of mechanical change inside a single function. The new getPrototype guard exactly mirrors the pattern already used twice in the same function's fast path (lines 5166 and 5243: if (JSValue proto = ...->getPrototype(...))). The getPropertySlot reorder is a trivial control-flow fix — the CLEAR_IF_EXCEPTION was already there, just unreachable when hasProperty was false. There is no behavior change for non-throwing inputs.
Other factors
- The CodeRabbit concern about the second test not exercising the getter path was addressed in 7e297bf with a
getterCalledassertion that makes the test self-verifying; CodeRabbit retracted the comment. - CI failures on builds 48468 and 49024 are all in unrelated subsystems (HTTP/2 fetch client, html-rewriter-leak, worker-nested-uncaught) that are also red on current
mainper the author's analysis; none touch property enumeration orBun.inspect. - The author noted overlap with #29642/#29671; this PR is the minimal targeted version of the
forEachPropertyImplportion. - No CODEOWNERS entry covers this file.
There was a problem hiding this comment.
LGTM — targeted null-deref fix that mirrors the existing fast-path getPrototype guard in the same function, with self-verifying regression tests.
Extended reasoning...
Overview
This PR fixes a fuzzer-found null-pointer dereference in JSC__JSValue__forEachPropertyImpl (src/bun.js/bindings/bindings.cpp), the property-enumeration routine behind Bun.inspect/console.log. Two changes: (1) clear exceptions after getPropertySlot regardless of return value (previously only cleared on the true path, so a Proxy get trap that throws and returns false would leak a pending exception into the next iteration), and (2) capture getPrototype() into a local, clear exceptions, and break on empty before calling .getObject() (previously iterating->getPrototype(...).getObject() would null-deref when the trap threw). Two regression tests are added to test/js/bun/util/inspect.test.js.
Security risks
None. This is a defensive guard that converts a crash into a graceful loop termination. No data exposure, auth, or permission surface is touched. The exceptions being swallowed are intentional — Bun.inspect is best-effort and already clears exceptions throughout this function.
Level of scrutiny
Low-to-moderate. While bindings.cpp is a core file, the change is ~10 lines confined to one function and is a near-verbatim copy of the pattern already used in the fast path of the same function (lines ~5243–5253: if (JSValue proto = ...) followed by CLEAR_IF_EXCEPTION(scope)). The getPropertySlot reorder is a pure statement reordering that preserves semantics on the success path and only adds exception clearing on the failure path. Non-exceptional behavior is unchanged.
Other factors
- The bug hunting system found no issues.
- The one CodeRabbit concern (whether the second test exercises the intended path) was addressed with a
getterCalledassertion that makes the test self-verifying; CodeRabbit acknowledged and resolved the thread. - CI failures are documented in detail by the author as pre-existing flakes on
main(HTTP/2 fetch client, HTMLRewriter leak, worker-teardown race) with no overlap with property enumeration. - The author noted overlapping PRs #29642/#29671 and clarified this is the minimal targeted version; that's a process question for whichever lands first, not a correctness concern with this diff.
- No CODEOWNERS entry covers this path.
|
Build 49062 (
All Linux shards (debian, ubuntu, alpine — x64/aarch64/baseline) and all 20 ASAN shards passed. |
When walking the prototype chain during property enumeration for Bun.inspect/console.log, getPrototype() on a Proxy can return an empty JSValue if the getPrototypeOf trap throws or if a pending exception exists from a prior getPropertySlot call that returned false. Calling .getObject() on an empty JSValue invokes a member function on a null JSCell pointer. Clear pending exceptions after getPropertySlot regardless of its return value, and clear exceptions + check for an empty result after getPrototype before dereferencing.
fc170cd to
5159905
Compare
There was a problem hiding this comment.
LGTM — minimal defensive null-guard that mirrors the fast-path's existing getPrototype/CLEAR_IF_EXCEPTION pattern.
Extended reasoning...
Overview
This PR fixes a fuzzer-found null-deref in JSC__JSValue__forEachPropertyImpl (the property enumerator behind Bun.inspect/console.log). Two changes in src/jsc/bindings/bindings.cpp:
- Reorder
CLEAR_IF_EXCEPTION(scope)to run before theif (!hasProperty) continue;so a Proxygettrap that throws and causesgetPropertySlotto returnfalsedoesn't leak a pending exception into the next iteration. - Capture
iterating->getPrototype(globalObject)into a local, clear exceptions, andbreakon an empty result before calling.getObject()— avoiding theasCell()null deref when a ProxygetPrototypeOftrap throws.
Two crash-safety tests are added to test/js/bun/util/inspect.test.js, the second with a self-verifying getterCalled assertion so it fails loudly if the slow path ever stops exercising the getter.
Security risks
None. This is purely defensive exception/null handling inside the inspect formatter; no auth, crypto, permissions, or user-input parsing is involved.
Level of scrutiny
Low. The diff is ~10 lines of logic and is a direct mirror of the fast-path handling already present in the same function (lines ~5327–5337: if (JSValue proto = prototypeObject.getPrototype(...)) followed by CLEAR_IF_EXCEPTION). On the no-exception happy path the new code is semantically identical to the old code, so there is no behavior change for non-Proxy objects — only the crash on hostile Proxy traps is eliminated.
Other factors
- Bug-hunting system found no issues.
- CodeRabbit raised one concern (test not exercising the intended path) which was rebutted by the author and retracted; the thread is resolved and a
getterCalledassertion was added in 7e297bf to make the test self-verifying. - No CODEOWNERS rules cover these files.
- The author has documented that all CI failures across builds 48468/49024/49062 are pre-existing flakes (HTTP/2 client, HTMLRewriter leak, worker-teardown race, GitHub 403 rate limits) unrelated to property enumeration;
inspect.test.jspasses on every shard that ran it. - Overlapping PRs #29642/#29671 exist; the author has acknowledged this and positioned this PR as the minimal targeted version. Which one to land is a maintainer call, but it doesn't affect the correctness of this change.
There was a problem hiding this comment.
LGTM — small defensive null-guard that mirrors the existing fast-path getPrototype handling in the same function.
Extended reasoning...
Overview
This PR fixes a fuzzer-found null-pointer dereference in JSC__JSValue__forEachPropertyImpl (src/jsc/bindings/bindings.cpp), the property-enumeration routine behind Bun.inspect/console.log. Two changes: (1) move CLEAR_IF_EXCEPTION after getPropertySlot to fire regardless of return value so a throwing Proxy get trap doesn't leave a pending exception, and (2) capture iterating->getPrototype() into a local JSValue, clear exceptions, and break on empty before calling .getObject(). Two regression tests are added to test/js/bun/util/inspect.test.js covering both the throwing-getPrototypeOf-trap case and the pending-exception-during-enumeration case (the latter with a self-verifying getterCalled assertion added in response to CodeRabbit feedback, which is now resolved).
Security risks
None. The change is purely defensive — adding a null check and an exception-clear in a code path that previously dereferenced null. It does not introduce new data flows, parse untrusted input differently, or change any auth/permission logic. If anything it removes a DoS vector (user-controlled object passed to console.log could crash the process).
Level of scrutiny
Low-to-moderate. While bindings.cpp is core JSC glue, the diff is ~10 lines and is a direct copy of the pattern already used by the fast path in the same function (lines 5327–5337: if (JSValue proto = ...getPrototype(...)) { ... } CLEAR_IF_EXCEPTION(scope);). In the non-exception case behavior is identical to before (proto.getObject() returns the same pointer the old chained call would have); in the exception case the code now cleanly exits the prototype walk instead of invoking UB. There are no design decisions to weigh here.
Other factors
- The single bug-hunter finding is explicitly tagged pre-existing / non-blocking — it points at the same pattern in
napi.cpp, which this PR does not touch, and the author has already noted that sibling PR #29642 covers it. - CodeRabbit's one concern (test not exercising the intended path) was addressed in 7e297bf with a
getterCalledassertion and the thread is resolved. - The author triaged four CI runs (48468, 49024, 49062, 51059); all failures are unrelated infra/flake issues (HTTP/2 client tests from #29766, HTMLRewriter leak test, worker-teardown race, GitHub 403s, queue-expired darwin jobs). All Linux and all 20 ASAN shards passed on build 49062, and
inspect.test.jspasses on every shard that ran it. - No CODEOWNERS apply to the touched files.
| JSValue proto = iterating->getPrototype(globalObject); | ||
| // Ignore exceptions from Proxy "getPrototypeOf" traps or leftover exceptions from the loop above. | ||
| CLEAR_IF_EXCEPTION(scope); | ||
| if (!proto) | ||
| break; | ||
| iterating = proto.getObject(); |
There was a problem hiding this comment.
🟣 Pre-existing / non-blocking: the identical unguarded getPrototype(globalObject).getObject() pattern this PR fixes also exists at src/jsc/bindings/napi.cpp:1837 inside napi_get_all_property_names — a Proxy with a throwing getPrototypeOf/getOwnPropertyDescriptor trap in the prototype chain triggers the same null-member-call there. Sibling PR #29642 bundled that napi.cpp fix; flagging so it isn't lost if #29642 is closed in favor of this minimal PR.
Extended reasoning...
What
This PR correctly hardens JSC__JSValue__forEachPropertyImpl by capturing the result of iterating->getPrototype(globalObject), clearing exceptions, and breaking on an empty JSValue before calling .getObject(). The exact same vulnerable pattern remains at src/jsc/bindings/napi.cpp:1837 in napi_get_all_property_names:
while (!current_object->getOwnPropertyDescriptor(globalObject, key.toPropertyKey(globalObject), desc)) {
JSObject* proto = current_object->getPrototype(globalObject).getObject(); // <-- same null-deref
if (!proto) {
break;
}
current_object = proto;
}There is no CLEAR_IF_EXCEPTION / RETURN_IF_EXCEPTION between the getOwnPropertyDescriptor call (which can throw via a Proxy getOwnPropertyDescriptor trap) and the getPrototype() call (which can throw via a Proxy getPrototypeOf trap), and no empty-JSValue guard before .getObject(). The if (!proto) check on the next line is too late — the null member call has already happened inside .getObject().
Why existing code doesn't prevent it
The only exception check in this region is NAPI_RETURN_IF_EXCEPTION(env) at line 1823, which runs before the filtering loop. Inside the loop, when current_object is a Proxy:
getOwnPropertyDescriptorinvokes the Proxy'sgetOwnPropertyDescriptortrap; if it throws, the call returnsfalsewith a pending exception, so thewhilebody executes.getPrototypethen either short-circuits on the pending exception or invokes the Proxy'sgetPrototypeOftrap (which can also throw), and in either case returns an emptyJSValue.- On JSVALUE64 an empty
JSValueis encoded as0, soisCell()is true (0 & NotCellMask == 0) andasCell()isnullptr.JSValue::getObject()therefore callsasCell()->getObject()on a null cell — the same UBSAN null-pointer-member-call this PR fixes inbindings.cpp.
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({}, { getPrototypeOf() { throw 0 } }). key_filter & filter_by_any_descriptoris non-zero (line 1827) → enters the per-key filtering loop.key_mode == napi_key_include_prototypes→ enters the prototype-climbwhileat line 1836.- The walk reaches the Proxy as
current_object.getOwnPropertyDescriptorreturnsfalse(key not on the Proxy target) → loop body runs. current_object->getPrototype(globalObject)invokes the ProxygetPrototypeOftrap, which throws → returns emptyJSValue..getObject()on the empty value:isCell()→ true,asCell()→nullptr,nullptr->getObject()readsm_type→ null-pointer member call (same fingerprintf20677b52d4735c2crash class).
The same outcome occurs if the Proxy instead defines a throwing getOwnPropertyDescriptor trap: the trap throws, getOwnPropertyDescriptor returns false with a pending exception, and getPrototype() returns empty because of the pending exception.
Impact & fix
Impact is narrower than the Bun.inspect case (requires a native addon calling napi_get_all_property_names with napi_key_include_prototypes plus an enumerable/writable/configurable filter on user-controlled objects), but it is the same crash class. The fix is identical to what this PR applies in bindings.cpp: capture getPrototype() into a local JSValue, RETURN_/CLEAR_IF_EXCEPTION, and bail if it's empty before calling .getObject() (and ideally also check for an exception after getOwnPropertyDescriptor).
Scope
This is pre-existing — this PR does not touch napi.cpp, add callers to it, or change its trigger surface. The author explicitly noted in the timeline that #29642 bundled both the forEachPropertyImpl fix and the napi.cpp fix, and that this PR is the "minimal, targeted version" with "Happy to close whichever is redundant once one lands." Flagging only so the napi.cpp half isn't lost if #29642 is closed in favor of this PR. Should not block merge.
|
Closing as a duplicate of #30099, which fixes the same two problems in
#29814, #29845, #30099, #30200, and #30457 all make the same two changes. #30099 additionally clears the exception in the fast-path prototype pre-scan, a third site the others miss, so it's the most complete of the set. |
1 participant
Found by the fuzzer. Fingerprint:
f20677b52d4735c2What
JSC__JSValue__forEachPropertyImpl(used byBun.inspect/console.logto enumerate properties) walks the prototype chain with:iterating = iterating->getPrototype(globalObject).getObject();When
iteratingis a Proxy and either itsgetPrototypeOftrap throws, or a pending exception exists from the property loop above (a Proxygettrap can causegetPropertySlotto returnfalsewith a pending exception, which was only cleared on thetruepath),getPrototype()returns an emptyJSValue.JSValue::getObject()on an empty value callsasCell()->getObject()whereasCell()isnullptr, triggering a null-pointer member call.Repro
Fix
getPropertySlotregardless of return value, so pending exceptions don't leak into later iterations.getPrototype, clear exceptions and break if the result is empty before callinggetObject(). This matches how the fast path in the same function already handlesgetPrototype.