Fix null deref in Bun.inspect when Proxy prototype throws - #30200
Fix null deref in Bun.inspect when Proxy prototype throws#30200robobun wants to merge 5 commits into
Conversation
|
Updated 6:05 AM PT - May 5th, 2026
❌ @robobun, your commit 216d8b3 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30200That installs a local version of the PR into your bun-30200 --bun |
WalkthroughAdjusts slow-path property enumeration in ChangesProxy Property Enumeration Exception Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 4/5 reviews remaining, refill in 12 minutes. 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 808-819: Test currently returns null from the revocable proxy's
getPrototypeOf, so it exercises the null-prototype path instead of the
revoked-proxy throw path; update the getPrototypeOf trap in the test (the
Proxy.revocable handler used in this case) to revoke() and then throw (e.g.,
throw new TypeError(...)) so Bun.inspect(obj) runs the revoked-proxy throw path
while keeping the same expect(...).not.toThrow() assertion.
🪄 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: e63d1aa8-f4e4-4432-9b3d-02083cdc264f
📒 Files selected for processing (2)
src/bun.js/bindings/bindings.cpptest/js/bun/util/inspect.test.js
| it("does not crash when a Proxy is revoked mid-iteration", () => { | ||
| const proto = { foo: 1, bar: 2 }; | ||
| const { proxy, revoke } = Proxy.revocable(proto, { | ||
| getPrototypeOf() { | ||
| revoke(); | ||
| return null; | ||
| }, | ||
| }); | ||
| const obj = {}; | ||
| Object.setPrototypeOf(obj, proxy); | ||
| expect(() => Bun.inspect(obj)).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Revoked-proxy throw path isn’t actually exercised.
Line 811-Line 814 revokes and then returns null, so this test validates a null prototype path, not a revoked-proxy getPrototypeOf throw. That can miss the exact empty-value path this fix targets.
Suggested test adjustment
- it("does not crash when a Proxy is revoked mid-iteration", () => {
+ it("does not crash when prototype is a revoked Proxy", () => {
const proto = { foo: 1, bar: 2 };
- const { proxy, revoke } = Proxy.revocable(proto, {
- getPrototypeOf() {
- revoke();
- return null;
- },
- });
+ const { proxy, revoke } = Proxy.revocable(proto, {});
const obj = {};
Object.setPrototypeOf(obj, proxy);
+ revoke();
expect(() => Bun.inspect(obj)).not.toThrow();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("does not crash when a Proxy is revoked mid-iteration", () => { | |
| const proto = { foo: 1, bar: 2 }; | |
| const { proxy, revoke } = Proxy.revocable(proto, { | |
| getPrototypeOf() { | |
| revoke(); | |
| return null; | |
| }, | |
| }); | |
| const obj = {}; | |
| Object.setPrototypeOf(obj, proxy); | |
| expect(() => Bun.inspect(obj)).not.toThrow(); | |
| }); | |
| it("does not crash when prototype is a revoked Proxy", () => { | |
| const proto = { foo: 1, bar: 2 }; | |
| const { proxy, revoke } = Proxy.revocable(proto, {}); | |
| const obj = {}; | |
| Object.setPrototypeOf(obj, proxy); | |
| revoke(); | |
| expect(() => Bun.inspect(obj)).not.toThrow(); | |
| }); |
🤖 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 808 - 819, Test currently
returns null from the revocable proxy's getPrototypeOf, so it exercises the
null-prototype path instead of the revoked-proxy throw path; update the
getPrototypeOf trap in the test (the Proxy.revocable handler used in this case)
to revoke() and then throw (e.g., throw new TypeError(...)) so Bun.inspect(obj)
runs the revoked-proxy throw path while keeping the same
expect(...).not.toThrow() assertion.
There was a problem hiding this comment.
Removed this test case — tests 1 and 2 already cover both fixed code paths and both segfault on main. The revoked-proxy variation either duplicates test 2 (if the trap throws) or throws cleanly from an unrelated path before ever reaching forEachProperty (if revoked via a get trap during the nodejs.util.inspect.custom lookup).
There was a problem hiding this comment.
@robobun, that makes sense — the early-throw-from-get-trap path during the nodejs.util.inspect.custom / Symbol.toPrimitive lookup would bypass forEachProperty entirely, so the revoked-proxy case would never exercise the guarded prototype-chain traversal. Tests 1 and 2 are the right targeted regressions for the two fixes. Thanks for the clear explanation!
🧠 Learnings used
Learnt from: robobun
Repo: oven-sh/bun PR: 27056
File: test/bundler/standalone.test.ts:281-324
Timestamp: 2026-02-16T04:26:25.185Z
Learning: In Bun test files that exercise Bun.build(), assertions for configuration-validation errors thrown synchronously by JSBundler.fromJS() (via globalThis.throwInvalidArguments()) should use toThrow, e.g., expect(() => Bun.build({...})).toThrow()). Do not use rejects.toThrow() since rejections occur only for asynchronous build errors.
Learnt from: robobun
Repo: oven-sh/bun PR: 28425
File: test/regression/issue/28422.test.ts:65-79
Timestamp: 2026-03-22T10:12:05.719Z
Learning: In oven-sh/bun test files matching test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}, follow CLAUDE.md by asserting the command exit code LAST—after all other assertions such as stdout/stderr checks and filesystem validation. Do not assert exitCode earlier than those checks. Also, avoid asserting stdout for commands like bun install whose output can vary between runs.
Learnt from: robobun
Repo: oven-sh/bun PR: 29441
File: test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts:10-14
Timestamp: 2026-04-18T10:36:45.033Z
Learning: In oven-sh/bun test files that spawn subprocesses using `bunEnv`, suppress the known ASAN startup noise in the subprocess stderr before asserting it is empty. Use the repo’s established convention: split stderr into lines and filter with `.filter(line => !line.startsWith("WARNING: ASAN interferes"))`, then assert the remaining stderr lines are empty. Do not switch to an alternative like `str.replace(...)`; the filter-based approach is the repo convention. This is safe because `ZigGlobalObject.cpp` emits that warning via `std::call_once`, so at most one matching line appears per process.
Learnt from: robobun
Repo: oven-sh/bun PR: 29359
File: test/js/node/test/parallel/test-macos-app-sandbox.js:57-61
Timestamp: 2026-04-20T21:14:19.191Z
Learning: In Node.js inline eval scripts executed via `node -e` / `node --eval` (e.g., child-process scripts that embed code passed to `--eval`), core modules like `fs`, `path`, `os`, `assert`, etc. are available as implicit globals via Node’s `evalScript`/`createGlobalRequire` behavior. When reviewing code that targets `node -e`/`--eval` inline script content, do not report “undefined variable” issues for calls such as `fs.readdirSync(...)`, `path.join(...)`, or other core-module APIs used without explicit `require(...)` within that inline script.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 29581
File: src/bun.js/modules/NodeModuleModule.cpp:663-681
Timestamp: 2026-04-22T20:47:10.896Z
Learning: In oven-sh/bun code reviews, do not recommend adding standalone regression tests that depend on setting `BUN_JSC_validateExceptionChecks=1` to exercise JSC throw-scope/exception-scope validator paths (e.g., PropertyCallback/reify interactions like `reifyAllStaticProperties`). Per `CLAUDE.md`, tests are expected to pass with `USE_SYSTEM_BUN=1`, and `BUN_JSC_validateExceptionChecks` is a no-op on release/system Bun builds. Instead, treat this class of validator coverage issue as covered by: (1) the x64-asan CI shard that enables the validator automatically, and (2) the `test/no-validate-exceptions.txt` opt-out list for tests that hit pre-existing throw-scope assertion failures unrelated to the change under review. If helpful, add an in-source comment pointing to the specific existing exerciser (e.g., the relevant `tsgo/bun-types` test) to document the intent without relying on the env var.
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
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 777-792: The test creates obj via Object.create(proto) and then
immediately replaces its prototype with a Proxy, which is redundant; simplify by
creating a plain object (const obj = {}) and then set its prototype to the Proxy
of proto (use Object.setPrototypeOf(obj, new Proxy(proto, {}))). Update the test
"does not crash when a getter throws through a Proxy prototype" to use proto and
obj as described so the behavior is unchanged but the setup is clearer.
🪄 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: 6dc720cc-8fd3-4224-91fc-ccc1d56d9c41
📒 Files selected for processing (1)
test/js/bun/util/inspect.test.js
| it("does not crash when a getter throws through a Proxy prototype", () => { | ||
| const proto = {}; | ||
| Object.defineProperty(proto, "thrower", { | ||
| get() { | ||
| throw new Error("getter threw"); | ||
| }, | ||
| enumerable: true, | ||
| configurable: true, | ||
| }); | ||
| proto.foo = function () {}; | ||
| proto.bar = function () {}; | ||
|
|
||
| const obj = Object.create(proto); | ||
| Object.setPrototypeOf(obj, new Proxy(proto, {})); | ||
| expect(Bun.inspect(obj)).toContain("foo"); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Optional: Simplify test setup for clarity.
Line 789 creates obj with proto as its prototype, but line 790 immediately replaces that prototype with a Proxy wrapping the same proto. The initial Object.create(proto) is redundant. Consider simplifying to const obj = {}; to match test 2's clearer pattern.
♻️ Suggested simplification
- const obj = Object.create(proto);
- Object.setPrototypeOf(obj, new Proxy(proto, {}));
+ const obj = Object.create(new Proxy(proto, {}));or
- const obj = Object.create(proto);
+ const obj = {};
Object.setPrototypeOf(obj, new Proxy(proto, {}));🤖 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 777 - 792, The test creates
obj via Object.create(proto) and then immediately replaces its prototype with a
Proxy, which is redundant; simplify by creating a plain object (const obj = {})
and then set its prototype to the Proxy of proto (use Object.setPrototypeOf(obj,
new Proxy(proto, {}))). Update the test "does not crash when a getter throws
through a Proxy prototype" to use proto and obj as described so the behavior is
unchanged but the setup is clearer.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/bun/util/inspect.test.js:808-819— This third test doesn't actually exercise either code change in this PR —revoke()runs inside thegetPrototypeOftrap, but the spec's handler-null check and target capture both happen before the trap is invoked, sogetPrototype()returnsjsNull()with no exception (which the pre-PR.getObject()already handled safely). It would pass onmainas-is. To hit the revoked-proxy → emptyJSValuepath, revoke beforegetPrototypeis reached — e.g. revoke inside agettrap during property iteration with nogetPrototypeOftrap defined. (Tests 1 and 2 do cover the fix, so this is just a misleading extra case.)Extended reasoning...
What the test is intended to cover
The PR description says
getPrototype()on a Proxy returns an emptyJSValuewhen the proxy is revoked, and that calling.getObject()on an empty value passes theisCell()check (since0 & NotCellMask == 0) and dereferences a nullJSCell*. This test sets up a revocable proxy whosegetPrototypeOftrap callsrevoke()and returnsnull, presumably to drive that path.Why the trap never produces an empty value or pending exception
Per ECMA-262 §10.5.1 (Proxy
[[GetPrototypeOf]]), JSC'sProxyObject::performGetPrototypedoes the following before calling the trap:- Loads the handler and checks it for
null(the "revoked" check). At this point the proxy is still live, so this passes. - Captures
targetinto a local.
Only then does it call the trap. Inside the trap,
revoke()nulls out the proxy's handler/target slots, but the localtargetreference is already captured, and the trap returnsnull— a valid prototype value. The post-trap invariant check callsIsExtensibleon the capturedtarget(which is{ foo: 1, bar: 2 }, extensible), so step 9 returnsnulldirectly. No exception is thrown and the return value isjsNull(), not an emptyJSValue.Why
jsNull()was already safe pre-PRThe pre-PR code at
bindings.cpp:5447wasiterating->getPrototype(globalObject).getObject().jsNull()hasOtherTagset, soisCell()isfalseand.getObject()returnsnullptr— the loop exits cleanly. The crash only happens for an emptyJSValue(raw bits0), where0 & NotCellMask == 0makesisCell()spuriously true.Step-by-step trace through
forEachPropertyImplobj = {}enters the slow path (its prototype is aProxyObject, which failscanPerformFastPropertyEnumerationForIterationBun).- Iteration 1 (
iterating = obj): no own properties.obj->getPrototype()is the ordinary[[GetPrototypeOf]]→ returns the proxy. No trap fires. - Iteration 2 (
iterating = proxy):getOwnPropertyNamesforwards to the target (noownKeystrap, not yet revoked) →[foo, bar]. For each,object->getPropertySlotwalks to the proxy andperformGetforwards to the target (nogettrap) → returns1/2, no exception, so the newCLEAR_IF_EXCEPTIONbeforecontinuesees nothing. Thenproxy->getPrototype(globalObject)runs the trap as analyzed above → returnsjsNull(), no exception.protois truthy,proto.getObject()isnullptr, loop exits. - The proxy is never touched again after the trap returns, so the
revoke()call is effectively dead.
Neither the new
CLEAR_IF_EXCEPTION(scope)before thecontinuenor theproto ? proto.getObject() : nullptrguard observes any state that differs frommain. This test passes onmainwithout the fix.Suggested change
To actually exercise the revoked-proxy → empty
JSValue→ null-deref path, the proxy must already be revoked whengetPrototypeis called on it. One way:const { proxy, revoke } = Proxy.revocable({ foo: 1, bar: 2 }, { get(t, p) { revoke(); return t[p]; }, }); const obj = {}; Object.setPrototypeOf(obj, proxy); expect(() => Bun.inspect(obj)).not.toThrow();
Here the
gettrap fires forfooduring property iteration and revokes the proxy; the subsequentproxy->getPrototype()atbindings.cpp:5448hits the revoked-handler check, throws aTypeError, and returns an emptyJSValue— exactly the case the new guard handles.This is a nit: tests 1 and 2 already provide real regression coverage for both code changes (test 1 covers the leaked exception from
getPropertySlot, test 2 covers a throwinggetPrototypeOftrap returning empty). This third test is just misleading about what it covers. - Loads the handler and checks it for
| 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 getPrototype(globalObject).getObject() pattern still exists in napi_get_all_property_names at src/bun.js/bindings/napi.cpp:1836-1842. A native addon enumerating with napi_key_include_prototypes + a writable/enumerable/configurable filter on an object whose prototype chain has a Proxy with a throwing getOwnPropertyDescriptor/getPrototypeOf trap can hit the identical null deref — might be worth applying the same empty-value + exception guard there in a follow-up.
Extended reasoning...
What the bug is
This PR fixes a null deref in forEachProperty where getPrototype() on a Proxy returns an empty JSValue (not jsNull()) when the trap throws, and .getObject() on an empty value passes isCell() (0 & NotCellMask == 0) and then dereferences a null JSCell*. The exact same pattern exists, untouched and unguarded, in napi_get_all_property_names at src/bun.js/bindings/napi.cpp:1836-1842:
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 RETURN_IF_EXCEPTION / CLEAR_IF_EXCEPTION between getOwnPropertyDescriptor / getPrototype and .getObject(), and no check that the prototype value is non-empty before calling .getObject() on it.
How it manifests
A native addon calls:
napi_get_all_property_names(env, obj, napi_key_include_prototypes,
napi_key_writable /* or enumerable/configurable */,
napi_key_numbers_to_strings, &result);on a JS object whose prototype chain contains a Proxy with hostile traps. Once the per-key filter loop is entered (line 1833), each iteration calls getOwnPropertyDescriptor and then getPrototype on current_object. If either of those throws while current_object is a Proxy, getPrototype() returns an empty JSValue and empty.getObject() segfaults reading m_type from a null JSCell* — the same crash mechanism described in this PR's description.
Why existing code doesn't prevent it
There is a NAPI_RETURN_IF_EXCEPTION at line 1823 after allPropertyKeys, which catches a trivially throwing getPrototypeOf trap (since allPropertyKeys walks the chain once). However:
- The inner loop at 1833-1842 re-walks the chain once per key with no exception checks at all, so a stateful trap that succeeds the first time and throws on a later call slips past line 1823.
- More directly, a Proxy
getOwnPropertyDescriptortrap that throws makes thewhilecondition returnfalsewith a pending exception; the very next line then callsgetPrototype()on the Proxy, andProxyObject::performGetPrototypebails out early with{}because an exception is already pending.
In both cases the empty value reaches .getObject() unchecked.
Step-by-step proof
- JS side:
const target = { foo: 1 }; const proxy = new Proxy(target, { getOwnPropertyDescriptor() { throw new Error("boom"); }, }); const obj = Object.setPrototypeOf({}, proxy); addon.enumerate(obj); // -> napi_get_all_property_names(..., napi_key_include_prototypes, napi_key_writable, ...)
allPropertyKeys(line 1818) walks the chain via[[OwnPropertyKeys]]+[[GetPrototypeOf]]; neither trap is overridden to throw here, so it succeeds and returns["foo"].NAPI_RETURN_IF_EXCEPTIONat 1823 passes.key_filter & filter_by_any_descriptoris true (napi_key_writable), so we enter the per-key loop with key"foo".key_mode == napi_key_include_prototypes, socurrent_object = objand we evaluate thewhilecondition:obj->getOwnPropertyDescriptor(globalObject, "foo", desc)→objhas no own"foo", returnsfalse, no throw.- Loop body:
obj->getPrototype(globalObject)returns the Proxy (no trap, target's proto),.getObject()→proxy.current_object = proxy. whilecondition again:proxy->getOwnPropertyDescriptor(globalObject, "foo", desc)invokes thegetOwnPropertyDescriptortrap, which throws.getOwnPropertyDescriptorreturnsfalsewith the exception left pending inscope.- Loop body:
proxy->getPrototype(globalObject)→ProxyObject::performGetPrototypesees the pending exception (or re-enters JS with it pending) and returnsJSValue()(empty). JSValue().getObject():isCell()checks(0 & NotCellMask) == 0→ true;asCell()returnsnullptr;JSCell::getObject()readsthis->m_type→ null deref / segfault.
(An equivalent path exists with a stateful getPrototypeOf trap that returns null during allPropertyKeys and throws inside the per-key loop.)
Impact
Process crash (SIGSEGV) reachable from a native addon that calls napi_get_all_property_names with napi_key_include_prototypes and any descriptor-filter bit on an attacker-/user-controlled object. Narrower than the Bun.inspect case (requires a native addon and a specific flag combination) but the same crash class.
How to fix
Apply the same guard this PR adds at bindings.cpp:5448-5451:
while (!current_object->getOwnPropertyDescriptor(globalObject, key.toPropertyKey(globalObject), desc)) {
NAPI_RETURN_IF_EXCEPTION(env); // or CLEAR_IF_EXCEPTION + break
JSValue protoVal = current_object->getPrototype(globalObject);
NAPI_RETURN_IF_EXCEPTION(env);
JSObject* proto = protoVal ? protoVal.getObject() : nullptr;
if (!proto) break;
current_object = proto;
}Scope
This is pre-existing — the PR does not touch napi.cpp, add callers, or otherwise interact with napi_get_all_property_names. Flagging it only because it's the identical pattern being fixed here and is a natural follow-up.
There was a problem hiding this comment.
LGTM — minimal, correct null-guard that mirrors the existing fast-path handling; the napi.cpp note I left inline is a pre-existing follow-up, not a blocker.
Extended reasoning...
Overview
This PR touches two files: an ~8-line change in src/bun.js/bindings/bindings.cpp inside JSC__JSValue__forEachPropertyImpl, and two new regression tests in test/js/bun/util/inspect.test.js. The C++ change does two things in the slow-path prototype-walk loop: (1) reorders the getPropertySlot result handling so CLEAR_IF_EXCEPTION(scope) runs before the continue, preventing a pending exception from leaking into the next iteration; and (2) stores getPrototype() into a local, clears any exception, and checks for an empty JSValue before calling .getObject() — exactly mirroring the fast-path handling already present at lines ~5336–5337 of the same function.
Security risks
None. This is purely defensive null/exception handling in the object-inspection path. It does not change auth, crypto, permissions, or any trust boundary. The only behavioral change is that Bun.inspect / console.log no longer segfaults when a Proxy in the prototype chain throws — exceptions are swallowed (consistent with how this function already treats getter/trap exceptions elsewhere) and iteration terminates cleanly.
Level of scrutiny
Low–medium. The change is mechanical and self-evidently correct: storing a return value before checking it, and guarding an empty JSValue before dereference. The while (iterating && …) loop condition (line 5349) already handles iterating == nullptr, so setting it to nullptr on a thrown trap cleanly terminates iteration. Both regression tests directly exercise the two fixed paths and segfault on main per the PR description.
Other factors
- CodeRabbit raised one minor test-redundancy nit (already addressed by removing a third test case) and one trivial style nit on
Object.create(proto)— neither affects correctness. - CI shows uniform
build-zig/build-cppfailures across every platform on commit 7cb6a5c; the identical-everywhere pattern points to transient build-infra rather than this change (the C++ is trivially compilable and present at HEAD in the checkout). Merge remains gated on green CI regardless of this approval. - The duplicate-PR bot flagged several prior attempts at the same fix; that's a process/dedup decision for maintainers and doesn't bear on the correctness of this diff.
- My earlier inline comment about
napi_get_all_property_namesinnapi.cppis a pre-existing sibling issue suggested as a follow-up, not introduced or affected by this PR.
When walking the prototype chain in forEachProperty, two issues could cause a null dereference when a Proxy was present: 1. If getPropertySlot threw (e.g. a getter on the Proxy target threw), it would return false and the `continue` skipped CLEAR_IF_EXCEPTION, leaving the exception pending for the rest of the loop. 2. getPrototype() on a Proxy returns an empty JSValue when it throws (revoked Proxy, throwing getPrototypeOf trap, or a pending exception from #1). Calling .getObject() on an empty JSValue dereferences a null JSCell*. Clear the exception before the `continue`, and guard the empty return from getPrototype() the same way the fast path already does.
Tests 1 and 2 already cover both fixed code paths. The revoked-proxy case either exercises the same path as test 2 (when the trap throws) or an unrelated path that throws cleanly before reaching forEachProperty (when revoked via get trap).
b14ce57 to
671b57b
Compare
There was a problem hiding this comment.
LGTM — minimal, well-targeted crash fix that mirrors the existing fast-path guard pattern, with regression tests for both fixed paths.
Extended reasoning...
Overview
This PR fixes a null pointer dereference in JSC__JSValue__forEachPropertyImpl (used by Bun.inspect / console.log) when an object's prototype chain contains a Proxy whose traps throw. Two changes in src/jsc/bindings/bindings.cpp:
- Reorders
CLEAR_IF_EXCEPTION(scope)to run before thecontinueaftergetPropertySlot, so a pending exception from a throwing Proxygettrap doesn't leak into subsequent loop iterations. - Stores the result of
iterating->getPrototype(globalObject)in a local, clears any exception, and checks for an emptyJSValuebefore calling.getObject()— preventing the nullJSCell*deref when a ProxygetPrototypeOftrap throws.
Two regression tests are added to test/js/bun/util/inspect.test.js that segfault on main and pass with the fix.
Security risks
None. This is purely defensive null/exception handling in the property-enumeration path of Bun.inspect. It removes a crash (DoS surface) rather than introducing one. No new user-controlled data flow, no auth/crypto/permission logic.
Level of scrutiny
Low-to-moderate. The diff is ~10 lines of mechanical change in a single function, and the new pattern is identical to what the fast path in the same function already does at lines 5327 / 5336-5337 (if (JSValue proto = ...getPrototype(...)) followed by CLEAR_IF_EXCEPTION). This is a fuzzer-found crash with a clear repro and a fix that follows established local convention. The function already swallows getter/trap exceptions throughout, so swallowing the getPrototypeOf trap exception here is consistent behavior, not a new design choice.
Other factors
- CodeRabbit's only substantive comment (revoked-proxy test not exercising the right path) was addressed by removing that test; the two remaining tests cover both code-path fixes directly.
- The remaining CodeRabbit nitpick (redundant
Object.create(proto)beforesetPrototypeOf) is explicitly marked trivial/low-value and doesn't affect correctness. - I previously left a heads-up about an identical pre-existing pattern in
napi.cpp— that's a follow-up suggestion, not a blocker for this PR. - The duplicate-PR bot flagged several near-identical fixes; that's a process/triage matter and doesn't affect the correctness of this change.
There was a problem hiding this comment.
LGTM — minimal, well-targeted crash fix with regression tests.
Extended reasoning...
Overview
This PR touches two files: a ~6-line change in src/jsc/bindings/bindings.cpp inside JSC__JSValue__forEachPropertyImpl (the slow-path property enumerator backing Bun.inspect/console.log), and two new regression tests in test/js/bun/util/inspect.test.js. The C++ change does two things: (1) hoists the getPropertySlot result into a local so CLEAR_IF_EXCEPTION runs before the continue, and (2) stores getPrototype() into a local, clears any pending exception, and null-checks before calling .getObject(). Both are purely defensive — no new behavior is introduced beyond not crashing.
Security risks
None. The change strictly narrows behavior from "segfault on hostile/throwing Proxy traps" to "swallow the exception and stop walking the prototype chain," which is exactly what the surrounding code already does for every other JS-observable call in this loop (CLEAR_IF_EXCEPTION / scope.tryClearException() are used throughout). No auth, crypto, permissions, or input parsing is touched.
Level of scrutiny
Low-to-moderate. This is a small, mechanical null-guard + exception-clear in a function that is already designed to be best-effort (it swallows getter exceptions and falls back to jsUndefined() a few lines below). The pattern proto ? proto.getObject() : nullptr is the idiomatic JSC guard for an empty JSValue returned under exception, and the loop condition (while (iterating && ...)) already terminates cleanly on nullptr. The fix matches the PR description's analysis precisely.
Other factors
The two added tests directly exercise both fixed code paths (throwing getter via Proxy → leaked exception; throwing getPrototypeOf trap → empty JSValue) and segfault on main per the description and fuzzer fingerprint. The CodeRabbit nit about Object.create(proto) redundancy is trivial/optional and doesn't affect correctness. The earlier revoked-proxy test case was removed after discussion and CodeRabbit agreed the remaining two tests are sufficient. I previously left an inline note that the same unguarded pattern exists in napi.cpp — that's pre-existing and out of scope here. The duplicate-PR bot flagged several similar PRs, but that's a process question for maintainers, not a correctness concern with this change.
There was a problem hiding this comment.
LGTM — targeted null-deref fix that mirrors the existing fast-path guard, with regression tests that segfault on main.
Extended reasoning...
Overview
This PR fixes a segfault in JSC__JSValue__forEachPropertyImpl (the slow path of Bun.inspect/console.log property enumeration) when an object's prototype chain contains a Proxy that throws. Two changes in src/jsc/bindings/bindings.cpp: (1) move CLEAR_IF_EXCEPTION before the continue after getPropertySlot so a pending exception isn't carried into subsequent iterations, and (2) check getPrototype()'s return for empty before calling .getObject() on it. Two regression tests are added to inspect.test.js.
Security risks
None. This is a defensive crash fix — it removes a null-pointer dereference reachable from user JS via Bun.inspect/console.log. Exceptions are swallowed (CLEAR_IF_EXCEPTION), which is the intentional, established behavior throughout this function (inspect is meant to be robust against throwing getters/traps; see lines 5374, 5421-5424 in the same function).
Level of scrutiny
Low-to-moderate. The change is ~10 lines, mechanical, and directly mirrors the guard pattern already used in the fast path of the same function at lines 5250 and 5327 (if (JSValue proto = ...->getPrototype(globalObject))). The slow path was simply missing the same check. The PR description gives a precise root-cause analysis (empty JSValue passing isCell() because 0 & NotCellMask == 0, then dereferencing a null JSCell*), and both added tests reproduce the crash on main.
Other factors
- Bug hunter found no issues.
- The single CI failure (
test-http-should-emit-close-when-connection-is-aborted.tstimeout on Windows) is unrelated to this change. - CodeRabbit's only remaining comment is an explicitly-labeled trivial/optional nitpick about
Object.create(proto)vs{}in test setup — not blocking, and the test as written still exercises the fix correctly. - I previously left an informational note that the same unguarded pattern exists in
napi_get_all_property_names(napi.cpp); that's pre-existing and out of scope for this PR. - The duplicate-PR bot flagged several prior attempts at this same fix; this one is the cleanest and has passing tests.
|
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
What does this PR do?
Fixes a null pointer dereference in
Bun.inspect/console.logwhen an object's prototype chain contains a Proxy that throws.When walking the prototype chain in
forEachProperty, two issues could cause a crash when a Proxy was present:If
getPropertySlotthrew (e.g. a getter invoked through a Proxy target threw), it returnsfalseand thecontinueskippedCLEAR_IF_EXCEPTION, leaving the exception pending for the rest of the loop.getPrototype()on a Proxy returns an emptyJSValue(notjsNull()) when it throws — revoked Proxy, throwinggetPrototypeOftrap, or a pending exception from (1). Calling.getObject()on an emptyJSValuepasses theisCell()check (since0 & NotCellMask == 0) and then calls a member function on a nullJSCell*.Repro
Or directly:
Fix
continueso it doesn't leak into the next iteration.getPrototype()the same way the fast path already does (check for empty before.getObject()and clear the exception).How did you verify your code works?
Added regression tests to
test/js/bun/util/inspect.test.jsthat segfault on main and pass with this change.Found by fuzzer (fingerprint
e6d9fce2d7afd71d).