fix null deref in forEachProperty prototype walk#28991
Conversation
|
Updated 6:51 AM PT - Apr 8th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 28991That installs a local version of the PR into your bun-28991 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughPrototype-chain iteration in the C++ bindings was made null-safe and clears pending exceptions when advancing to an object's prototype. A regression test was added that spawns a child Bun process, sets an object's prototype to a Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 — minimal, correct null-check matching the existing fast-path pattern, with regression test.
Extended reasoning...
Overview
This PR fixes a null dereference crash in the slow path of JSC__JSValue__forEachPropertyImpl in src/bun.js/bindings/bindings.cpp. A two-file change: one C++ bug fix (3 lines changed) and one regression test added to test/js/bun/util/inspect.test.js.
Security risks
None. This is a crash fix that converts a segfault/UBSAN abort into a graceful exception. No auth, crypto, or permission paths are involved.
Level of scrutiny
Low — the fix is a direct, mechanical application of the pattern already used on the fast path (lines 5121–5131). The guard proto ? proto.getObject() : nullptr plus CLEAR_IF_EXCEPTION(scope) is identical in structure and intent to the surrounding code. The regression test reproduces the exact Fuzzilli-found crash.
Other factors
No prior reviews exist on this PR. The change is self-contained, well-described, and has a clear before/after explanation. The test adequately exercises the three call sites (Bun.inspect, String(), and the toBe error formatting path) where the crash was observed.
robobun
force-pushed
the
farm/094b242e/fix-foreach-proto-empty-jsvalue
branch
2 times, most recently
from
f21bd66 to
7965927
Compare
| break; | ||
| if (iterating == globalObject) | ||
| break; | ||
| iterating = iterating->getPrototype(globalObject).getObject(); | ||
| JSValue proto = iterating->getPrototype(globalObject); | ||
| CLEAR_IF_EXCEPTION(scope); | ||
| iterating = proto ? proto.getObject() : nullptr; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 The PR correctly fixes the slow-path loop (line 5368) and the fast-path loop (line 5257), but the fast-path setup block at lines 5166–5178 has an identical unguarded getPrototype() call. When an object has no own properties (outOfLineSize==0 && inlineSize==0), it sets fast=false and calls object->getPrototype(globalObject) — if a Proxy getPrototypeOf trap throws, the empty JSValue is returned, the if(proto) block is skipped, and the pending exception is never cleared. Execution falls through to the slow path where RETURN_IF_EXCEPTION at line 5276 fires immediately, silently skipping all prototype-chain enumeration. Fix: add CLEAR_IF_EXCEPTION(scope); after line 5176 (closing brace of the if(proto) block in the fast-path setup).
Extended reasoning...
The Bug
The PR addresses a null-dereference crash in JSC__JSValue__forEachPropertyImpl when a Proxy getPrototypeOf trap throws during prototype-chain walking. It correctly patches two sites: the slow-path loop at lines 5367–5369 (the primary crash site) and the fast-path loop at lines 5247–5257 (which already had CLEAR_IF_EXCEPTION). However, there is a third getPrototype call at line 5170 — the fast-path setup block — that still lacks the guard.
The Code Path
Lines 5166–5178 handle the case where the object has a fast structure but zero own properties (outOfLineSize==0 && inlineSize==0). In that case:
fastis set tofalse(line 5168)object->getPrototype(globalObject)is called (line 5170)- If the result is truthy, the prototype's structure is examined and
fastmay be set back totrue - If
getPrototypethrows (e.g., via a Proxy trap), it returns an emptyJSValue; theif(proto)body is never entered — but noCLEAR_IF_EXCEPTIONfollows at line 5176
Why Existing Code Doesn't Catch It
After the fast-path setup block exits at line 5178, fast is false (set at 5168 and never restored), so the if(fast) block at line 5184 is entirely skipped. Execution continues to the slow path at line 5263. The very first RETURN_IF_EXCEPTION(scope, void()) in the slow path (line 5276) sees the stale exception from the Proxy trap and returns the function immediately — before any properties of the original object or its prototype chain are enumerated.
Concrete Proof
Suppose obj has a fast structure, zero own properties, and its prototype is a Proxy whose getPrototypeOf trap throws TypeError: boom:
- Line 5167:
structure->outOfLineSize()==0 && structure->inlineSize()==0→ true - Line 5168:
fast = false - Line 5170:
getPrototype(globalObject)→ Proxy trap fires, throws, returns emptyJSValue - Line 5170:
if(proto)→ false, block skipped, exception left pending inscope - Line 5178: exits fast-path setup;
fastis stillfalse - Line 5184:
if(fast)→ false, skipped entirely - Line 5263: enters slow path;
iterating = prototypeObject.getObject() = obj - Line 5276:
RETURN_IF_EXCEPTION(scope, void())→ fires on the stale Proxy exception → function returns, zero properties reported
This means any object with no own properties whose prototype is a throwing Proxy will appear completely empty to forEachProperty, rather than correctly reporting the error or walking the chain.
Impact
The consequence is silent data loss during property enumeration: properties that exist on the prototype chain are simply not reported, with no error surfaced to the caller. In the formatting context (Bun.inspect, String()), this produces silently truncated output rather than a crash — harder to detect than the original null-dereference but still incorrect behavior.
Fix
Add CLEAR_IF_EXCEPTION(scope); after line 5176 (the closing brace of if(proto)), exactly matching the pattern at lines 5247–5257 (fast-path loop) and lines 5367–5369 (PR fix for slow-path loop). A comment like // Ignore exceptions from Proxy getPrototype trap. would match the existing style.
There was a problem hiding this comment.
The fast-path setup block at lines 5166–5178 still has the unguarded getPrototype() call without a following CLEAR_IF_EXCEPTION. The PR only fixed the slow-path loop. Line 5170 calls object->getPrototype(globalObject) — if a Proxy trap throws, the empty JSValue is returned, if(proto) is skipped, and the pending exception stays in the scope. When fast remains false and execution falls through to the slow path, RETURN_IF_EXCEPTION fires immediately, silently skipping all enumeration.
Fix: add CLEAR_IF_EXCEPTION(scope); after line 5176 (closing brace of the if(proto) block):
if (JSValue proto = object->getPrototype(globalObject)) {
if ((structure = proto.structureOrNull())) {
prototypeObject = proto;
fast = canPerformFastPropertyEnumerationForIterationBun(structure);
prototypeCount = 1;
}
}
CLEAR_IF_EXCEPTION(scope); // <-- add this| JSValue proto = iterating->getPrototype(globalObject); | ||
| CLEAR_IF_EXCEPTION(scope); | ||
| iterating = proto ? proto.getObject() : nullptr; |
There was a problem hiding this comment.
🟣 The same unsafe getPrototype().getObject() pattern this PR fixes in forEachPropertyImpl also exists verbatim at napi.cpp:1837 inside napi_get_all_property_names(). If a Proxy getPrototypeOf trap throws, getPrototype() returns an empty JSValue (all zeros), and calling .getObject() on it dereferences a null cell pointer — the \!proto check at line 1838 never executes because the crash happens inside .getObject() before the result is stored. This is a pre-existing bug of the exact same class, but it was not addressed by this PR.
Extended reasoning...
What the bug is and how it manifests
In napi_get_all_property_names() (napi.cpp:1836–1841), when key_mode == napi_key_include_prototypes and descriptor filtering is requested, the function walks the prototype chain of each key to find the owning object's property descriptor:
JSObject* proto = current_object->getPrototype(globalObject).getObject();
if (\!proto) {
break;
}This is the exact pattern this PR fixes in bindings.cpp. If getPrototype() encounters a Proxy whose getPrototypeOf trap throws, it returns an empty JSValue (all-zero bit pattern). Calling .getObject() on that empty value calls isObject(), which calls isCell() (returns true because all-zero encodes as a cell pointer) then asCell()->isObject() — but asCell() returns nullptr, so this is a null pointer dereference and crashes under ASAN/UBSAN.
The specific code path
napi.cpp:1836–1841 (inside the key_filter & filter_by_any_descriptor branch of napi_get_all_property_names): the while loop calls getPrototype(globalObject) and immediately chains .getObject() without first checking the returned JSValue for truthiness or clearing any pending exception.
Why existing guards don't help
NAPI_PREAMBLE declares a throw scope but the comment in that macro explicitly states: "You should not use this throw scope directly". There is no CLEAR_IF_EXCEPTION call in this loop. The if (\!proto) { break; } guard at line 1838 never executes because the dereference crash occurs inside .getObject() before the result is assigned to proto.
Impact
Any native addon calling napi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_enumerable|...) on an object whose prototype chain contains a Proxy with a throwing getPrototypeOf trap will crash Bun. This is a hard segfault / UBSAN null-member-call, not a catchable JS error.
How to fix
Apply the same pattern this PR uses in bindings.cpp:
JSValue protoValue = current_object->getPrototype(globalObject);
CLEAR_IF_EXCEPTION(scope); // use a local throw scope, not the preamble scope
if (\!protoValue) break;
JSObject* proto = protoValue.getObject();
if (\!proto) break;
current_object = proto;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). objhas been constructed so thatObject.getPrototypeOf(obj)returns a Proxy whosegetPrototypeOftrap is() => { throw new Error('trap\!'); }.allPropertyKeyssucceeds (it has its own prototype walk; the trap may or may not fire there).- Because
key_filter & napi_key_enumerableis truthy, execution enters the descriptor-filtering branch (line 1827). - For each key, the while loop on line 1836 fires.
current_object->getOwnPropertyDescriptorreturns false (the key is inherited). - Line 1837:
current_object->getPrototype(globalObject)fires the Proxy trap, which throws. JSC returns an empty JSValue (all zeros) and sets a pending exception. .getObject()is immediately called on the empty JSValue:isObject()→isCell()returnstrue(zero bits) →asCell()returnsnullptr→nullptr->isObject()— null dereference, process crash.- The
if (\!proto)guard at line 1838 is never reached.
forEachPropertyImpl's slow path called .getObject() directly on the return value of iterating->getPrototype(), which crashes when the call returns an empty JSValue (isCell() returns true, asCell() is null). Use the same pattern as the fast path: check truthiness and clear pending exceptions from the Proxy getPrototypeOf trap.
robobun
force-pushed
the
farm/094b242e/fix-foreach-proto-empty-jsvalue
branch
from
7965927 to
1e11be9
Compare
![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 767-771: Add an assertion to verify the child process exit code is
0: after the existing checks on result.signalCode and stderr, assert that
result.exitCode === 0 so the test verifies the process completed successfully
(the code path where Bun.inspect(v) runs and the thrown toBe error is caught).
Locate the assertions around the variable result in the test (the block with
result.stderr, result.signalCode) and add the exitCode check there to follow the
spawn-process coding guideline.
🪄 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: 2b24eda9-8bee-43a6-be8a-d2224eff4dc1
📥 Commits
Reviewing files that changed from the base of the PR and between 7965927ecd2e0cf9c2cbf471153c3f558c405873 and 1e11be9.
📒 Files selected for processing (2)
src/bun.js/bindings/bindings.cpptest/js/bun/util/inspect.test.js
| const stderr = result.stderr?.toString() ?? ""; | ||
| expect(result.signalCode).toBeUndefined(); | ||
| expect(stderr).not.toContain("Segmentation fault"); | ||
| expect(stderr).not.toContain("runtime error"); | ||
| expect(stderr).not.toContain("panic"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add exitCode check per coding guidelines.
The test correctly checks signalCode and stderr for crash indicators, but per coding guidelines, exit codes should also be checked when spawning processes. Since the script catches the toBe error and Bun.inspect(v) should complete successfully, the process should exit with code 0.
Suggested fix
const stderr = result.stderr?.toString() ?? "";
+ expect(result.exitCode).toBe(0);
expect(result.signalCode).toBeUndefined();
expect(stderr).not.toContain("Segmentation fault");
expect(stderr).not.toContain("runtime error");
expect(stderr).not.toContain("panic");As per coding guidelines: "Always check exit codes and test error scenarios when spawning processes in tests."
📝 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.
| const stderr = result.stderr?.toString() ?? ""; | |
| expect(result.signalCode).toBeUndefined(); | |
| expect(stderr).not.toContain("Segmentation fault"); | |
| expect(stderr).not.toContain("runtime error"); | |
| expect(stderr).not.toContain("panic"); | |
| const stderr = result.stderr?.toString() ?? ""; | |
| expect(result.signalCode).toBeUndefined(); | |
| expect(stderr).not.toContain("Segmentation fault"); | |
| expect(stderr).not.toContain("runtime error"); | |
| expect(stderr).not.toContain("panic"); | |
| expect(result.exitCode).toBe(0); |
🤖 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 767 - 771, Add an assertion to
verify the child process exit code is 0: after the existing checks on
result.signalCode and stderr, assert that result.exitCode === 0 so the test
verifies the process completed successfully (the code path where Bun.inspect(v)
runs and the thrown toBe error is caught). Locate the assertions around the
variable result in the test (the block with result.stderr, result.signalCode)
and add the exitCode check there to follow the spawn-process coding guideline.
|
Gate is rejecting because it can't build the with-fix variants — all four build steps hit network failures fetching WebKit/tinycc ( The C++ fix is a 3-line change matching the pattern already used in the fast-path loop (bindings.cpp:5247-5257), and the test was verified locally on the pre-rebase build. I don't see a code change that can make the gate's infrastructure successfully fetch dependencies. |
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const stderr = result.stderr?.toString() ?? ""; | ||
| expect(result.signalCode).toBeUndefined(); | ||
| expect(stderr).not.toContain("Segmentation fault"); | ||
| expect(stderr).not.toContain("runtime error"); | ||
| expect(stderr).not.toContain("panic"); | ||
| }); |
There was a problem hiding this comment.
🟡 The regression test never asserts expect(result.exitCode).toBe(0), so a non-crash failure (e.g., Bun.inspect(v) throwing an uncaught JS exception and exiting with code 1) would leave signalCode undefined, none of the stderr substring checks would fire, and the test would silently pass despite the child process having failed. Adding expect(result.exitCode).toBe(0) would close this gap.
Extended reasoning...
What the bug is and how it manifests
The new regression test (lines 763–772) spawns a child process and makes four assertions: result.signalCode is undefined, and stderr does not contain "Segmentation fault", "runtime error", or "panic". There is no expect(result.exitCode).toBe(0) call. The child script ends with an unguarded Bun.inspect(v); that has no surrounding try/catch. If that call ever throws an uncaught JS exception (instead of crashing with a signal), the child process exits with code 1 and no signal — all four existing assertions pass, but the child process actually failed.
The specific code path that triggers it
The child script is:
import { expect } from "bun:test";
const v = expect();
Object.setPrototypeOf(v, new Proxy(Object.getPrototypeOf(v), {}));
try { v.toBe(v); } catch {}
Bun.inspect(v);The try/catch around v.toBe(v) prevents the expected assertion error from propagating. But Bun.inspect(v) on the last line is unguarded. Any future regression that makes Bun.inspect throw a JS exception for proxy-prototype objects (rather than crashing with SIGSEGV) would cause the child to exit with code 1 silently.
Why existing assertions do not prevent it
result.signalCodeisundefinedon a normal exit-code-1 (no signal was raised).- The stderr substring checks only look for crash-related strings; a JS exception message like "TypeError: cannot read property of undefined" would not match any of the three patterns.
- Together, all four assertions pass, but the test is a false green.
Addressing the refutation
The refutation correctly observes that for the specific regression this PR fixes (SIGSEGV from null deref in the slow path prototype walk), the signalCode check is the primary and sufficient guard. The fix either prevents the crash (exit 0) or the crash occurs with SIGSEGV. However, the test is intended to serve as a long-lived regression guard. If a future refactoring changes the failure mode from a crash to a JS exception, the signalCode check would no longer catch it. The exitCode assertion costs one line and makes the test robust to both crash and non-crash failure modes.
Impact
False-green tests: a future regression in Bun.inspect that produces a non-signal, non-zero exit would go completely undetected by this test.
How to fix
Add expect(result.exitCode).toBe(0); alongside the signalCode assertion:
expect(result.exitCode).toBe(0);
expect(result.signalCode).toBeUndefined();
expect(stderr).not.toContain("Segmentation fault");
expect(stderr).not.toContain("runtime error");
expect(stderr).not.toContain("panic");Step-by-step proof of the false-green scenario
- A future refactoring causes
Bun.inspect(v)to throwTypeError: Cannot read properties of nullfor objects with Proxy prototypes, instead of crashing with SIGSEGV. - The child process exits with code 1.
signalCodeisundefined(no signal was raised). - The error is printed to stderr as something like:
TypeError: Cannot read properties of null (reading 'foo'). result.signalCodeisundefined— assertion passes.stderrdoes not contain "Segmentation fault" — assertion passes.stderrdoes not contain "runtime error" — assertion passes.stderrdoes not contain "panic" — assertion passes.- Test result: PASS — but the child process failed with exit code 1. The regression goes undetected.
| writeFileSync( | ||
| script, | ||
| `import { expect } from "bun:test"; | ||
| const v = expect(); | ||
| Object.setPrototypeOf(v, new Proxy(Object.getPrototypeOf(v), {})); | ||
| try { v.toBe(v); } catch {} | ||
| Bun.inspect(v); |
There was a problem hiding this comment.
🟡 The new regression test uses tmpdirSync() and writeFileSync() to create a temporary script file, violating root CLAUDE.md line 117 which explicitly prohibits tmpdirSync. Since the spawned script is a short single file, test/CLAUDE.md's guideline of preferring -e over tempDir for single-file tests also applies — using cmd: [bunExe(), '-e', scriptContent] would eliminate the tmpdirSync, writeFileSync, and fs import entirely.
Extended reasoning...
What the bug is
Root CLAUDE.md line 117 states: "Use tempDir from "harness" to create a temporary directory. Do not use tmpdirSync or fs.mkdtempSync to create temporary directories." The new regression test at line 752 calls tmpdirSync() directly, which is the pattern explicitly prohibited by this rule.
The specific code path
The test at test/js/bun/util/inspect.test.js:752-758 does:
const dir = tmpdirSync();
const script = join(dir, "repro.js");
writeFileSync(script, `import { expect } from "bun:test"; ...`);
const result = Bun.spawnSync({ cmd: [bunExe(), script], ... });Why existing code does not prevent it
While tmpdirSync was already imported and used elsewhere in this file (lines 107, 422), that does not justify adding a new violation — each new usage still breaks the stated convention. The rule is clear and unconditional.
The additional test/CLAUDE.md guideline
test/CLAUDE.md lines 62–63 state: "For single-file tests, prefer -e over tempDir." The spawned script is a short 5-line single file with a static import statement. Bun's -e flag supports static imports (confirmed by other tests in the repo using --input-type=module or inline ES module syntax). This makes the current approach more complex than necessary.
Step-by-step proof
- The test calls
const dir = tmpdirSync()— banned by root CLAUDE.md line 117. - The test writes a 5-line script to a temp file via
writeFileSync— unnecessary if using-e. - The same test could be written as:
const scriptContent = `import { expect } from "bun:test";
const v = expect();
Object.setPrototypeOf(v, new Proxy(Object.getPrototypeOf(v), {}));
try { v.toBe(v); } catch {}
Bun.inspect(v);`;
const result = Bun.spawnSync({ cmd: [bunExe(), "--input-type=module", "-e", scriptContent], env: bunEnv, ... });This eliminates: tmpdirSync, writeFileSync, import { writeFileSync } from "fs", and import { join } from "path".
Impact
The test is functionally correct and tests the right crash scenario. This is purely a convention/style violation that adds unnecessary file I/O, temp directory management, and imports to a test that could be a clean inline invocation.
…get_all_property_names Hold the current link in the prototype chain as a JSValue under an EnsureStillAliveScope so a Proxy trap that runs JS (and may GC) during property enumeration can't free it out from under us. Also guard the equivalent getPrototype().getObject() pattern in napi_get_all_property_names, propagating Proxy trap exceptions as napi_pending_exception. Supersedes #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325.
|
Superseded by #29642, which combines the |
…get_all_property_names Hold the current link in the prototype chain as a JSValue under an EnsureStillAliveScope so a Proxy trap that runs JS (and may GC) during property enumeration can't free it out from under us. Also guard the equivalent getPrototype().getObject() pattern in napi_get_all_property_names, propagating Proxy trap exceptions as napi_pending_exception. Supersedes #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325.
…get_all_property_names Hold the current link in the prototype chain as a JSValue under an EnsureStillAliveScope so a Proxy trap that runs JS (and may GC) during property enumeration can't free it out from under us. Also guard the equivalent getPrototype().getObject() pattern in napi_get_all_property_names, propagating Proxy trap exceptions as napi_pending_exception. Supersedes #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325.
1 participant
Fixes a crash found by Fuzzilli (fingerprint
6b65dd404623d6cd).The slow path of
JSC__JSValue__forEachPropertyImplcalled.getObject()directly on the return value ofiterating->getPrototype():iterating = iterating->getPrototype(globalObject).getObject();If
getPrototype()returns an emptyJSValue(e.g. because a ProxygetPrototypeOftrap threw),getObject()crashes: an emptyJSValueis all zeros, soisCell()returnstruewhileasCell()returnsnullptr, which UBSAN/ASAN catches as a member call on a nullJSCell.Use the same pattern the fast path (lines 5121–5131) already uses: check truthiness of the returned
JSValueand clear any pending exception from the Proxy trap.Repro
Before the fix this segfaults while formatting
vfor thetoBeerror message. After, it throws a normalexpect(...).toBe(...)failure.Regression test added in
test/js/bun/util/inspect.test.js.