Fix crashes when inspecting objects whose property enumeration throws - #37175
Fix crashes when inspecting objects whose property enumeration throws#37175robobun wants to merge 11 commits into
Conversation
|
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 (16)
💤 Files with no reviewable changes (2)
WalkthroughChangesThe PR hardens inspection and property traversal against JavaScript exceptions. It adds Windows environment inspection symbol wiring, retryable inspection initialization, safe prototype traversal, updated SQL lazy evaluation, and regression tests. Inspection exception safety
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
@coderabbitai review |
|
|
Updated 7:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 54367dc has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37175That installs a local version of the PR into your bun-37175 --bun |
![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):
-
🟣
src/jsc/bindings/bindings.cpp:5637-5643— The samegetPrototype(globalObject).getObject()null-deref pattern this PR fixes has one other occurrence insrc/jsc/bindings/napi.cpp:2071(insidenapi_get_all_property_names's descriptor-filter loop). Per REVIEW.md's "fix the whole class in the same PR" rule, it's worth applying the same guard there or noting the intentional exclusion — pre-existing and in a different subsystem (N-API), so non-blocking.Extended reasoning...
What the bug is
This PR identifies
iterating->getPrototype(globalObject).getObject()as a crash pattern: when a ProxygetPrototypeOftrap throws (orgetPrototypeis called with an exception already pending),getPrototypereturns an emptyJSValue. On an empty JSValue,isCell()returns true (ValueEmpty == 0passes the not-cell-mask test),asCell()returnsnullptr, andasCell()->isObject()dereferences a null pointer → SIGSEGV. The PR fixes this atbindings.cpp:5637-5643by checkingscope.exception()before calling.getObject().Grepping for the exact pattern in
src/jsc/bindings/finds one remaining hit atnapi.cpp:2071:while (!owner->getOwnPropertyDescriptor(globalObject, propKey, desc)) { JSObject* proto = owner->getPrototype(globalObject).getObject(); if (!proto) { break; }
The
if (!proto) break;on the following line does not help — the crash happens inside.getObject()beforeprotois assigned.Code path that triggers it
Reachable from
napi_get_all_property_nameswhen a native N-API module calls it withnapi_key_include_prototypesand a descriptor filter (napi_key_enumerable/napi_key_writable/napi_key_configurable) on an object whose prototype chain contains a hostile Proxy.Step-by-step:
collectInheritedPropertyKeys(line ~2051) walks the prototype chain to collect property names. A statefulgetPrototypeOftrap returns a valid prototype (ornull) here, soNAPI_RETURN_IF_EXCEPTIONat line ~2056 passes.- The descriptor-filter loop then re-walks the chain per property key. For a key not owned by the base object,
getOwnPropertyDescriptorreturns false and the loop body runs. - Either (a) the stateful
getPrototypeOftrap now throws on this second walk, soowner->getPrototype(globalObject)returns{}; or (b) a throwinggetOwnPropertyDescriptortrap on the Proxy returns false with an exception pending, and the subsequentgetPrototypecall bails at its firstRETURN_IF_EXCEPTIONand returns{}. .getObject()on the empty JSValue callsasCell()->isObject()on a null cell → SIGSEGV.
Why existing code doesn't prevent it
The
NAPI_RETURN_IF_EXCEPTIONcheck happens before the filter loop, but there is no exception check betweengetOwnPropertyDescriptorandgetPrototype, nor betweengetPrototypeand.getObject(). Theif (!proto)guard at line 2072 executes only after.getObject()has already dereferenced the null cell.Impact
A native N-API module that enumerates properties (with prototype inclusion + descriptor filtering) on user-supplied objects can be crashed by a stateful/throwing Proxy in the prototype chain. Narrow attack surface — requires native module cooperation and an adversarial proxy — but it is the identical crash mechanism this PR is fixing for
console.log.Fix
Same shape as this PR's fix: split the call, check for a pending exception (returning
napi_pending_exceptionviaNAPI_RETURN_IF_EXCEPTIONor breaking) before calling.getObject():JSValue protoValue = owner->getPrototype(globalObject); NAPI_RETURN_IF_EXCEPTION(scope); JSObject* proto = protoValue.getObject(); if (!proto) break;
Why this is flagged
REVIEW.md's "Fix the whole class in the same PR" rule: "Grep for every sibling site sharing the pattern… If a site is intentionally excluded, say so in the PR." This is the only other hit for the pattern in
src/jsc/bindings/and the PR description does not mention excluding it.That said, this is pre-existing (not introduced or touched by this PR), lives in a different subsystem (N-API vs inspect), and has a much narrower trigger than
console.log. Non-blocking — worth either applying the same one-line guard here or noting the intentional exclusion.
|
Addressed both findings in 60ded04:
|
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/napi.cpp`:
- Around line 2071-2074: Update the prototype-walking loop around
owner->getOwnPropertyDescriptor() to call NAPI_RETURN_IF_EXCEPTION(env)
immediately after the descriptor lookup, before invoking owner->getPrototype().
Preserve the existing exception check after getPrototype() for proxy traps from
that operation.
🪄 Autofix
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: 2a33d9ca-3333-48d1-8111-c9acefa3eaf9
📒 Files selected for processing (8)
src/js/builtins/ProcessObjectInternals.tssrc/jsc/bindings/BunObject.cppsrc/jsc/bindings/JSEnvironmentVariableMap.cppsrc/jsc/bindings/UtilInspect.cppsrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/bindings.cppsrc/jsc/bindings/napi.cpptest/js/bun/util/inspect.test.js
💤 Files with no reviewable changes (1)
- src/jsc/bindings/BunObject.cpp
| it.concurrent("console.log(Bun) survives a lazy property initializer throwing", async () => { | ||
| const code = ` | ||
| globalThis.Symbol = -6; | ||
| console.log(Bun); | ||
| console.log("after-inspect"); | ||
| `; | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", code], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| // Enumeration must make it past the throwing initializers to the end of the table. | ||
| expect(stdout).toContain("zstdDecompress"); | ||
| expect(stdout).toContain("after-inspect"); | ||
| expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); | ||
| }); |
There was a problem hiding this comment.
🔴 The first new test ("console.log(Bun) survives a lazy property initializer throwing") passes on unfixed release Bun 1.3.14 — USE_SYSTEM_BUN=1 bun test reports PASS, violating CLAUDE.md's CRITICAL rule and REVIEW.md's "a test that passes both ways is worse than no test". On release, a throwing PropertyCallback reifies the slot as undefined and getPropertySlot returns true, so there is no release-observable delta for this input; the condition it guards is debug-only. Either find an assertion that distinguishes fixed/unfixed release, or gate it as debug-lane coverage (e.g. it.skipIf(!isDebug) with a comment) like the describe.skipIf(!isASAN) block below it. The second new test correctly SIGSEGVs unfixed release, so its coverage is fine.
Extended reasoning...
What the finding is
CLAUDE.md marks the USE_SYSTEM_BUN=1 check CRITICAL: "Your test is NOT VALID if it passes with USE_SYSTEM_BUN=1." REVIEW.md lists it under merge-blocking test rejections: "Confirm deleting each load-bearing clause of your fix breaks at least one test — a test that passes both ways is worse than no test." The first of the two new tests in this PR — it.concurrent("console.log(Bun) survives a lazy property initializer throwing", …) at test/js/bun/util/inspect.test.js:810-827 — passes on the current unfixed release binary.
Empirical proof on Bun 1.3.14 (revision 0d9b296af, which lacks this PR)
Control — the same binary is definitively unfixed, because the second new test's repro crashes it:
$ bun -e 'const p = new Proxy({a:1},{getPrototypeOf(){throw new Error("trap")}}); console.log(Object.create(p));'
panic(main thread): Segmentation fault at address 0x5
exit=132
First test's child code on that same binary:
$ bun -e 'globalThis.Symbol = -6; console.log(Bun); console.log("after-inspect");'
exit=0
stderr: (0 bytes)
stdout: contains "zstdDecompress" (2×) and "after-inspect" (1×)
All four assertions in the test hold on the unfixed release binary: expect(stdout).toContain("zstdDecompress") ✓, expect(stdout).toContain("after-inspect") ✓, expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }) ✓. bunExe() returns process.execPath and bunEnv does not set BUN_JSC_validateExceptionChecks, so USE_SYSTEM_BUN=1 bun test test/js/bun/util/inspect.test.js -t "lazy property initializer" reports PASS.
Why there is no release-observable delta for this input
The PR reorders CLEAR_IF_EXCEPTION before if (!hasProperty) continue; in JSC__JSValue__forEachPropertyImpl. But on release, when a static PropertyCallback initializer throws, JSC still reifies the slot (as undefined) and getPropertySlot returns true — so hasProperty is true, control never took the continue branch for Bun.$ / Bun.sql / Bun.SQL / Bun.postgres, and it reached the pre-existing CLEAR_IF_EXCEPTION on the very next line anyway. The unfixed release output confirms this: the throwing properties render as $: undefined, sql: undefined, postgres: undefined, SQL: undefined, and enumeration continues cleanly to zstdDecompress.
The other two changes this test's input touches are also debug-only for release: the removed reportUncaughtExceptionAtEventLoop calls in defaultBunSQLObject / constructBunSQLObject were #if BUN_DEBUG, and the releaseAssertNoException / storedPrototype assertions this input trips are only compiled under BUN_JSC_validateExceptionChecks=1 on debug builds. So the PR description's claim that "in release builds the stale exception silently made later properties vanish from the output" is not what this test observes — release output is identical before and after, for this input.
Why this matters
The fix is real (it prevents a debug-build abort under exception-scope verification and keeps the fuzzer clean), but this test does not defend it. If the reorder in bindings.cpp:5575-5582 regressed tomorrow, this test would stay green on every release CI lane. That is exactly the failure mode REVIEW.md's rule targets.
How to fix
Two options:
- Gate it as debug-lane coverage. Import
isDebugfromharnessand wrap the test init.concurrent.skipIf(!isDebug)(…), with the comment updated to say the failure mode is a debug assertion (releaseAssertNoExceptionunderBUN_JSC_validateExceptionChecks). Thedescribe.skipIf(!isASAN)block directly below at line 849 is precedent for exactly this pattern in the same file. - Find an assertion that distinguishes release output. From direct observation there does not appear to be one for
globalThis.Symbol = -6; console.log(Bun)— the throwing properties print asundefinedand everything else is identical. If a different input can make the reordered!hasProperty → continuebranch actually fire on release with an exception pending, use that instead.
The second new test (throwing getPrototypeOf trap) correctly SIGSEGVs the unfixed release binary (exit 132), so item #2's coverage is fine and needs no change.
|
Responses to the latest round of review comments:
|
|
The descriptor-trap check suggested in the latest review comment is already in place: 7992a1f made NAPI_RETURN_IF_EXCEPTION the first statement of that while body, before getPrototype runs. The comment appears to have been generated against the earlier diff. |
|
Both findings from the latest review round are addressed in 6a062a9:
I also swept the remaining |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/napi.cpp (1)
2070-2085: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd regression coverage for both descriptor lookup paths.
This change adds two exception-return paths. No same-change test exercises them. Add an N-API test through
get_all_property_nameswith a throwing descriptor trap andnapi_key_enumerable. Cover bothnapi_key_include_prototypesandnapi_key_own_only. Assert that the trap error propagates without a crash.As per coding guidelines, every behavioral change must include an automated regression test in the same change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/bindings/napi.cpp` around lines 2070 - 2085, Add regression coverage for both descriptor lookup branches exercised by get_all_property_names: use a proxy with a throwing getOwnPropertyDescriptor trap, call napi_key_enumerable with napi_key_include_prototypes and napi_key_own_only, and assert the trap error propagates without crashing.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/util/inspect.test.js`:
- Around line 829-846: Strengthen the test around the console.log call by
asserting stdout contains the default rendering of the enumerable custom-inspect
symbol and does not contain "custom!". Keep the existing "after-inspect"
assertion and successful stderr/exitCode checks unchanged.
---
Outside diff comments:
In `@src/jsc/bindings/napi.cpp`:
- Around line 2070-2085: Add regression coverage for both descriptor lookup
branches exercised by get_all_property_names: use a proxy with a throwing
getOwnPropertyDescriptor trap, call napi_key_enumerable with
napi_key_include_prototypes and napi_key_own_only, and assert the trap error
propagates without crashing.
🪄 Autofix
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: a97fc7cb-c092-45e2-b1d4-1734d63c625b
📒 Files selected for processing (4)
src/jsc/bindings/napi.cppsrc/jsc/modules/NodeUtilTypesModule.cpptest/js/bun/util/inspect.test.jstest/js/node/util/util.test.js
|
Both remaining findings addressed in fed88ec:
|
|
Applied in b399bc8: the util.isError repro now runs as a spawned fixture like the other crash-regression tests in this PR, asserting the child prints the caught trap error and exits 0. On the unfixed binary the child segfaults and only this one test fails instead of the whole runner. |
|
CI status for the latest build (90508): the PR's own tests (inspect, util.isError, napi) pass on every lane, including both Windows targets. The remaining failures are unrelated to this diff:
From my side the change is complete and verified: six crash fixes in the inspect, enumeration, napi, and util.isError exception paths, with regression tests that fail on current releases and pass here, checked on Linux and Windows. |
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunt pass came up clean — every finding from the earlier rounds has been addressed through b399bc8. Because the change spans exception handling in seven JSC binding files (forEachPropertyImpl, LazyProperty initializer contracts, napi prototype walk, VMInquiry slot lifetime, Windows env re-entrancy) and bakes in a design choice to permanently cache a toString fallback for utilInspectFunction when node:util fails to evaluate once, a human look is still worthwhile.
What was reviewed:
- The reordered
CLEAR_IF_EXCEPTIONinforEachPropertyImpland the newgetPrototypeguard match whatforEachPropertyOrderedalready does. m_utilInspectFunction/m_utilInspectStylizeColorFunctionnow alwaysinit.seton failure; the fallback is cached for the VM lifetime — worth a human confirming that tradeoff.- napi descriptor-trap checks now cover both
include_prototypesandown_onlyarms, with a Node-paritycheckSameOutputtest. windowsEnvreceives the inspect symbol viavm.symbolRegistry().symbolForKey("nodejs.util.inspect.custom"), which is the same registry keyBun.inspect.customresolves to.
Extended reasoning...
Overview
The PR fixes five related crashes in the property-enumeration path used by console.log / Bun.inspect when user JS (lazy property initializers, proxy traps) throws mid-enumeration. It touches bindings.cpp (forEachPropertyImpl exception ordering + getPrototype empty-value guard), napi.cpp (three new NAPI_RETURN_IF_EXCEPTION sites in the napi_get_all_property_names prototype walk), NodeUtilTypesModule.cpp (VMInquiry slot scoping + RETURN_IF_EXCEPTION after getPrototype in util.isError), ZigGlobalObject.cpp (LazyProperty initializers for utilInspectFunction / utilInspectStylizeColorFunction now always init.set a fallback), UtilInspect.cpp and the REPL formatter (clear-and-fallback when the lazy inspect function throws), BunObject.cpp (removed debug-only reportUncaughtExceptionAtEventLoop re-entry), and JSEnvironmentVariableMap.cpp + ProcessObjectInternals.ts (pass the inspect-custom symbol from C++ so Windows env setup no longer reifies a Bun lazy property mid-lookup). Four regression tests and a Node-parity napi test cover each fix.
Security risks
None identified. The changes tighten exception handling on paths reachable via hostile proxy traps and clobbered globals; no new user-controlled input reaches allocation sizes, paths, or credentials. The tryClearException calls swallow errors, but only on the best-effort formatting path where console.log is expected to keep going rather than crash — consistent with the existing CLEAR_IF_EXCEPTION pattern in the same loop.
Level of scrutiny
High. These are C++ JSC bindings on the exception-scope boundary — exactly the category REVIEW.md flags as most-blocked ("Exception checks after every call that can enter JS", "Never let a pointer or slice outlive the memory it points into"). The PR's own history bears this out: eight review rounds surfaced sibling sites (util.isError, the napi else-arm), a Windows-only re-entrancy crash, and a LazyProperty contract violation, each requiring a follow-up commit. A human should confirm the LazyProperty fallback design: once node:util fails to evaluate (e.g. a clobbered global), m_utilInspectFunction is permanently set to a toString-based stub for the VM's lifetime, so even if the global is later restored, custom inspect via that path won't recover. That's a defensible tradeoff (the alternative is a hard crash), but it's a behavioral choice.
Other factors
All prior automated findings (mine and CodeRabbit's) are resolved in the current diff. Test coverage is thorough — spawned crash repros for each SIGSEGV, a Node-parity checkSameOutput napi test, and the util.isError repro was moved to a spawned fixture per the last round's feedback. The one remaining unresolved inline (the first inspect test passing on 1.3.14 stable) was rebutted by the author: it fails on canary, which is what USE_SYSTEM_BUN targets. The Windows CI failures at 60ded04 that prompted the env-setup and LazyProperty changes are stated fixed in c9a6d0f; CI status on the current HEAD (b399bc8) should be verified before merge.
|
On the one tradeoff the review flags for a human, the permanent toString fallback for utilInspectFunction when node:util fails to evaluate: LazyProperty initializers in vendored JSC are once-only by contract. callFunc asserts that init.set ran after the initializer returns, and there is no reset API, so "leave it uninitialized and retry on the next access" is not expressible without patching vendored WebKit. The failure itself only happens when evaluating node:util throws, which in practice means the script already clobbered a global the module depends on (the repro sets globalThis.Symbol = -6). The exception from the failed load is not swallowed at that moment: it stays pending, and the callers added in this PR clear it and use default formatting for that one inspect. What the cached fallback costs is the narrow case where a script breaks a global, inspects an object with a custom inspect hook, restores the global, and inspects again in the same VM; that later inspect uses String(value) through the fallback instead of the real util.inspect. The alternative on the first failure was an abort in both debug and release builds, so the fallback seemed like the right side of the tradeoff, but it is easy to revisit if a retry-capable slot is preferred. |
|
Another fuzzer fingerprint ( globalThis.Symbol = NaN; // fuzzer did `Symbol++`
try { new CompressionStream(globalThis); } catch (e) {}
A direct Verified on current main (9008ae7): both repros abort a debug build, and with this PR's |
|
@robobun this conflicts with main now, please rebase and get a fresh CI run so it can be merged. |
In the slow path of JSC__JSValue__forEachPropertyImpl, a property slot lookup that reports not-found with an exception pending (a throwing lazy property initializer) skipped the exception clear, so the next property's initializer ran with the exception still set and tripped exception scope verification in debug builds. The prototype walk in the same loop called getObject() on the result of getPrototype() without checking for an exception. A Proxy getPrototypeOf trap that throws returns an empty JSValue, and getObject() on that dereferences a null JSCell. Also drop the debug-only reportUncaughtExceptionAtEventLoop calls in the Bun.SQL lazy property callbacks: they re-entered JS while the exception was still pending on the VM, which makes static table reification misreport lookups, and the exception is propagated to the caller right after anyway.
The napi_get_all_property_names descriptor-filter loop had the same getPrototype().getObject() pattern: a throwing proxy trap returns an empty JSValue and getObject() on it dereferences a null JSCell. Check for the pending exception before converting.
Two more ways a hostile global environment crashed property enumeration,
both found by the Windows lanes on this PR:
The windowsEnv builtin read Bun.inspect.custom during process.env setup,
reifying a lazy property on the Bun object. When env setup runs inside
another Bun lazy property initializer that then throws (Bun.$ evaluates
process.env before Symbol("cwd")), that structure transition poisons
the in-progress static property lookup. Pass the registered symbol from
C++ instead.
The utilInspectFunction and utilInspectStylizeColorFunction LazyProperty
initializers bailed without init.set when requiring node:util threw,
which trips LazyProperty's post-init assertion and corrupts the
property. Always set a value (a toString-based fallback, or the no-color
stylize) and leave the exception pending. Callers clear it and fall back
to default formatting instead of custom inspect.
…n pending A throwing getOwnPropertyDescriptor proxy trap left the exception pending while getPrototype ran the next trap.
Two bugs at the same line: the VMInquiry PropertySlot above was still alive when getPrototype ran a proxy's getPrototypeOf trap, and a live VMInquiry slot forbids VM entry (debug builds abort in checkVMEntryPermission, release builds turn the blocked entry into an empty JSValue). The empty value then passed isCell() and the inherits checks dereferenced a null JSCell. Scope the slot so it dies before the prototype read, and propagate the trap's exception like Node's instanceof does. Also mirror the descriptor-trap exception check on the napi_key_own_only arm of napi_get_all_property_names.
…fallback assertions The napi addon test runs napi_get_all_property_names with an enumerable filter over proxies whose getOwnPropertyDescriptor trap throws for one key, in both own_only and include_prototypes modes, asserting Node parity (napi_pending_exception with the trap error pending). The include_prototypes case segfaulted before the fix. The custom-inspect fallback test now also asserts the hook did not run and the object rendered with default formatting.
… load Replace the set-once LazyProperty slots for util.inspect and its color stylize with WriteBarrier members filled by accessors. When node:util fails to evaluate the accessor returns null with the exception pending and caches nothing, so the next inspect retries instead of the whole process being stuck on a toString stub. The callers already handle a null return. Drop the stub function and the unused Bun__REPL__formatValue. internal/sql/shared.ts read Bun.env at module scope, which reifies a property on the Bun object while the Bun.sql / Bun.SQL lookup that is evaluating the module is still in progress; if the tree then throws, debug builds assert in storedPrototype. Read process.env instead, which is the same object. Tests: the custom inspect fallback test now covers colors, the repeated call, and recovery once the global is restored; a spawned sql test covers the failing module tree.
… calls callCustomInspectFunction called utilInspectFunction() and then createInspectOptionsObject() with no exception check in between. Both accessors declare a ThrowScope, so when the first custom inspect in a process asked for colors, exception scope verification aborted on the x64-asan lane (bunshell.test.ts). The recovery test now performs the colored call first so the same sequence is covered.
efa764c to
d4446c7
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased onto current main (39fb3c1) and force-pushed; the branch is now 10 linear commits (the earlier merge commit and the two empty retrigger commits are gone) and the resulting diff against main is the same 16 files as before. Fresh CI: https://buildkite.com/bun/bun/builds/97246 Three conflicts, all mechanical:
On the rebased tree the debug build passes the inspect, util, sql env, custom-inspect, url and broadcast-channel files under BUN_JSC_validateExceptionChecks=1 (458 tests) plus the two napi tests, and the new inspect and isError tests still fail against the current canary, so nothing on main fixed these independently. |
|
Build 97246 is at 177 passed, 0 failed, but its two darwin 14 aarch64 test-bun shards have sat in scheduled for about five hours while newer main builds' arm64 shards have been picked up and passed, so they look stuck rather than queued. I only have read access on Buildkite; retrying those two jobs from the Buildkite side should complete the run (darwin x64 and every other lane are green), or it can be merged without them if you prefer. |
|
One more way to reach the const o = { [Symbol.for("nodejs.util.inspect.custom")]() { return "custom"; } };
let result;
function recurse() {
try { recurse(); } catch {}
if (result === undefined) { try { result = Bun.inspect(o); } catch {} }
}
recurse();
console.log(result);On |
|
Build 97827 (the rerun): 176 passed. The one red lane is x64-asan, where spawn-stdin-readable-stream.test.ts hit a heap-use-after-free (a 97KB stream chunk read in the fetch-body-to-stdin path, last touched by #36087); this PR does not touch that code and I have filed it separately. The two darwin aarch64 test shards are again sitting in scheduled, so that queue looks like a general problem for this pipeline rather than something specific to this build. Everything this PR touches is green on both runs. |
| JSValue protoValue = owner->getPrototype(globalObject); | ||
| // A throwing proxy trap leaves protoValue empty; getObject() on it is a null deref. | ||
| NAPI_RETURN_IF_EXCEPTION(env); |
There was a problem hiding this comment.
🟡 The sibling sweep of getPrototype sites ("I also swept the remaining getPrototype(globalObject) call sites in src/jsc") was C++-only; the Rust N-API napi_get_prototype at src/runtime/napi/napi_body.rs:979-980 has the same bug class fixed here — object.get_prototype() on a Proxy with a throwing getPrototypeOf trap returns napi_ok with an empty result and a pending exception, whereas Node returns napi_pending_exception. Per REVIEW.md ("Fix the whole class… If a site is intentionally excluded, say so in the PR"), consider either checking env.has_pending_exception() after the call, or noting in the PR body that Rust-side get_prototype callers are excluded (the API returns JSValue, not JsResult<JSValue>; five sites across napi_body.rs / ConsoleObject.rs / pretty_format.rs). Nit: pre-existing, no release crash, narrow trigger.
Extended reasoning...
What the finding is
The PR's sibling sweep of throwing-getPrototype sites — stated in the review response as "I also swept the remaining getPrototype(globalObject) call sites in src/jsc for the widened pattern" — covered only C++ files under src/jsc/. The Rust N-API implementation napi_get_prototype at src/runtime/napi/napi_body.rs:979-980 has the identical bug class this PR fixes in napi_get_all_property_names (napi.cpp:2080-2086):
result.set(env, object.get_prototype(env.to_js()));
env.ok()JSValue::get_prototype (JSValue.rs:2322) returns bare JSValue (not JsResult<JSValue>) and calls JSC__JSValue__getPrototype directly, with no from_js_host_call wrapper. JSC__JSValue__getPrototype (bindings.cpp:4163-4167) has no ThrowScope and no exception check — it just encodes value.getPrototype(arg1). env.ok() (napi_body.rs:124-126) unconditionally sets NapiStatus::ok without checking for a pending exception. So a throwing getPrototypeOf trap leaves the exception pending, writes an empty JSValue to *result, and returns napi_ok.
Step-by-step proof
- A native addon calls
napi_get_prototype(env, proxy, &out)whereproxyisnew Proxy({}, { getPrototypeOf() { throw new Error('trap') } }). preamble!(napi_body.rs:477-485) checks for a pending exception at entry only — none is pending yet, so it passes.object.is_empty()andobject.is_undefined_or_null()are both false (a Proxy is a non-empty object), so control reaches line 979.object.get_prototype(env.to_js())→JSC__JSValue__getPrototype→value.getPrototype(globalObject)→ProxyObject::getPrototype, which calls the user's trap. The trap throws;getPrototypereturns an emptyJSValue(encoded 0) with the exception pending on the VM.result.set(env, <empty>)writes 0 to*out. This does not dereference a cell, so no release crash — this differs from the napi.cpp site, which called.getObject()on the empty value.env.ok()returnsnapi_ok. The addon receivesnapi_ok+ an encoded-0napi_value+ a pending exception it did not expect.
Node.js returns napi_pending_exception here: its GetPrototypeV2() returns an empty MaybeLocal, and CHECK_MAYBE_EMPTY bails with the pending-exception status.
Why existing code doesn't prevent it
preamble! only checks exceptions at entry. The Rust JSValue::get_prototype type signature (fn get_prototype(&self, global: *mut JSGlobalObject) -> JSValue) does not surface the exception — the same shape as the C++ getPrototype(globalObject).getObject() before this PR fixed it, minus the null-deref. There is no post-body exception check between line 979 and the env.ok() return.
Impact
No release crash (the empty JSValue is stored, not dereferenced). The addon receives napi_ok with an invalid result and a pending exception, diverging from Node. The addon may then pass the empty napi_value to another N-API call and get napi_invalid_arg for no obvious reason, or the pending exception surfaces one N-API call late. Under BUN_JSC_validateExceptionChecks=1, the addon's next N-API call that declares a ThrowScope (via preamble! → NAPI_PREAMBLE) would observe and return napi_pending_exception, so the exception is caught at the next call rather than this one — one call late.
Why this belongs in scope, and why it's still a nit
REVIEW.md, Correctness: the bug class, not the bug: "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep). Grep for every sibling site sharing the pattern… If a site is intentionally excluded, say so in the PR." This is a direct sibling of the napi.cpp site the PR fixes and adds a test for, in the same N-API surface (napi_get_prototype vs napi_get_all_property_names), missed because the sweep was scoped to src/jsc C++ files. The PR does not touch napi_body.rs, and the trigger requires a native addon plus a Proxy with a throwing getPrototypeOf trap — narrow. It does not block merge.
How to fix
Either:
result.set(env, object.get_prototype(env.to_js()));
if env.has_pending_exception() {
return env.pending_exception();
}
env.ok()…or note in the PR body that Rust-side get_prototype callers are intentionally excluded from this sweep. There are five: napi_body.rs:979, ConsoleObject.rs:3242/3972/4038, pretty_format.rs:877 — none wrap the call in from_js_host_call, and get_prototype itself does not return JsResult. Fixing the API signature to return JsResult<JSValue> would cover all five at once but is a larger change than this PR's scope.
|
On the Rust-side sibling (napi_get_prototype in napi_body.rs): confirmed. JSValue::get_prototype returns a bare value, so a throwing getPrototypeOf trap comes back as napi_ok with an empty result and the exception pending, where Node returns napi_pending_exception. I also checked the formatter callers of the same Rust function (ConsoleObject.rs) by inspecting proxies over objects, classes and functions with a throwing trap, directly and nested, under exception scope validation; they are not reachable because the formatter unwraps proxies to their targets first. Since that napi case is a parity issue rather than a crash and the right fix is changing get_prototype to return a result (which touches all five Rust callers), I have left it out of this PR, noted the exclusion in the description, and filed it for a separate fix so this one can merge as reviewed. |
|
Closing: everything in this PR is covered by an open PR, one per bug.
|
…p or getPrototypeOf throws during the property walk (#29642) ### Problem - `Bun.inspect()`, `console.log()` and `expect()` failure output crash with `Segmentation fault at address 0x5` (debug builds: UBSan "member call on null pointer of type 'JSC::JSCell'" in `JSCJSValueCell.h`) when a property lookup throws while an object is being formatted. Release repro: ```js const proto = new Proxy({ a: 1 }, { getPrototypeOf() { throw new Error("boom"); } }); console.log(Object.create(proto)); ``` and likewise with a Proxy whose `get` trap (or a getter reached through a Proxy) throws for one property. - The same happens with a lazily initialized property of the `Bun` object whose initializer throws (fuzzer sample: `globalThis.Symbol` replaced, then `Bun.inspect(Bun)`; the builtin behind `Bun.$` calls `Symbol("cwd")`). Release builds print `Bun` with most of its properties missing (72 of 115 on the shipped build); debug builds abort with `ASSERTION FAILED: Unexpected exception observed` / `Symbol is not a function. (In 'Symbol("cwd")' ...)` when the next lazy property is built. - Plain-JavaScript variant of the same leak: `console.log` of a module namespace during an import cycle, when one export is still in its temporal dead zone, throws `ReferenceError: Cannot access 'x' before initialization` out of `console.log` (the stale exception is picked up while the next export is formatted). `util.inspect` prints such an export as `<uninitialized>`. - Cause, in the slow path of `JSC__JSValue__forEachPropertyImpl` (`src/jsc/bindings/bindings.cpp`): - `object->getPropertySlot()` reports a throwing Proxy trap, a throwing lazy initializer or a TDZ namespace export as "not found" with the exception still pending, and the loop `continue`d before the `CLEAR_IF_EXCEPTION` below it. The following lookups and formatting callbacks then run with that exception pending (the dropped properties, the rethrown `ReferenceError`, the debug assertion). - When the walk moves to the next prototype, `iterating->getPrototype(globalObject).getObject()` runs `getObject()` on the empty `JSValue` that `getPrototype` returns when it threw (either because of the stale exception above or because the `getPrototypeOf` trap itself throws). The empty value passes `isCell()`, so this reads the type byte of a null cell: the fault at address 5. - `napi_get_all_property_names` (`src/jsc/bindings/napi.cpp`, descriptor filter loop) has the same `getPrototype().getObject()` chain after an unchecked `getOwnPropertyDescriptor`, so a Proxy trap throwing there returned `napi_ok` with an exception pending in own-only mode and segfaulted in include-prototypes mode. - `defaultBunSQLObject` / `constructBunSQLObject` (`src/jsc/bindings/BunObject.cpp`) had a debug-only block that handed a sql module load failure to `reportUncaughtExceptionAtEventLoop` while the exception was still pending on the VM, so `globalThis.Symbol = NaN; Bun.sql` (and the fuzzer sample above, once the walk gets past `Bun.$`) aborted debug builds with `ASSERTION FAILED: ... object->structure() == this` in `Structure::storedPrototype` instead of throwing. ### Fix - `bindings.cpp`: clear the exception after `getPropertySlot` regardless of its result, which is what the ordered variant `JSC__JSValue__forEachPropertyOrdered` already does; read the next prototype into a `JSValue`, clear the exception and stop the walk when it is empty. (An earlier revision also held the prototype being walked under an `EnsureStillAliveScope`; dropped, since the raw pointer is used after every call into JS in the loop body and so is live across them anyway.) - Behaviour change to note: a property whose lookup throws is now left out of the output instead of the whole `console.log` / `Bun.inspect` call throwing or crashing. For TDZ namespace exports this differs from `util.inspect`'s `<uninitialized>`; printing that marker would be a formatter feature on top of this fix and is not attempted here. - `napi.cpp`: check for an exception after `getOwnPropertyDescriptor` and after `getPrototype` and return `napi_pending_exception`, which is what Node returns for these cases. - `BunObject.cpp`: drop the debug-only report. The exception is propagated to the reader by the `RETURN_IF_EXCEPTION` right below it, so debug builds now behave like release builds (`Bun.sql` throws). - Why this is the right place: the formatter deliberately swallows errors thrown by individual properties (getters, traps) and prints the rest of the object; these two sites were the only ones in the walk that acted on a "not found" result or a prototype value before clearing the exception that produced it. Skipping just the property (or stopping at just the prototype) whose lookup threw is the existing behaviour for every other throw site in this function. - Verification: - `test/js/bun/util/inspect.test.js`, "Bun.inspect when a property lookup throws" (5 spawned cases): a Proxy `get` trap, a getter behind a Proxy, a `getPrototypeOf` trap, a throwing lazy `Bun` property, and a two-file import cycle with a TDZ export. Without the fix (shipped release build and an unfixed debug build) all five fail: the Proxy children segfault / fail UBSan, the `Bun` child prints `[false,false,...]` on release and aborts on debug, the cycle child exits 1 with the `ReferenceError`; with it each prints everything except the one property whose lookup threw. - `test/js/bun/util/BunObject.test.ts`, "a lazy property whose builtin fails to load throws from the read": `Bun.$` / `sql` / `SQL` / `postgres` with `Symbol` broken throw a `TypeError` on two consecutive reads. Aborts on an unfixed debug build (the `BunObject.cpp` hunk); passes on release either way, as the removed block is debug-only. The fixture builds `process.env` before breaking `Symbol` because the `$` builder reads it, and building it on Windows reifies another `Bun` property mid-lookup, which on a Windows debug build would hit the separate `storedPrototype` assertion that #37001 fixes (verified on Linux only). - `test/napi/napi.test.ts`: `getOwnPropertyDescriptor` trap throwing in own-only and include-prototypes mode (compared against Node), and a `getPrototypeOf` trap that throws on the second call so the check after `getPrototype` is the one that fires. - Repros above and the tests also run clean under `BUN_JSC_validateExceptionChecks=1`. ### Background - `forEachPropertyImpl` is the property walk behind Bun's native formatter. It collects the property names of the object and of up to five prototypes, looks each one up through the original object with `getPropertySlot`, and hands the value to a callback that formats it. Errors thrown by individual properties are swallowed on purpose so that one bad getter does not make `console.log` throw. - A JSC exception is "pending" state on the VM, not C++ unwinding. A function that throws returns a failure value (`false`, or the empty `JSValue`) and leaves the exception on the VM; until something clears or rethrows it, most JSC entry points return early as soon as they are called, and debug builds assert when a function that did not throw is observed returning with an exception pending. `CLEAR_IF_EXCEPTION` drops the pending exception. - The empty `JSValue` (`JSValue()`) is encoded as 0. `isCell()` is true for it, so `getObject()` on it dereferences a null cell pointer rather than returning null; callers have to test the value itself first. - Lazy properties of the `Bun` object are entries in a static property table whose value is produced by a builder the first time the property is read (`PropertyCallback`). Some builders evaluate built-in JavaScript modules (the shell for `Bun.$`, the sql module for `Bun.sql`), so they can throw when that module fails to evaluate, and JSC reports that to the reader as "property not found" plus a pending exception. ### Consolidated duplicates Found repeatedly by the fuzzer (fingerprint `d678cafe50a2ad6e`). Earlier round, folded in here in April: #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325. This round, closed in favour of this PR: #30099 #30245 #37160 #37175 #37213 #37256 #37428 #38700 #38921 #39363 #39365 #39380 #39412 #39413 (and #39411, closed earlier). The `BunObject.cpp` hunk and the `BunObject.test.ts` test come from #30245 / #37428; the same hunk is also part of #37001, which fixes the underlying `storedPrototype` assertion in JSC. Related fixes that are not part of this bug and stay open on their own: #39382 (a custom inspect function when `node:util` fails to load), #37202 (`util.isError` with a throwing `getPrototypeOf` trap), #37331 (`forEachPropertyOrdered` when the callback throws), #37001 (stale structure in `JSObject::getPropertySlot`), #32263 (additional checks in the same napi loop). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 11 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js test/napi/napi.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
2 participants
What does this PR do?
Fixes a crash found by fuzzing (flaky under the fuzzer because it depends on global state from earlier iterations in the same process; deterministic repro below), plus several more crashes in the same family that surfaced while fixing it.
Inspecting an object can run arbitrary JS: lazy property initializers on native objects, getters, and proxy traps. The property enumeration loop used by
console.logandBun.inspect(JSC__JSValue__forEachPropertyImpl) mishandled exceptions from that JS in two places, and several things downstream of it crashed once those were fixed.1. Exception leaked across loop iterations. When
getPropertySlotreported not-found with an exception pending (a lazy property initializer threw), thecontinueskippedCLEAR_IF_EXCEPTION, so the next property's initializer ran with the exception still set and tripped exception scope verification in debug builds:In release builds the stale exception silently made later properties vanish from the output. The fix clears the exception before the
continue, matching whatJSC__JSValue__forEachPropertyOrderedalready does.2. Null deref in the prototype walk. The same loop advanced with
iterating->getPrototype(globalObject).getObject(). AProxygetPrototypeOftrap that throws makesgetPrototypereturn an emptyJSValue, andgetObject()on an empty value calls a member function on a nullJSCell. This crashes release builds:The same pattern had two other live occurrences: the prototype walk in
napi_get_all_property_names(napi.cpp, both theinclude_prototypesandown_onlyarms now check for a pending exception after each trap) andutil.isError(NodeUtilTypesModule.cpp). Theutil.isErrorone had a second problem at the same line: aVMInquiryPropertySlot was still alive whengetPrototyperan the trap, and a live VMInquiry slot forbids VM entry (debug builds abort incheckVMEntryPermission; in release the blocked entry is what produced the empty value). The slot is now scoped to die before the prototype read.util.isError(new Proxy({}, { getPrototypeOf() { throw 0 } }))segfaults current releases.3. Debug-only re-entry with a pending exception, and the Bun.sql module tree.
defaultBunSQLObjectandconstructBunSQLObjectcalledreportUncaughtExceptionAtEventLoopwhile the exception was still pending; that re-enters JS and trips a structure assertion. Every other caller clears first and these two propagate right after, so the calls are removed. With them gone, the sql tree had the same hazard as item 4 below:internal/sql/shared.tsreadBun.envat module scope, which reifies a property on the Bun object while theBun.sql/Bun.SQLlookup evaluating the module is still in progress, so a tree that then fails to evaluate (globalThis.Error = -6; Bun.sql) left debug builds asserting instoredPrototype. It now readsprocess.env, which is the same object. That is the only module-scope read of a Bun property in the sql tree.4. Windows: env setup reified a Bun property mid-lookup. The
windowsEnvbuiltin readBun.inspect.customwhile building theprocess.envproxy.Bun.$'s initializer evaluatesprocess.envbeforeSymbol("cwd"), so on Windows the firstBun.$access transitioned the Bun object's structure and then threw, poisoning the in-progress static property lookup (caught by this PR's Windows CI). The symbol is now passed in from C++.5. util.inspect lazy slots could not fail.
m_utilInspectFunctionandm_utilInspectStylizeColorFunctionwere set-onceLazyPropertyslots whose initializers bailed withoutinit.setwhen requiringnode:utilthrew, which violates the LazyProperty contract and aborts both debug and release builds:The failure is usually transient (the registry does not cache a failed load; a stack overflow during the first custom inspect is enough), so caching a fallback would leave inspect degraded for the rest of the process. The two slots are now
WriteBarriermembers filled by accessors: on failure the accessor returns null with the exception pending and caches nothing, so the next inspect retries.callCustomInspectFunctionclears it and falls back to default formatting for that one value; the other callers (URL, URLSearchParams, BroadcastChannel, streams) already propagate a pending exception. The unusedBun__REPL__formatValueis deleted.Not in this PR:
globalThis.Symbol = -6; require.cacheaborts in the same way throughm_lazyRequireCacheObject(NodeModuleModule.cpp). It is a different subsystem and is tracked for a separate fix using the same retryable-slot shape. Also left out: the Rust side has its own prototype read,JSValue::get_prototype, which returns a bare value with no way to report a throwing trap. Its formatter callers are not reachable with a throwing trap (the formatter unwraps proxies to their targets first; checked under exception scope validation), butnapi_get_prototypereturnsnapi_okwith an empty result and the exception left pending where Node returnsnapi_pending_exception. That is a parity bug rather than a crash and the right fix is at theget_prototypesignature, so it is tracked separately as well.How did you verify your code works?
Regression tests, each verified against the unfixed binary:
test/js/bun/util/inspect.test.js: the clobbered-Symbol enumeration ofBun, the throwinggetPrototypeOftrap, and the custom inspect fallback. The fallback test pins that the fallback applies with and without colors, that a repeated call still falls back (nothing cached), and that the hook works again once the global is restored. All fail or crash on current releases.test/js/node/util/util.test.js: spawnedutil.isErrorrepro (segfaults current releases).test/napi/napi.test.ts+ addon:napi_get_all_property_nameswith a throwing descriptor trap in both key modes, asserting byte parity with Node (theinclude_prototypescase segfaults current releases).test/js/sql/adapter-env-var-precedence.test.ts: spawned failing-sql-tree repro. This one is a debug assertion only: it aborts the debug build without theshared.tschange (verified by reverting just that file) and passes release builds either way. The rest of that file exercises the env resolution thatshared.tsdoes, and passes unchanged withprocess.env.Full files pass on Linux (
bun bd teston inspect, util, bun-inspect, custom-inspect, url, broadcast-channel, napi, and the sql env file) and items 1, 2, 4 and the earlier version of 5 were also verified on a Windows machine. The original fuzzer reproduction exits cleanly on the fixed debug build.no test proof · iteration 11 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js test/js/sql/adapter-env-var-precedence.test.ts test/napi/napi.test.ts