Fix null deref in Bun.inspect when Proxy getPrototypeOf throws#29071
Fix null deref in Bun.inspect when Proxy getPrototypeOf throws#29071
Conversation
In JSC__JSValue__forEachPropertyImpl, when walking the prototype chain through a ProxyObject, getPrototype() can return an empty JSValue if the getPrototypeOf trap throws or if there is a pending exception from a previous Proxy trap. Calling .getObject() on an empty JSValue calls asCell()->getObject() on a null pointer. Also clear exceptions from getPropertySlot before the early continue so that a throwing Proxy trap that returns false does not leave a pending exception for subsequent iterations or for getPrototype().
|
Updated 3:40 AM PT - Apr 9th, 2026
❌ @autofix-ci[bot], your commit 59f144f has 3 failures in
Add 🧪 To try this PR locally: bunx bun-pr 29071That installs a local version of the PR into your bun-29071 --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)
WalkthroughModified property enumeration logic in C++ bindings to store and check Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 — clean null-deref fix with regression tests, consistent with existing exception-handling patterns in the function.
Extended reasoning...
Overview
Two-line fix in JSC__JSValue__forEachPropertyImpl (bindings.cpp) plus a new regression test file. The change fixes a null pointer dereference when walking prototype chains through Proxy objects whose getPrototypeOf trap throws.
Security Risks
None. The fix makes the code more robust by clearing exceptions and guarding a pointer dereference — it reduces crash surface rather than opening any new attack vectors.
Level of Scrutiny
Low. The changes are surgical, well-scoped, and follow the exact same CLEAR_IF_EXCEPTION pattern already used multiple times in the same function. The root cause is clearly described and the fix directly addresses it.
Other Factors
No bugs were reported by the automated hunting system. Regression tests covering both the throwing-trap case and the fuzzer-discovered case are included. The commit is already on main.
| } | ||
|
|
||
| JSC::PropertySlot slot(object, PropertySlot::InternalMethodType::Get); | ||
| if (!object->getPropertySlot(globalObject, property, slot)) | ||
| continue; | ||
| bool hasProperty = object->getPropertySlot(globalObject, property, slot); | ||
| // Ignore exceptions from "Get" proxy traps. | ||
| CLEAR_IF_EXCEPTION(scope); | ||
| if (!hasProperty) | ||
| continue; | ||
|
|
||
| if ((slot.attributes() & PropertyAttribute::DontEnum) != 0) { | ||
| if (property == propertyNames->underscoreProto |
There was a problem hiding this comment.
🟣 This is a pre-existing issue: the fast-path initial setup (~line 5170, not in this diff) calls object->getPrototype(globalObject) with no CLEAR_IF_EXCEPTION afterward, leaving a stale exception in scope when the prototype is a Proxy with a throwing getPrototypeOf trap. The PR fixes the slow-path loop termination and the getPropertySlot site but misses this initialization site, so the stale exception causes RETURN_IF_EXCEPTION in the slow path (~line 5276) to fire immediately, silently aborting all property enumeration before any properties are visited. The regression tests only assert that Bun.inspect() does not throw, not that it produces complete output, so this silent-abort path is not caught.
Extended reasoning...
Bug: Missing CLEAR_IF_EXCEPTION after fast-path initial getPrototype() silently aborts property enumeration
What the bug is and how it manifests
Inside JSC__JSValue__forEachPropertyImpl, there is a fast-path initialization block (around line 5170, before the diff's hunk) that tests whether the object has any own properties. When none are found (outOfLineSize == 0 && inlineSize == 0), it attempts to advance to the prototype via object->getPrototype(globalObject). There is no CLEAR_IF_EXCEPTION after this call. If the object's direct prototype is a Proxy with a throwing getPrototypeOf trap, getPrototype() returns an empty JSValue with the exception still pending in scope. The if (JSValue proto = ...) condition evaluates as falsy so the body is skipped, but the exception remains live.
The specific code path that triggers it
At ~line 5170 (fast-path init, NOT in this diff):
if (JSValue proto = object->getPrototype(globalObject)) {
if ((structure = proto.structureOrNull())) { ... }
}
// No CLEAR_IF_EXCEPTION here -- exception left pending\!
// fast = false is set -> falls through to slow path at ~line 5263
Then in the slow path at ~line 5276:
object->getOwnNonIndexPropertyNames(...);
RETURN_IF_EXCEPTION(scope, void()); // fires from the stale line-5170 exception\!
Why existing code does not prevent it
The PR correctly adds CLEAR_IF_EXCEPTION in two places: (1) the slow-path loop's getPrototype() call (~line 5365), and (2) the getPropertySlot call before continue (~line 5293). The fast-path loop's getPrototype() call (~line 5247-5257) already had it. But the fast-path initialization at ~line 5170 was not addressed -- this is the gap.
Concrete step-by-step proof
- Create an object with no own properties whose prototype is a Proxy with a throwing getPrototypeOf trap.
- Call Bun.inspect(obj).
- forEachPropertyImpl enters fast-path init. inlineSize == 0, outOfLineSize == 0 -> tries the prototype.
- object->getPrototype(globalObject) on the Proxy invokes the trap, which throws. Returns empty JSValue. Exception is now pending in scope.
- No CLEAR_IF_EXCEPTION. fast = false. prototypeObject stays as the original object.
- Fast-path block is skipped. Slow path begins at ~line 5263.
- getOwnNonIndexPropertyNames is called with a stale exception pending.
- RETURN_IF_EXCEPTION(scope, void()) fires, returning early. All property enumeration is silently aborted.
- Bun.inspect() does not throw (the exception was consumed by RETURN_IF_EXCEPTION), so expect(() => Bun.inspect(obj)).not.toThrow() passes -- but the output is missing all properties.
How to fix it
Add CLEAR_IF_EXCEPTION(scope) immediately after the fast-path initialization getPrototype() call (~line 5170), consistent with the pattern already used in the fast-path loop (~line 5257) and now in the slow-path loop (~line 5368).
There was a problem hiding this comment.
This block is only entered when fast is true, which requires canPerformFastPropertyEnumerationForIterationBun(object->structure()) — a ProxyObject fails that check (overridesGetOwnPropertySlot), so object itself is never a Proxy here.
object->getPrototype(globalObject) on an ordinary object is a plain internal [[Prototype]] slot read and does not invoke any trap on the returned prototype; it just returns the Proxy reference. The getPrototypeOf trap only fires when getPrototype() is called on the Proxy, which happens later in the slow-path loop (now guarded by this PR). So line 5170 cannot throw in the described scenario.
| break; | ||
| if (iterating == globalObject) | ||
| break; | ||
| iterating = iterating->getPrototype(globalObject).getObject(); | ||
| 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.
🟣 This PR fixes the getPrototype().getObject() null-deref in bindings.cpp but the identical unguarded pattern exists in napi.cpp at line 1837 inside napi_get_all_property_names — a pre-existing bug with the same root cause. When a Proxy getPrototypeOf trap throws, getPrototype() returns an empty JSValue, and calling getObject() on it crashes inside getObject() before the if (!proto) check can run.
Extended reasoning...
Root cause (same as this PR)
This PR correctly documents that an empty JSValue has isCell() == true but asCell() == nullptr. When getPrototype() is called on a ProxyObject whose getPrototypeOf trap throws, it returns an empty JSValue. Then .getObject() calls asCell()->isObject(), which dereferences a null pointer — a crash — before returning null.
The identical pattern in napi.cpp
At napi.cpp:1837, inside napi_get_all_property_names when key_mode == napi_key_include_prototypes:
0 ""
0 ""
0 ""
1 "/usr/include/stdc-predef.h" 1 3 4
0 "" 2
1 ""
The if (!proto) null check runs after getObject() returns. But when getPrototype() throws, the crash happens inside getObject() (at asCell()->isObject()), never reaching the null check.
Trigger conditions
Call napi_get_all_property_names with key_mode = napi_key_include_prototypes and descriptor filters (napi_key_enumerable, napi_key_writable, or napi_key_configurable) on an object whose prototype chain includes a Proxy with a throwing getPrototypeOf trap.
Step-by-step proof
- Create a native addon calling napi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_enumerable, ...)
- obj has a prototype that is a new Proxy({}, { getPrototypeOf() { throw new Error(); } })
- The iteration reaches the Proxy in the chain and calls getPrototype(globalObject)
- The getPrototypeOf trap throws → getPrototype() returns an empty JSValue
- .getObject() is called on the empty JSValue → asCell() returns nullptr → nullptr->isObject() → null dereference → crash
Secondary issue: exception not cleared
The exception set by the throwing getPrototypeOf trap is never cleared inside the while loop. On the next iteration, the pending exception will contaminate getOwnPropertyDescriptor and subsequent calls.
Recommended fix (mirrors bindings.cpp fix in this PR)
0 ""
0 ""
0 ""
1 "/usr/include/stdc-predef.h" 1 3 4
0 "" 2
1 ""
There was a problem hiding this comment.
Pre-existing and out of scope for this fuzzer-crash fix — keeping this PR focused on forEachPropertyImpl. #28882 already covers the napi.cpp instance.
|
CI: all 13 The only failures are:
Infra flake on the aarch64 glibc builder; not related to this change. |
1 participant
Fuzzilli fingerprint:
03307a0eaa9cbbc8What does this PR do?
Fixes a null pointer dereference in
JSC__JSValue__forEachPropertyImpl(used byBun.inspect/console.log) when walking the prototype chain through aProxyObject.Root cause
When advancing to the next prototype in the chain:
iterating = iterating->getPrototype(globalObject).getObject();getPrototype()on aProxyObjectcan return an emptyJSValuewhen:getPrototypeOftrap throws, orgettrap in the property loop that was not cleared beforecontinue)An empty
JSValuehasisCell() == truebutasCell() == nullptr, so.getObject()callsnullptr->getObject().Fix
getPrototype(), clear any exception (matching the othergetPrototypecall sites in this function), and only callgetObject()on a non-empty value.getPropertySlotbefore the earlycontinue, so a throwing trap that also returnsfalsedoesn't leave a pending exception lingering into the next iteration / intogetPrototype().How did you verify your code works?
Minimal repros that crashed before and now exit cleanly:
Added regression tests in
test/js/bun/util/inspect-proxy-prototype.test.js. Existingtest/js/bun/util/inspect.test.jsstill passes.