Fix null deref in Bun.inspect when Proxy prototype trap throws#30225
Fix null deref in Bun.inspect when Proxy prototype trap throws#30225robobun wants to merge 1 commit into
Conversation
JSC__JSValue__forEachPropertyImpl walks the prototype chain to enumerate properties for formatting. When the prototype is a Proxy: - If the Proxy 'get' trap throws (or delegates to a getter that throws), getPropertySlot returns false with a pending exception. The CLEAR_IF_EXCEPTION was placed after the 'continue', so the exception was never cleared and leaked into later operations. - iterating->getPrototype(globalObject).getObject() was called without an exception check. A throwing 'getPrototypeOf' trap (or any pending exception) makes getPrototype return an empty JSValue; calling .getObject() on that is a null JSCell dereference. Clear the exception before deciding to skip the property, and split the getPrototype call so we can clear the exception and bail on an empty result. Fuzzilli fingerprint: 0decaebd1bf2d774
WalkthroughThe PR fixes exception-handling issues in property enumeration and prototype-chain traversal within the Bun JavaScript bindings. Exception states are now explicitly cleared during iteration over object properties and prototype chains. Test coverage is enhanced with parameterized subprocess-based tests that verify proxy-related prototype trap scenarios exit cleanly. ChangesProxy Exception Handling & Test Coverage
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 1/5 review remaining, refill in 45 minutes and 40 seconds. 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 506-509: The test currently asserts stderr is exactly empty which
flakes under ASAN; update the assertion to filter out lines beginning with
"WARNING: ASAN interferes" from the proc.stderr.text() result before asserting
emptiness. Concretely, after awaiting proc.stderr.text() (variable stderr) split
into lines, remove any lines that startWith "WARNING: ASAN interferes", rejoin
and then assert the filtered stderr is "". Keep the existing checks for stdout
("OK\n") and exitCode (0) unchanged.
🪄 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: 79953745-e8a8-4a47-9c53-de4d187953a4
📒 Files selected for processing (2)
src/bun.js/bindings/bindings.cpptest/js/bun/util/inspect.test.js
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(stdout).toBe("OK\n"); | ||
| expect(exitCode).toBe(0); |
There was a problem hiding this comment.
Filter known ASAN startup noise before asserting empty stderr.
With bunEnv, ASAN can emit a known warning line on stderr; asserting stderr === "" can make this flaky on ASAN CI even when behavior is correct.
Suggested patch
- expect(stderr).toBe("");
+ const stderrLines = stderr
+ .split("\n")
+ .filter(line => !line.startsWith("WARNING: ASAN interferes"))
+ .filter(Boolean);
+ expect(stderrLines).toEqual([]);Based on learnings: In oven-sh/bun test files that spawn subprocesses using bunEnv, suppress known ASAN startup noise by filtering lines starting with "WARNING: ASAN interferes" before asserting stderr is empty.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/util/inspect.test.js` around lines 506 - 509, The test currently
asserts stderr is exactly empty which flakes under ASAN; update the assertion to
filter out lines beginning with "WARNING: ASAN interferes" from the
proc.stderr.text() result before asserting emptiness. Concretely, after awaiting
proc.stderr.text() (variable stderr) split into lines, remove any lines that
startWith "WARNING: ASAN interferes", rejoin and then assert the filtered stderr
is "". Keep the existing checks for stdout ("OK\n") and exitCode (0) unchanged.
|
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 — focused null-deref fix with regression tests; the napi.cpp note is pre-existing and can be a follow-up.
Extended reasoning...
Overview
This PR fixes a segfault in JSC__JSValue__forEachPropertyImpl (used by Bun.inspect / console.log) when walking the prototype chain of an object whose prototype is a Proxy with throwing traps. Two changes in bindings.cpp:
- Reorder
CLEAR_IF_EXCEPTIONbefore thecontinueaftergetPropertySlot, so a throwinggettrap doesn't leave a pending exception that poisons subsequent JSC calls. - Split
iterating->getPrototype(globalObject).getObject()into separate steps with an exception clear and an empty-value check, so a throwinggetPrototypeOftrap (which makesgetPrototypereturn an emptyJSValue) doesn't dereference a nullJSCell.
Three subprocess-based regression tests are added in inspect.test.js covering each crash variant.
Security risks
None introduced. This hardens the inspector against adversarial Proxy traps — the change strictly reduces crash surface. No new inputs, no auth/crypto/permissions code touched.
Level of scrutiny
Low-to-moderate. The C++ change is ~10 lines, mechanical, and follows the existing CLEAR_IF_EXCEPTION(scope) pattern already used throughout this function. The control-flow change is easy to verify locally: the empty-JSValue check (if (!nextProto) break;) handles the throwing-trap case, and nextProto.getObject() returning nullptr for a non-object (e.g. null prototype) is already handled by the loop's iterating != nullptr condition, preserving prior behavior.
Other factors
- Found via Fuzzilli; PR includes the fingerprint and a clear repro.
- Regression tests spawn a child
bun -eand assert clean exit +OKoutput, so they actually exercise the crash path rather than relying on in-process behavior. - The only finding from review is a pre-existing instance of the same pattern in
napi.cpp(napi_get_all_property_names), which is out of scope here and noted as a follow-up. - No prior reviewer comments to address.
| JSValue nextProto = iterating->getPrototype(globalObject); | ||
| // Ignore exceptions from Proxy "getPrototypeOf" trap. | ||
| CLEAR_IF_EXCEPTION(scope); | ||
| if (!nextProto) | ||
| break; | ||
| iterating = nextProto.getObject(); |
There was a problem hiding this comment.
🟣 Heads-up (pre-existing, not introduced here): the same getPrototype(globalObject).getObject() pattern without an exception check still exists at src/bun.js/bindings/napi.cpp:1837 in napi_get_all_property_names. A throwing Proxy getPrototypeOf/getOwnPropertyDescriptor trap on the prototype chain will cause the same null deref there when called with napi_key_include_prototypes + a descriptor filter — might be worth a follow-up since it's the identical bug class.
Extended reasoning...
What the bug is
This PR fixes a null deref in JSC__JSValue__forEachPropertyImpl where iterating->getPrototype(globalObject).getObject() was called without checking for a pending exception. The exact same vulnerable pattern still exists in napi_get_all_property_names at src/bun.js/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;
}There is no exception check between getPrototype(globalObject) and .getObject(), and the preceding getOwnPropertyDescriptor (also a Proxy trap) is similarly unchecked.
Code path that triggers it
A native addon calls napi_get_all_property_names with:
key_mode = napi_key_include_prototypeskey_filtercontaining any ofnapi_key_enumerable | napi_key_writable | napi_key_configurable(so the descriptor-filtering branch at line 1827 runs)- An object whose prototype chain contains a
Proxywith a throwinggetPrototypeOftrap, or a throwinggetOwnPropertyDescriptortrap.
The earlier allPropertyKeys call (line 1818) goes through the Proxy ownKeys/getPrototypeOf traps and is guarded by NAPI_RETURN_IF_EXCEPTION at line 1823. However, the inner loop at line 1836 invokes getOwnPropertyDescriptor — a different trap that allPropertyKeys does not exercise — so a Proxy whose ownKeys/getPrototypeOf succeed but whose getOwnPropertyDescriptor throws will sail past line 1823 and blow up here.
Why the existing guard doesn't help
When a Proxy trap throws (or there is already a pending exception), JSObject::getPrototype / ProxyObject::getPrototype returns an empty JSValue. On JSVALUE64, an empty JSValue is encoded as 0, which passes the isCell() check (0 & NotCellMask == 0). getObject() therefore evaluates asCell()->isObject() on a null JSCell* and segfaults before reaching the if (!proto) break; on line 1838 — that null check never gets a chance to run.
Step-by-step proof
- JS sets up
objwhose prototype isnew Proxy(target, { getOwnPropertyDescriptor() { throw new Error('nope'); } })andtargethas a keyfoo. - Native addon calls
napi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_enumerable, napi_key_numbers_to_strings, &result). allPropertyKeyswalks the chain viaownKeys/getPrototypeOf(no throw), returns['foo'].NAPI_RETURN_IF_EXCEPTIONpasses.- Filter loop runs for
'foo'.object->getOwnPropertyDescriptoronobjitself returns false (own miss), enters loop body. obj->getPrototype(globalObject)returns the Proxy;.getObject()returns the Proxy object;current_object = proxy.- Next iteration:
proxy->getOwnPropertyDescriptor(...)invokes the trap, which throws. Returns false → loop body again with a pending exception. proxy->getPrototype(globalObject)runsProxyObject::getPrototype, which begins withRETURN_IF_EXCEPTION(scope, { })→ returns emptyJSValue..getObject()on empty →asCell()isnullptr→nullptr->isObject()→ SIGSEGV.
(The simpler variant — a Proxy whose getPrototypeOf throws only on the second call — also reaches step 7 directly.)
Impact
Same as the bug this PR fixes: a hard crash (segfault) of the Bun process, but gated behind a native addon calling this specific N-API with prototype-walking + descriptor filtering on adversarial input. Less reachable than the Bun.inspect case, but still the same crash class.
Fix
Mirror what this PR does in bindings.cpp:
JSValue protoVal = current_object->getPrototype(globalObject);
NAPI_RETURN_IF_EXCEPTION(env); // or break + report
JSObject* proto = protoVal.getObject();
if (!proto) break;and add an exception check after getOwnPropertyDescriptor in the loop condition. This is pre-existing and untouched by this PR — flagging only because it's the identical bug class and a natural follow-up.
1 participant
What does this PR do?
Fixes a null pointer dereference in
Bun.inspect()/console.log()when formatting an object whose prototype is aProxywith throwing traps.JSC__JSValue__forEachPropertyImplwalks the prototype chain to enumerate properties for formatting. When the prototype is aProxy:If the Proxy
gettrap throws (or delegates to a getter that throws),getPropertySlotreturnsfalsewith a pending exception. TheCLEAR_IF_EXCEPTIONwas placed after thecontinue, so the exception was never cleared and leaked into later operations.iterating->getPrototype(globalObject).getObject()was called without an exception check. A throwinggetPrototypeOftrap (or any pending exception) makesgetPrototypereturn an emptyJSValue; calling.getObject()on that dereferences a nullJSCell.Repro
Also reproducible via
expect():How did you verify your code works?
Added regression tests in
test/js/bun/util/inspect.test.jscovering all three crash variants (throwinggettrap, throwinggetPrototypeOftrap, stateful getter that throws on second access). Verified they segfault on the baked release build and pass on the debug build with this fix.Fuzzilli fingerprint:
0decaebd1bf2d774