Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR makes console inspection resilient to exceptions from lazy properties, proxy traps, prototype access, and unavailable inspection helpers. It also removes debug-only SQL exception reporting while preserving normal exception propagation. ChangesConsole inspection resilience
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 2
🤖 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/web/console/console-log.test.ts`:
- Around line 154-169: Update both subprocess-based tests, including the test
around “console.log(Bun) survives lazy properties whose initializer throws” and
the separately referenced test, to use test.concurrent instead of it. Import
test from bun:test and preserve each test’s existing setup and assertions.
- Around line 154-169: Update both regression tests at
test/js/web/console/console-log.test.ts:154-169 and
test/js/web/console/console-log.test.ts:171-190 to drain proc.stderr
concurrently with proc.stdout and proc.exited, then assert the combined
subprocess output. Apply the same change to both tests while preserving their
existing success and exit-code assertions.
🪄 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: 36ae9763-851a-4050-a568-2c4555f85d87
📒 Files selected for processing (3)
src/jsc/bindings/BunObject.cppsrc/jsc/bindings/bindings.cpptest/js/web/console/console-log.test.ts
💤 Files with no reviewable changes (1)
- src/jsc/bindings/BunObject.cpp
|
Applied both suggestions in 13e8e43: the two new tests now run with it.concurrent, and both drain stderr alongside stdout and the exit code, asserting all three together (stderr is empty on the fixed build, and on an unfixed debug build the assertion text lands there, so the failure message now shows it). Verified with bun bd test test/js/web/console/console-log.test.ts, 6 pass. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Ignore exceptions from "Get" proxy traps and throwing lazy | ||
| // property initializers; either may report "not found" with the | ||
| // exception still pending. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Trimmed the comment in forEachPropertyImpl to a single line in 19719ae. It keeps the one non-obvious fact (getPropertySlot can throw while reporting not found, so the clear has to come before the continue) without the justification prose. Tests still pass locally. |
|
Updated 1:01 AM PT - Aug 17th, 2026
✅ @robobun, your commit 09f65a0b1149d8414b9ad81d7f00947cc095cba9 passed in 🧪 To try this PR locally: bunx bun-pr 37202That installs a local version of the PR into your bun-37202 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Good catch on the release-observable signal. Verified it on an unfixed release build (1.4.0-canary 45ee955): with Symbol clobbered, the leaked exception made every later static-table reification report not found, so the output jumped straight from the header to main (the first custom accessor entry, whose different lookup path finally hit the clear). Everything between $ and main, including Archive, was missing. 666a42d adds expect(stdout).toContain("Archive"), and with that both new tests now fail under USE_SYSTEM_BUN=1: the first via the missing property, the second via a segfault at address 0x5 (exit 139), since the null cell getObject() on the empty prototype value crashes release builds outright, not only under UBSan. The stderr drain finding was based on a stale diff: both tests have drained stderr in the Promise.all and asserted it empty since 13e8e43. |
There was a problem hiding this comment.
LGTM — all prior feedback addressed (stderr drain, it.concurrent, one-line comment, and the release-visible Archive assertion so both tests now fail under USE_SYSTEM_BUN=1).
What was reviewed:
forEachPropertyImplreorder matches the sibling pattern inforEachPropertyOrdered(bindings.cpp:5766-5770).getPrototypeempty-check stops the walk instead of calling.getObject()on a null cell — the second test segfaults (exit 139) on unfixed release, per the author's verification.- Removed debug-only
reportUncaughtExceptionAtEventLoopcalls ran with a pending exception;RETURN_IF_EXCEPTIONon the next line already propagates.
Extended reasoning...
Overview
Three files touched: src/jsc/bindings/bindings.cpp (reorder CLEAR_IF_EXCEPTION before the not-found continue in forEachPropertyImpl; check getPrototype() for empty before .getObject()), src/jsc/bindings/BunObject.cpp (remove two #if BUN_DEBUG reportUncaughtExceptionAtEventLoop calls whose exception already propagates via RETURN_IF_EXCEPTION), and test/js/web/console/console-log.test.ts (two new concurrent subprocess regression tests).
Security risks
None. This is the console.log/Bun.inspect property-iteration path — a diagnostic/output path, not auth/crypto/permissions. The changes clear pending exceptions during best-effort inspection (matching how the surrounding code already handles getter/trap exceptions) and add a null-guard on an empty getPrototype() result.
Level of scrutiny
Moderate. C++ JSC bindings are memory-sensitive, but each hunk is a minimal, well-localized fix: the getPropertySlot reorder is byte-for-byte the same shape as forEachPropertyOrdered at bindings.cpp:5765-5770; the nextPrototype.isEmpty() guard prevents a member call on a null cell (release segfault, not just UBSan); and the removed debug-only calls were themselves the bug (running process.get("_fatalException") with a pending exception).
Other factors
Every prior review comment was addressed across four follow-up commits: CodeRabbit's test.concurrent + stderr-drain (13e8e43), the comment-cop paragraph trim (19719ae), and my previous run's USE_SYSTEM_BUN=1 concern (666a42d added expect(stdout).toContain("Archive"), and the author verified both tests now fail on unfixed release — the first via missing properties, the second via a segfault at 0x5). The bug-hunting system found no issues this run. Tests follow harness conventions: await using, bunEnv, concurrent pipe drain, combined-object assertion.
There was a problem hiding this comment.
LGTM — all prior review feedback has been addressed.
What was reviewed:
forEachPropertyImpl: the reorderedCLEAR_IF_EXCEPTIONbeforecontinuenow matchesforEachPropertyOrdered(bindings.cpp:5766-5770), and the slow-pathgetPrototypeempty-check matches the fast path at :5554-5564.- Removed debug-only
reportUncaughtExceptionAtEventLoopcalls in the SQL lazy callbacks —RETURN_IF_EXCEPTIONon the next line already propagates, and re-entering JS with a pending exception was itself the bug. - Tests: both now drain stderr, run concurrently, and assert a release-observable signal (
Archivein output) so they fail underUSE_SYSTEM_BUN=1.
Extended reasoning...
Overview
Three files touched: src/jsc/bindings/bindings.cpp (+9/-4 in JSC__JSValue__forEachPropertyImpl), src/jsc/bindings/BunObject.cpp (-6, removes two debug-only reportUncaughtExceptionAtEventLoop calls), and test/js/web/console/console-log.test.ts (+47, two subprocess regression tests). Fixes a fuzzer-found debug abort plus a UBSan null-cell deref reachable from plain JS via console.log.
Security risks
None. This is the console formatter's property/prototype walk, which already swallows exceptions from getters/traps by design; the fix adds two more clear points and an empty-value guard. No auth, crypto, network, or user-facing API surface. The removed code was #if BUN_DEBUG-gated and its removal cannot affect release behavior.
Level of scrutiny
Medium — JSC bindings C++ is memory-safety-sensitive, but the change is mechanical: (1) hoist the getPropertySlot result into a bool so CLEAR_IF_EXCEPTION runs before the not-found continue, exactly as forEachPropertyOrdered already does at bindings.cpp:5766-5770; (2) guard nextPrototype.isEmpty() before .getObject(), matching the fast-path handling at :5554-5564; (3) delete two debug-only lines that re-entered JS with a pending exception. Each change makes a slow path consistent with its already-correct sibling.
Other factors
Four rounds of review feedback were all applied: CodeRabbit's it.concurrent + stderr-drain (13e8e43), comment-cop's long-comment complaint (19719ae), and my two prior inline nits — stderr drain (already fixed by 13e8e43) and the USE_SYSTEM_BUN=1 release-observable assertion (666a42d adds expect(stdout).toContain("Archive"), verified to fail on an unfixed release build). Both of my inline threads are resolved. The PR description includes fails-without/passes-with evidence on both debug-ASAN and release. The bug hunting system found no issues on the current revision.
|
The Windows CI failure was real: with Symbol clobbered, console.log(Bun) still aborted there through two paths the PR had not covered, and the test child died before writing anything (hence the empty stdout).
Verified on windows-x64 (the failing lane's scenario): the repro prints the full object and exits 0, console-log tests 6/6, and the env suites (env-windows, run-process-env, env.test.ts) pass. Linux: 6/6 plus the tamper sweep, with the custom inspect case confirmed to abort debug builds and fail release builds before this commit. |
| // Reading Bun.inspect.custom in the builtin would reify a Bun property while | ||
| // env init may be running inside another Bun property's lazy initializer, | ||
| // invalidating the structure cached by that lookup's getPropertySlot. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Stand-in for util.inspect (and its stylize helpers) when loading node:util | ||
| // throws, which user code can cause by clobbering globals the module reads. | ||
| // Identity keeps stylize's no-color semantics and lets inspection stay usable. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The initializer must set something; a throwing one leaves the | ||
| // property lazy-tagged, which asserts in LazyProperty::callFunc. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The initializer must set something; a throwing one leaves the | ||
| // property lazy-tagged, which asserts in LazyProperty::callFunc. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Trimmed the four new comments to one line each in the latest commit, same facts kept: the symbol is passed in because reading Bun.inspect.custom reifies a Bun property mid-lookup, and LazyProperty initializers must set even on exception. |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/jsc/bindings/ZigGlobalObject.cpp:2357-2366— The identity-fallback branch here is unreachable for the scenario it was added for: whennode:utilfails to load,utilInspectFunction()now returns the identity fallback with no exception pending, sogetStylizeWithColor(fallback)succeeds and returns a JS closure —dynamicDowncast<JSFunction>succeeds and lines 2363-2366 are skipped. That closure then throwsTypeError: undefined is not an objectatinspect.styles[type]when a custom inspect callsoptions.stylizewith colors on. Still strictly better than pre-PR (throw vs. assert/UB); consider putting empty.styles/.colorson the fallback function, or detectingutilInspectFunction() === fallbackand taking the identity path directly.Extended reasoning...
What the finding is
The PR adds
functionUtilInspectFallback(identity, ZigGlobalObject.cpp:2011-2016) so that whennode:utilfails to load — which user code can trigger by clobbering globals likeSymbol—m_utilInspectFunctionandm_utilInspectStylizeColorFunctionstillinit.set()something instead of returning early and leaving the LazyProperty tagged (which asserts inLazyProperty::callFuncon debug and reuses a lazy-tagged pointer as a JSFunction on release). The comment at line 2013 says "Identity keeps stylize's no-color semantics and lets inspection stay usable."For
m_utilInspectStylizeColorFunctionspecifically, the fallback branch at lines 2363-2366 is not reached in the scenario it was added for, and the stylize that IS installed will throw when a user's custom inspect calls it.The specific code path
With
globalThis.Symbol = NaNandcolors=true:createInspectOptionsObject(UtilInspect.cpp:30) callsglobalObject->utilInspectStylizeColorFunction().- Its initializer (ZigGlobalObject.cpp:2349-2366) first calls
utilInspectFunction(). That initializer runsrequireId(NodeUtil)→ throws (node:util readsSymbolat module load) →scope.tryClearException()→init.set(JSFunction::create(..., functionUtilInspectFallback, ...)). It returns the identity native JSFunction with no exception pending. - Back in the color-stylize initializer,
argsnow contains the identity fallback.profiledCall(getStylize, ..., args, returnedException)runsgetStylizeWithColor(fallbackIdentity). getStylizeWithColor(src/js/builtins/UtilInspect.ts:4-14) is:It only returns a closure — it does not touchexport function getStylizeWithColor(inspect) { return function stylizeWithColor(str, styleType) { const style = inspect.styles[styleType]; ... }; }
inspect.stylesat construction time. SoprofiledCallsucceeds,returnedException == nullptr,scope.exception() == nullptr.dynamicDowncast<JSFunction>(result)succeeds (JS closures areJSFunction) →init.set(closure)→ early return at line 2361. The fallback branch at 2363-2366 is never taken.- Later,
createInspectOptionsObjectputs this closure onoptions.stylize. If a user's[util.inspect.custom](depth, options)callsoptions.stylize(str, "string"), the closure evaluatesinspect.styles[styleType]whereinspectis the bare native fallback function — it has no.stylesproperty →undefined[styleType]→TypeError.
Why existing code doesn't prevent it
The fallback check at line 2358 only fires when
getStylizeWithColoritself throws or returns a non-function. Neither happens: the builtin unconditionally returns a closure regardless of whatinspectis. And since them_utilInspectFunctioninitializer now clears its exception before returning, there's no pending exception for line 2358 to observe either.The new test doesn't catch this because
bunEnvsetsNO_COLOR: "1"(test/harness.ts), socolors=falseandutilInspectStylizeNoColorFunction()(which just wrapsstylizeWithNoColor, a plain identity that never readsinspect.styles) is used instead. The test's custom inspect() => "custom-ok"also never callsoptions.stylize.Impact
This is nit, not normal, because:
- Strictly better than pre-PR: before this PR, the same scenario asserted in debug (
LazyProperty::callFunc) and reused a lazy-tagged pointer as a JSFunction in release. A catchable TypeError from inside a user's custom inspect is a clear improvement — the formatter's caller (Bun__JSValue__callCustomInspectFunction) already has to handle a throwing custom inspect (users can throw for any reason). - Triple edge case: requires (a) globals clobbered such that
node:utilfails to load, (b)colors=true(TTY, noNO_COLOR/FORCE_COLOR=0), and (c) an object whose[util.inspect.custom]actually callsoptions.stylize. This is fuzzer territory. - No memory unsafety, no crash.
The observation is worth noting because (1) the
m_utilInspectStylizeColorFunctionfallback branch is effectively dead code for its primary intended use case, and (2) the comment's stated design goal ("Identity keeps stylize's no-color semantics") is not achieved for the color path. REVIEW.md's "Cover the variant matrix — both states of every flag" applies: the test exercisesNO_COLOR=1only.How to fix
Either:
- Add empty
.stylesand.colorsobject properties on the fallback function (soinspect.styles[type]returnsundefinedandstylizeWithColorfalls through toreturn str, matching the no-color identity), or - After
utilInspectFunction()returns, check whether it's the fallback (e.g. compare against a stored pointer, or check for absence of.styles) and take the identity-fallback path directly instead of callinggetStylizeWithColoron it.
The first option is a one-liner in the
m_utilInspectFunctioninitializer's fallback branch and makes the closure behave as documented.
|
1e4ef61 covers the latest review findings and the CI failure on a868a86:
|
|
Good find, that was the only other occurrence of the pattern in the repo. 7f74ab4 applies the same guard to the prototype climb in napi_get_all_property_names, using NAPI_RETURN_IF_EXCEPTION rather than a clear since N-API surfaces pending exceptions: one check after getOwnPropertyDescriptor (a trap can throw while reporting not found) and one after getPrototype before using the result. Verified the napi property-name suite passes, including the existing Proxy and String wrapper parity test that runs this loop. No dedicated test for the adversarial stateful trap: triggering it requires tuning how many getPrototypeOf calls happen before the throw, and that count is an engine internal that differs between JSC and V8, so a checkSameOutput parity test would be flaky by construction. The crash class itself is covered by the console-log regression tests. |
|
Right again, tryClearException keeps terminations pending (this file documents that at the clearTerminationException helper), so the bare assertNoException could abort if worker.terminate() landed inside requireId(NodeUtil). 46296af switches to assertNoExceptionExceptTermination to match the other four sites in the file, and additionally skips the getStylize call when a termination survived the nested initializer, since entering JS with it pending would trip the call machinery's own assertions. Both initializers still always set, and the termination propagates to whoever forced the property. Console-log tests and the exception check validator cases pass locally. |
|
Both fixed in 0547cef, and the isError one turned out deeper than the analysis: the crash happens before the throw path is even reached. The VMInquiry PropertySlot a few lines up holds a DisallowVMEntry for its lifetime and was still alive at the getPrototype call, so invoking any Proxy getPrototypeOf trap (throwing or not) aborted debug builds at VM::checkVMEntryPermission; on release the rejected trap result then produced the null cell dereference your analysis described. The slot is now scoped to the toStringTag check, and the exception check after getPrototype covers the throwing case. Added a test in test/js/node/util/util.test.js asserting the trap error propagates (matching Node, whose instanceof walk also runs the trap), plus a spot check that a trap returning Error.prototype now answers true instead of aborting. Also added the missing check after getOwnPropertyDescriptor in the napi_key_own_only arm. And fair point on the earlier sweep: my grep only matched the .getObject() spelling, so the isCell variant slipped through. I re-checked getPrototype call sites with no adjacent exception handling and these two were the remaining offenders; util and napi property suites pass. |
| // The VMInquiry slot disallows VM entry for its lifetime; it must die | ||
| // before getPrototype below can legally run a Proxy trap. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
Actionable comments posted: 2
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/ZigGlobalObject.cpp (1)
2315-2321: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate
node:util.inspectbefore downcasting.User code can replace the mutable
node:util.inspectexport before this lazy initializer runs. A non-function value reachesuncheckedDowncast<JSFunction>and skips the fallback. Accept the value only whendynamicDowncast<JSFunction>(prop)succeeds; otherwise initialize the fallback.🤖 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/ZigGlobalObject.cpp` around lines 2315 - 2321, Update the lazy initializer around nodeUtilValue and prop to validate node:util.inspect with dynamicDowncast<JSFunction> before assigning it. If the value is not a JSFunction, preserve the fallback initialization path instead of asserting or unchecked-downcasting it.
🤖 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 2072-2075: Update the prototype-walk logic around the descriptor
lookup and getPrototype call to capture the getOwnPropertyDescriptor result
before branching, invoke NAPI_RETURN_IF_EXCEPTION(env) unconditionally
immediately after the call, and only then decide whether to continue or exit the
walk. Ensure the loop condition does not consume the descriptor result before
exception handling.
In `@test/js/node/util/util.test.js`:
- Around line 155-159: Update the “propagates a throwing getPrototypeOf trap”
test to capture the exact Error instance thrown by the proxy’s getPrototypeOf
trap, invoke util.isError(proxy) inside the assertion, and verify the caught
value is identical to that instance with toBe rather than matching only its
message.
---
Outside diff comments:
In `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Around line 2315-2321: Update the lazy initializer around nodeUtilValue and
prop to validate node:util.inspect with dynamicDowncast<JSFunction> before
assigning it. If the value is not a JSFunction, preserve the fallback
initialization path instead of asserting or unchecked-downcasting it.
🪄 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: fb0c55df-6288-43a0-8591-e948ccb88585
📒 Files selected for processing (4)
src/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/napi.cppsrc/jsc/modules/NodeUtilTypesModule.cpptest/js/node/util/util.test.js
|
All three applied in 6e61c5f:
console-log (8), util isError, and napi property suites pass locally. |
|
Done in fcf96c4. The isError scenarios now run in a spawned child like the console-log ones, since before the fix even the non-throwing trap was a crashing input (the VMInquiry slot aborted any trap invocation), and a regression should fail one test instead of killing the whole util file. The child verifies all three cases and prints markers the test asserts exactly: the thrown error is the identical instance (keeping the earlier identity check), a trap returning Error.prototype answers true (the spot check that previously only existed in my comment), and a null prototype answers false. |
There was a problem hiding this comment.
I reviewed this PR across four iterations and the last outstanding item (spawning the util.isError Proxy-trap crash inputs in a subprocess, plus the Error.prototype spot-check) is now addressed in fcf96c4. No new issues found this pass. Because the change restructures two LazyProperty initializers in ZigGlobalObject.cpp around the must-always-init.set() invariant and touches exception-scope handling across bindings.cpp / napi.cpp / NodeUtilTypesModule.cpp, a maintainer look would still be worthwhile.
What was reviewed:
forEachPropertyImplclear-before-continue and thegetPrototypeempty-value guard — matches the ordered variant's handling.- The
m_utilInspectFunction/m_utilInspectStylizeColorFunctionfallback paths —assertNoExceptionExceptTerminationmatches file convention, termination is skipped beforeprofiledCall, and both initializers now unconditionallyinit.set. jsFunctionIsErrorVMInquiry-slot scoping and theRETURN_IF_EXCEPTIONaftergetPrototype; the spawned test covers throwing,Error.prototype-returning, and null-returning traps.napi_get_all_property_names: bothgetOwnPropertyDescriptorarms and thegetPrototypeclimb now check exceptions before use.
Extended reasoning...
Overview
Nine files: the core fix in bindings.cpp (JSC__JSValue__forEachPropertyImpl — clear pending exception before continue on not-found, and guard the empty return of a throwing getPrototypeOf trap in the prototype walk); ZigGlobalObject.cpp restructures the m_utilInspectFunction and m_utilInspectStylizeColorFunction LazyProperty initializers so they always init.set() (identity fallback) even when requireId(NodeUtil) throws or util.inspect was replaced with a non-function, with assertNoExceptionExceptTermination and a termination-skip before entering JS; BunObject.cpp drops two debug-only reportUncaughtExceptionAtEventLoop calls that ran with the exception still pending; NodeUtilTypesModule.cpp scopes the VMInquiry PropertySlot in jsFunctionIsError so it dies before the Proxy getPrototypeOf trap fires, and adds RETURN_IF_EXCEPTION after getPrototype; napi.cpp adds NAPI_RETURN_IF_EXCEPTION after getOwnPropertyDescriptor in both arms and after getPrototype in the climb; JSEnvironmentVariableMap.cpp + ProcessObjectInternals.ts pass the nodejs.util.inspect.custom symbol into the Windows-env builtin instead of reading Bun.inspect.custom mid-lookup. Four spawned regression tests in console-log.test.ts and one in util.test.js.
Security risks
None. The change hardens against user-tampered globals and adversarial Proxy traps; it removes crash paths rather than adding attack surface. No auth/crypto/permissions code touched.
Level of scrutiny
High — this is JSC exception-scope handling and LazyProperty initializer semantics, where a mistake is a debug abort or a silent stale-exception leak. That scrutiny has already been applied: four prior review rounds on this PR each surfaced a real gap (missing getPrototype guards in napi.cpp and NodeUtilTypesModule.cpp, bare assertNoException vs termination, unguarded uncheckedDowncast on a mutable util.inspect export, in-process crash-input test), and each was fixed with a verified test. The multi-agent bug hunter found nothing this pass; the one candidate raised (Windows \r\n in the spawned util.test.js assertion) was examined and refuted.
Other factors
The PR description's evidence block shows the new tests fail on unfixed debug/ASAN and release builds and pass with the fix. All four of my prior inline comments are marked resolved with corresponding commits. The remaining reason to defer rather than approve is the LazyProperty initializer restructuring: the "must always init.set() even on exception (callFunc asserts)" invariant and the termination-exception interaction are subtle enough that a maintainer familiar with LazyProperty::callFunc should confirm the fallback shape is what they want cached for the process lifetime.
There was a problem hiding this comment.
I reviewed the fcf96c4 revision and didn't find any new issues — the spawned util.isError fixture now covers the throwing trap, the Error.prototype-returning trap, and the null case, which was the last open item. Given the breadth here (LazyProperty initializer contracts and exception-scope handling across five C++ binding files, plus the new functionUtilInspectFallback design for tampered-util cases), a maintainer sign-off would still be worthwhile.
Checked this pass: the assertNoExceptionExceptTermination swap landed and the surviving-termination branch skips profiledCall; both napi_key_* arms and the loop-condition descriptor call now guard on exception; the VMInquiry slot in jsFunctionIsError is scoped dead before getPrototype; the Windows inspectCustom symbol is created directly from vm.symbolRegistry() so it does not touch the Bun object.
Extended reasoning...
Overview
Fixes a fuzzer-found debug abort where a stale pending exception from a throwing lazy PropertyCallback (e.g. Bun.$ when globalThis.Symbol is clobbered) leaked past the not-found continue in JSC__JSValue__forEachPropertyImpl into the next property's initializer. Scope grew across review iterations to cover the whole class: throwing getPrototypeOf traps in the same walk, in napi_get_all_property_names, and in util.isError; the m_utilInspectFunction / m_utilInspectStylizeColorFunction LazyProperty initializers now always init.set() (falling back to an identity host function when node:util fails to load or util.inspect is replaced); the debug-only reportUncaughtExceptionAtEventLoop calls in the Bun.sql callbacks are dropped; and the Windows process.env builtin receives the inspect-custom symbol as an argument instead of reading Bun.inspect.custom mid-lookup. Five subprocess regression tests cover the crashing inputs.
Security risks
None identified. The changes harden exception handling against adversarial user input (tampered globals, throwing Proxy traps), and the fallback path only degrades formatting to identity — no auth, crypto, or permission surface is touched.
Level of scrutiny
High. This is JSC binding C++ touching exception-scope discipline, LazyProperty initializer contracts ("must set even on exception"), DisallowVMEntry lifetime via VMInquiry slots, and termination-exception handling. It took six rounds of review feedback to converge (missing sibling arms, wrong assertNoException variant, unscoped VMInquiry slot, unspawned crash fixture), each of which was a real defect. That iteration count and the introduction of a new fallback-behavior design (functionUtilInspectFallback with an empty styles object) put this outside what I'd auto-approve.
Other factors
All prior inline comments are resolved and the evidence block shows the tests fail on an unfixed ASAN build and pass on both debug-ASAN and release with the fix. The napi change has no dedicated adversarial-trap test (robobun explained the parity-test count is engine-internal and would flake), which a maintainer may want to weigh. The Windows-only JSEnvironmentVariableMap.cpp / ProcessObjectInternals.ts change is straightforward but only exercised on the Windows CI lane.
fcf96c4 to
e10778e
Compare
…shing The VMInquiry PropertySlot used for the toStringTag check forbids entering the VM for as long as it is alive, so a Proxy getPrototypeOf trap reached by the getPrototype() call below it aborted, and the empty value returned by a throwing trap was used as a cell. Scope the slot to the tag check and check for an exception after getPrototype().
e10778e to
09f65a0
Compare
robobun
changed the title
|
Narrowed this PR down to the util.isError fix. The console.log / Bun.inspect property walk fix, the napi_get_all_property_names checks and the BunObject.cpp debug-only report are consolidated in #29642; the util.inspect lazy-property fix is being kept in #39382; the windowsEnv change only worked around the stale-structure assertion that #37001 fixes in JSC. The util.isError change and its test are unchanged apart from the rebase. |
| // node util.isError relies on toString | ||
| // https://github.com/nodejs/node/blob/cf8c6994e0f764af02da4fa70bc5962142181bf3/doc/api/util.md#L2923 | ||
| // util.isError is deprecated and removed in node 23 | ||
| PropertySlot slot(object, PropertySlot::InternalMethodType::VMInquiry, &vm); | ||
| bool has = object->getPropertySlot(globalObject, vm.propertyNames->toStringTagSymbol, slot); | ||
| scope.assertNoException(); | ||
| if (has) { | ||
| if (slot.isValue()) { | ||
| JSValue value = slot.getValue(globalObject, vm.propertyNames->toStringTagSymbol); | ||
| if (value.isString()) { | ||
| String tag = asString(value)->value(globalObject); | ||
| CLEAR_IF_EXCEPTION(scope); | ||
| if (tag == "Error"_s) | ||
| return JSValue::encode(jsBoolean(true)); | ||
| { | ||
| PropertySlot slot(object, PropertySlot::InternalMethodType::VMInquiry, &vm); | ||
| bool has = object->getPropertySlot(globalObject, vm.propertyNames->toStringTagSymbol, slot); | ||
| scope.assertNoException(); | ||
| if (has) { | ||
| if (slot.isValue()) { | ||
| JSValue value = slot.getValue(globalObject, vm.propertyNames->toStringTagSymbol); | ||
| if (value.isString()) { | ||
| String tag = asString(value)->value(globalObject); | ||
| CLEAR_IF_EXCEPTION(scope); | ||
| if (tag == "Error"_s) | ||
| return JSValue::encode(jsBoolean(true)); | ||
| } | ||
| } | ||
| } | ||
| // The VMInquiry slot disallows VM entry while alive; the Proxy trap below needs it dead. | ||
| } | ||
|
|
||
| JSValue proto = object->getPrototype(globalObject); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| if (proto.isCell() && (proto.inherits<JSC::ErrorInstance>() || proto.asCell()->type() == ErrorInstanceType || proto.inherits<JSC::ErrorPrototype>())) |
There was a problem hiding this comment.
🔴 The rebase/squash dropped 7 of the 9 files this PR describes: the head commit (09f65a0) touches only NodeUtilTypesModule.cpp + util.test.js, but the title, description, evidence block ("9 files touched", four passing console-log tests at lines 154–240), and every resolved review thread describe the bindings.cpp / ZigGlobalObject.cpp / napi.cpp / ProcessObjectInternals.ts / console-log.test.ts changes that are absent from the diff and not on main. Merging as-is closes fuzzer fingerprint 79443a4a1351567b without fixing it (globalThis.Symbol = NaN; console.log(Bun) still aborts at bindings.cpp:5646–5649), and CI won't catch it because the console-log regression tests were dropped with the fix. Please either restore the 7 dropped files or retitle/redescribe to the util.isError-only scope.
Extended reasoning...
What the finding is
The PR title is "Fix stale pending exceptions in console.log property iteration" and the description opens with "Fixes a fuzzer-found abort (fingerprint 79443a4a1351567b)", listing a 9-file diff-hotspot with 224 insertions. The evidence block shows four console-log.test.ts tests passing at lines 154–240 and cites a Rebase note claiming "Everything else applied cleanly and the suites above were rerun on the new base."
But git show --stat 09f65a0b (the squashed head, and the entirety of what get_pr_diff returns) is:
util.isError: propagate a throwing getPrototypeOf trap instead of crashing
src/jsc/modules/NodeUtilTypesModule.cpp | 26 ++++++++++++++----------
test/js/node/util/util.test.js | 36 ++++++++++++++++++++++++++++++++-
2 files changed, 50 insertions(+), 12 deletions(-)
The head commit's own message describes only the util.isError fix. Seven of the nine described files are absent, including the titular fix and its regression tests.
Step-by-step proof: each described change is absent from the diff and not on main
Verified against HEAD (the PR checkout) and git log -- <file> on main:
-
bindings.cpp— the PR's titular fix. Lines 5646–5649 still read:if (!object->getPropertySlot(globalObject, property, slot)) continue; // Ignore exceptions from "Get" proxy traps. CLEAR_IF_EXCEPTION(scope);
The not-found branch
continues past theCLEAR_IF_EXCEPTION— exactly the bug the title says is fixed. The reproducerglobalThis.Symbol = NaN; console.log(Bun);still aborts debug builds. -
test/js/web/console/console-log.test.ts.wc -lreports 152 lines; the file ends at the SharedArrayBuffer test. The four spawned regression tests the evidence block shows as(pass)at lines 154–240 ("console.log(Bun) survives lazy properties whose initializer throws", "stops the prototype walk when a getPrototypeOf trap throws", the two stylize tests) do not exist. The evidence block is showing pass output for tests that aren't in the tree. -
ZigGlobalObject.cpp. Lines 2310/2313/2337/2345/2350 stillRETURN_IF_EXCEPTION(scope, )withoutinit.set, and line 2315 stilluncheckedDowncast<JSFunction>(prop). The identity-fallback /dynamicDowncast/assertNoExceptionExceptTerminationhardening — the subject of three resolved review threads on this PR — is absent. -
napi.cpp. Lines 2079–2088 still readwhile (!owner->getOwnPropertyDescriptor(...)) { JSObject* proto = owner->getPrototype(globalObject).getObject(); ... } else { owner->getOwnPropertyDescriptor(...); }with noNAPI_RETURN_IF_EXCEPTIONafter eithergetOwnPropertyDescriptorcall or aftergetPrototype— the subject of two resolved review threads. -
ProcessObjectInternals.ts:453. Still readsBun.inspect.custominsidewindowsEnv. The Rebase note says the only conflict was here and was resolved by "appending the inspect symbol argument after"resetForDelete— but noinspectCustomparameter exists. The very conflict the note claims to have resolved lost the fix. -
BunObject.cpp/JSEnvironmentVariableMap.cpp.git logon both shows only unrelated commits; the described removals/additions are not present.
Why this is not covered by anything else
- These fixes did not land separately on main:
git logon each of the 7 dropped files shows only unrelated commits (Drop the DOMWrapperWorld wrapper HashMap #39334, bake: name both flags at every insert_stale_extra call #39171). - CI will not catch the missing titular fix because the
console-log.test.tsregression tests that would have caught it were dropped in the same squash. - The description's Rebase note explicitly asserts the opposite of what happened ("Everything else applied cleanly and the suites above were rerun on the new base"), so a reviewer approving on the description would close fingerprint
79443a4a1351567bwhile the crash remains.
Impact
Merging as-is:
- Closes a fuzzer fingerprint for a debug abort / release output-truncation bug that is not fixed.
- Ships a PR whose title, description, evidence block, and every resolved review thread describe changes that are not in the diff — CLAUDE.md: "Be humble & honest — NEVER overstate what you got done"; REVIEW.md: "Include every reproduction from the linked issue" and "Every behavioral change ships an automated test in the same PR."
- Leaves the LazyProperty contract violation (return without
init.set), the napi prototype-climb null-cell dereference, and the Windows env mid-reification abort — each raised and marked resolved in review — unfixed with their threads closed.
What is in the diff (the util.isError VMInquiry-scope fix + RETURN_IF_EXCEPTION after getPrototype, with a spawned regression test) is coherent and correct on its own.
How to fix
Two valid paths:
- Restore the dropped 7 files from the pre-squash branch (bindings.cpp, ZigGlobalObject.cpp, napi.cpp, BunObject.cpp, JSEnvironmentVariableMap.cpp, ProcessObjectInternals.ts, console-log.test.ts) and re-run the two suites the evidence block references so it reflects the actual head; or
- Retitle to "util.isError: propagate a throwing getPrototypeOf trap instead of crashing" (matching the head commit), replace the description with the util.isError-only scope, drop the fuzzer-fingerprint "Fixes" claim and the stale evidence block, and open a follow-up for the console.log / LazyProperty / napi / Windows-env fixes.
Why normal, not nit
The default "PR-description mismatch is nit" rule covers stale/imprecise descriptions where the code is what was intended. Here 7 of 9 files — including the fix the PR is named after — went missing during a rebase the description explicitly claims preserved them, the evidence block shows pass output for tests that don't exist, and merging spuriously closes a tracked crash with no CI guard. That is a concrete failure the author must resolve before merge, not a wording tweak.
|
This review ran against the pre-retitle state. The narrowing was deliberate and is already reflected on the PR: the title now matches the head commit, and the description covers only the util.isError fix, with a History section pointing to where the rest went. The property walk fix, the napi climb and the BunObject.cpp debug calls are in #29642, the util.inspect lazy property hardening is in #39382, and the windowsEnv change was dropped because #37001 fixes the stale-structure abort it was working around at the JSC level. The old evidence block and rebase note were removed along with the old description, so nothing on the PR still claims the console.log fix or the fuzzer fingerprint; those belong to #29642 now. What remains here is the one fix none of those PRs cover, with its spawned test, and CI on the narrowed commit is green apart from the usual flaky retries. |
|
Closing in favor of #36913, which has been open since August 4 with the same two fixes in |
… like Node (#39418) ### Problem - `napi_get_prototype` on a Proxy runs the Proxy's `getPrototypeOf` trap. Node does not: V8's `Object::GetPrototype`, which Node's `napi_get_prototype` wraps, cannot run JS and returns `null` for any Proxy (verified with Node 26.3, output in the details below). - Running the trap also makes the failure mode wrong: when the trap throws (or returns a non-object, or the proxy is revoked) the call returns `napi_ok`, writes a NULL `napi_value` into `*result`, and leaves the exception pending without telling the addon. Current canary on the new fixture: `trap throws: status=0 pending=true result=null handle exception=the trap's error`. - This is an unreleased regression from #33731 (86caf6e): `napi_get_prototype` used to call the JSC C API's `JSObjectGetPrototype`, which is `getPrototypeDirect()`, and JSC creates Proxy structures with a null prototype, so 1.3.14 already returned `null` for a Proxy without running anything. #33731 replaced it with `JSValue::get_prototype`, JSC's full `[[GetPrototypeOf]]` (traps included), which returned a bare `JSValue` that is empty (encoded 0, i.e. a NULL handle) when it threw, so the call site in `src/runtime/napi/napi_body.rs` could not tell success from failure. Affects 1.4.0 canary only; the `napi_get_prototype` line in the #36801 audit ("already avoids the trap") describes the pre-#33731 state. - The other `getPrototype` exception-handling sites found by the same reading (the inspect property walk, `napi_get_all_property_names`, `util.isError`) are separate fixes in #29642, #32263 and #37202; this PR merges cleanly in either order with them. ### Fix - `napi_get_prototype` returns `null` for a `ProxyObject` without touching its handler, the same result Node produces and the same result 1.3.14 produced. Every other object type's `[[GetPrototypeOf]]` in JSC is a plain read (`JSGlobalProxy` forwards to the global object, `ImportMetaObject` returns null, primitives get their wrapper prototype), so the function no longer runs JS for any input, like Node's. - `JSValue::get_prototype` (`src/jsc/JSValue.rs`) now returns `JsResult<JSValue>` through `host_fn::from_js_host_call`, the wrapper `JSValue::call` and `unwrap_boxed_primitive` already use for JSC calls that return empty exactly when they threw. This is what let the empty value reach `*result`, and the remaining callers (`ConsoleObject.rs`, `pretty_format.rs`, `ScopeFunctions.rs`, all already returning `JsResult`) now propagate with `?` instead of holding a possibly-empty value. They only ever see ordinary objects (the formatters print a Proxy's target), so their output is unchanged. In `napi_get_prototype` the `Err` arm maps to `napi_pending_exception` as the generic Node-API protocol; with the Proxy case handled above it is not reachable with today's object types. - Test: `test/napi/napi.test.ts` (`napi_get_prototype`) runs fixture `test_napi_get_prototype_proxy` (`napi-app/module.js`) through `checkSameOutput`, so the output is compared byte for byte with Node, and then pins the lines. The helper `perform_get_prototype` (`napi-app/js_test_helpers.cpp`) pre-fills `*result` with a sentinel so the output distinguishes a real `null` from "not written" and from a NULL handle. Cases: proxy without traps, callable proxy, a trap that counts its calls (must stay 0), a trap that throws, a trap returning a number, a revoked proxy, plus plain / null-prototype objects and an object whose prototype is a proxy (returned as-is, only the object itself is special-cased). - Fails under `USE_SYSTEM_BUN=1` (1.4.0-canary.1: 7 of 11 lines differ from Node, see details), passes with `bun bd test test/napi/napi.test.ts`. - Also run on the debug build: the rest of `test/napi/napi.test.ts`, Node's own `test_general` suite (`test/napi/node-napi-tests/.../test_general/do.test.ts`, which asserts `napi_get_prototype` matches `Object.getPrototypeOf` for ordinary objects), `test/js/bun/util/inspect.test.js`, `test/js/bun/test/printing/diffexample.test.ts`, `jest-each.test.ts`, `describe.test.ts`, `pretty-format-overflow.test.ts`, `console-table.test.ts`; plus a script formatting classes, functions, null-prototype objects and proxies, and a `bun test` file exercising `describe.each` / `test.each` / `skipIf` and class-instance `toEqual` diffs, both clean under `BUN_JSC_validateExceptionChecks=1`. - Noted while reviewing the other Proxy-receiver paths, not changed here (different function and file): `napi_remove_wrap` on a Proxy or on `globalThis` reports success but leaves the wrap attached, because `src/jsc/bindings/napi.cpp` removes it with the virtual `deleteProperty` (which `ProxyObject` refuses for private names and `JSGlobalProxy` forwards to its target) while `napi_wrap` / `napi_unwrap` use `putDirect` / `getDirect`; Node removes it. Probe output is in the second details block. ### Background - `[[GetPrototypeOf]]` is the spec operation behind `Object.getPrototypeOf`. For ordinary objects it reads a field and cannot fail; a Proxy implements it by calling its handler's `getPrototypeOf` trap, so it can run arbitrary JS and throw. V8's `Object::GetPrototype` returns a plain `Local<Value>` rather than a `MaybeLocal`, so it cannot run a trap, and it reports `null` for a Proxy; that is the behavior Node-API addons are written against. - `getPrototypeDirect()` is JSC's raw read of the prototype stored in an object's Structure, bypassing any override; `getPrototype()` is the full operation that dispatches to `ProxyObject`'s trap. The JSC C API's `JSObjectGetPrototype` is the former. - JSC signals a throw from a value-returning operation by returning the empty `JSValue` (encoding 0) and leaving the exception on the VM. `host_fn::from_js_host_call` maps that to `Err(JsError::Thrown)` and, in debug builds, asserts the value is empty if and only if an exception is pending. - A `napi_value` is an encoded `JSValue`, so storing the empty value hands the addon a NULL handle. - `napi_pending_exception` is the status a Node-API call returns when JS it ran threw; the exception stays pending for `napi_get_and_clear_last_exception` and out-params are left unwritten. <details> <summary>Fixture output: Node 26.3 (and this branch, identical) vs current canary</summary> Node v26.3.0, and this branch: ``` plain object: status=0 pending=false result=Object.prototype exception=none null prototype: status=0 pending=false result=null exception=none proxy without traps: status=0 pending=false result=null exception=none callable proxy: status=0 pending=false result=null exception=none trap returns Array.prototype: status=0 pending=false result=null exception=none getPrototypeOf trap calls: 0 trap throws: status=0 pending=false result=null exception=none trap returns a number: status=0 pending=false result=null exception=none revoked proxy: status=0 pending=false result=null exception=none object whose prototype is a proxy: status=0 pending=false result=the proxy exception=none plain object again: status=0 pending=false result=Object.prototype exception=none ``` Bun 1.4.0-canary.1 (`USE_SYSTEM_BUN=1`): ``` plain object: status=0 pending=false result=Object.prototype exception=none null prototype: status=0 pending=false result=null exception=none proxy without traps: status=0 pending=false result=Array.prototype exception=none callable proxy: status=0 pending=false result=Function.prototype exception=none trap returns Array.prototype: status=0 pending=false result=Array.prototype exception=none getPrototypeOf trap calls: 1 trap throws: status=0 pending=true result=null handle exception=the trap's error trap returns a number: status=0 pending=true result=null handle exception=TypeError revoked proxy: status=0 pending=true result=null handle exception=TypeError object whose prototype is a proxy: status=0 pending=false result=the proxy exception=none plain object again: status=0 pending=false result=Object.prototype exception=none ``` </details> <details> <summary>napi_remove_wrap follow-up: probe output (existing try_wrap / try_remove_wrap / try_unwrap helpers from napi-app, wrapping the number 6, then trying to re-wrap with 7)</summary> Node v26.3.0: ``` plain: wrap=true remove=6 unwrap_after=undefined rewrap=true proxy: wrap=true remove=6 unwrap_after=undefined rewrap=true callable proxy: wrap=true remove=6 unwrap_after=undefined rewrap=true globalThis: wrap=true remove=6 unwrap_after=undefined rewrap=true ``` Bun (this branch, unchanged in this respect): ``` plain: wrap=true remove=6 unwrap_after=undefined rewrap=true proxy: wrap=true remove=6 unwrap_after=6 rewrap=false callable proxy: wrap=true remove=6 unwrap_after=6 rewrap=false globalThis: wrap=true remove=6 unwrap_after=6 rewrap=false ``` </details> <details> <summary>Earlier version of this PR</summary> The first revision kept running the trap and changed only the failure report: `napi_pending_exception` with `*result` untouched when the trap threw, tested on Bun alone since Node never reaches that path. Review asked to match Node instead and not run JS, which is the current shape; the `JsResult` change to `JSValue::get_prototype` is unchanged from that revision. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts <!-- robobun:evidence:end -->
…aps (#36913) ### Problem - `util.isError()` crashes the process instead of returning or throwing when given a revoked Proxy or a Proxy with a `getPrototypeOf` trap. Release builds die with `panic(main thread): Segmentation fault at address 0x0`; debug builds abort in `VM::checkVMEntryPermission()`. ```js const r = Proxy.revocable({}, {}); r.revoke(); require("node:util").isError(r.proxy); // segfault require("node:util").isError(new Proxy({}, { getPrototypeOf() { throw 1; } })); // segfault require("node:util").isError(new Proxy({}, { getPrototypeOf: () => Error.prototype })); // segfault too ``` - The deprecated API is not the only way in: `internal/streams/iter/utils.ts` (`wrapError`) runs every value a source throws through `util.isError`, so a program that throws such a Proxy into `node:stream/iter` segfaults too (probe in the details block below). - Cause 1, `jsFunctionIsError` in `src/jsc/modules/NodeUtilTypesModule.cpp`: the result of `object->getPrototype(globalObject)` is used with no exception check. When the call throws (revoked proxy, throwing trap) it returns the empty `JSValue`, which passes `isCell()`, so `proto.inherits<ErrorInstance>()` reads the structure of a null cell. - Cause 2, same function: the `VMInquiry` `PropertySlot` used a few lines earlier for the `Symbol.toStringTag` lookup is still alive when `getPrototype()` runs, so its `DisallowVMEntry` is still in force when the Proxy tries to call the trap. Debug builds crash there; release builds skip the trap and get `undefined` back, which the Proxy turns into a TypeError, which then feeds cause 1. This is why even a well behaved trap (returning `Error.prototype` or `null`) crashed. ### Fix - `slot.disallowVMEntry.reset()` once the `toStringTag` lookup has returned. This is the idiom JSC itself uses when a VMInquiry slot is followed by code that may run JS (`JSScope.cpp`, `JSGlobalObject.cpp`, `JSInjectedScriptHost.cpp`). The only later use of the slot is `getValue()` on a slot that `isValue()`, which reads a stored value and does not enter the VM. - `RETURN_IF_EXCEPTION` after `getPrototype()`, so a revoked proxy throws its TypeError and a throwing trap rethrows the trap's own value. - Why this is the right behavior: Node's legacy `util.isError(e)` ends in `e instanceof Error`, which performs the same `[[GetPrototypeOf]]` on the proxy. It throws on a revoked proxy, propagates the trap's exception unchanged, and honors a trap's return value (`null` is not an Error, `Error.prototype` is). The new test asserts exactly those outcomes. - Verified with `test/js/node/util/util.test.js`, `util > isError > handles revoked proxies and getPrototypeOf traps`. The crashing inputs run in a child process, one line of output per case: revoked proxy throws a TypeError, a throwing trap propagates its Error, a trap throwing a plain object rethrows that same object (`caught === thrown`), `null` trap gives `false`, `Error.prototype` trap gives `true`, a Proxy wrapping an Error gives `true`. - Debug build with `NodeUtilTypesModule.cpp` reverted to main: the test fails (the child aborts); the same-object case run on its own aborts with exit 134. - Debug build with the fix: the file passes (209 tests). - `USE_SYSTEM_BUN=1` (release 1.4.0 canary): the test fails, the child segfaults. This PR replaces #37202, which fixed the same two problems in the same function; its extra test case (the trap's thrown value comes back as the same object) has been folded into the test here. Not changed here: `jsFunctionIsError` still only looks one prototype level deep and at `Symbol.toStringTag`, which differs from Node's `toString(e) === "[object Error]" || e instanceof Error` for some non-crashing inputs. That is a behavior question separate from this crash and is left as is. ### Background - `PropertySlot` with `InternalMethodType::VMInquiry` asks JSC for a lookup with no observable side effects (no getters, no Proxy traps). To enforce that, the slot holds a `DisallowVMEntry` token for its whole lifetime; while any such token exists, JSC refuses to enter the VM to run JavaScript (debug: crash, release: the call is skipped and yields `undefined`). - A throwing JSC host call does not unwind. It records the exception on the VM and returns a placeholder (the empty `JSValue` here); the caller has to check the throw scope (`RETURN_IF_EXCEPTION`) before touching the result. - The empty `JSValue` is encoded as 0, which the `isCell()` fast check accepts, so using it as a cell is a null dereference. <details><summary>Probe: reaching the crash through node:stream/iter</summary> ```js // bun --experimental-stream-iter repro.js const { share } = require("node:stream/iter"); const poison = new Proxy({}, { getPrototypeOf() { throw new Error("trap"); } }); async function* src() { throw poison; } (async () => { try { for await (const x of share(src()).pull()) {} } catch (e) { console.log("caught:", String(e)); } })(); ``` bun 1.4.0 canary (8326d1b): `panic(main thread): Segmentation fault at address 0x0`, exit 139. With this PR: `caught: Error: trap`, exit 0. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 5 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/util/util.test.js bun test v1.4.0 (8326d1b) test/js/node/util/util.test.js: (pass) util > toUSVString [5.02ms] (pass) util > inherits [5.33ms] (pass) util > isArray > all cases [7.18ms] (pass) util > isRegExp > all cases [4.38ms] (pass) util > isDate > all cases [165.31ms] (pass) util > isError > all cases [15.19ms] 185 | env: bunEnv, 186 | stdout: "pipe", 187 | stderr: "pipe", 188 | }); 189 | const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); 190 | expect(stderr).toBe(""); ^ error: expect(received).toBe(expected) - "" + "/root/.bun/build-cache/webkit-c6cfe90c6064bd80-debug-asan/include/JavaScriptCore/JSCJSValueStructure.h:45:34: runtime error: member call on null pointer of type 'JSC::JSCell' + SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /root/.bun/build-cache/webkit-c6cfe90c6064bd80-debug-asan/include/JavaScriptCore/JSCJSValueStructure.h:45:34 + " - Expected - 1 + Received + 3 ... (truncated) release without fix: all passed bun test v1.4.0-canary.1 (2897716) test/js/node/util/util.test.js: (pass) util > toUSVString [0.11ms] (pass) util > inherits [0.10ms] (pass) util > isArray > all cases [0.10ms] (pass) util > isRegExp > all cases [0.05ms] (pass) util > isDate > all cases [6.04ms] (pass) util > isError > all cases [0.32ms] (pass) util > isError > handles revoked proxies and getPrototypeOf traps [17.19ms] (pass) util > isObject > all cases [0.11ms] (pass) util > isPrimitive > all cases [0.11ms] (pass) util > isBuffer > all cases [0.05ms] (pass) util > _extend > all cases [0.13ms] (pass) util > isBoolean > all cases [0.04ms] (pass) util > isNull > all cases [0.04ms] (pass) util > isUndefined > all cases [0.03ms] (pass) util > isNullOrUndefined > all cases [0.03ms] (pass) util > isNumber > all cases [0.03ms] (pass) util > isString > all cases [0.02ms] (pass) util > isSymbol > all cases [0.03ms] (pass) util > isFunction > all cases [0.04ms] (pass) util > types.isNativeError > all cases [0.07ms] (pass) util > TextEncoder > is same as global TextEncoder [0.02ms] (pass) util > TextDecoder > is same as global TextDecoder [0.02ms] (pass) util > format [0.23ms] (pass) util > formatWithOption ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/util/util.test.js bun test v1.4.0 (8326d1b) test/js/node/util/util.test.js: (pass) util > toUSVString [4.96ms] (pass) util > inherits [5.58ms] (pass) util > isArray > all cases [7.00ms] (pass) util > isRegExp > all cases [3.70ms] (pass) util > isDate > all cases [175.40ms] (pass) util > isError > all cases [17.81ms] (pass) util > isError > handles revoked proxies and getPrototypeOf traps [852.71ms] (pass) util > isObject > all cases [4.70ms] (pass) util > isPrimitive > all cases [9.14ms] (pass) util > isBuffer > all cases [4.43ms] (pass) util > _extend > all cases [7.81ms] (pass) util > isBoolean > all cases [2.75ms] (pass) util > isNull > all cases [2.91ms] (pass) util > isUndefined > all cases [2.82ms] (pass) util > isNullOrUndefined > all cases [2.81ms] (pass) util > isNumber > all cases [2.70ms] (pass) util > isString > all cases [2.64ms] (pass) util > isSymbol > all cases [2.67ms] (pass) util > isFunction > all cases [3.09ms] (pass) util > types.isNativeError > all cases [4.29ms] (pass) util > TextEncoder > is same ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 871f486 features baseline 22 deps, 120 codegen, 1175 objects in 637ms ninja: Entering directory `/workspace/bun/build/release' [1/127] gen generated_host_exports.rs generated_host_exports.rs: 92 exports (host=3, lazy=10, generic=79, rust=0); 241 extern-C blocks audited [2/127] gen cpp.rs (cppbind) [3/127] gen BunProcess.lut.h Generating /workspace/bun/build/release/codegen/BunProcess.lut.h from /workspace/bun/src/jsc/bindings/BunProcess.cpp [4/127] gen JS modules (bundle-modules) Preprocess modules (8084ms) Bundle modules (45ms) Postprocesss modules (32ms) Bundle Functions (651ms) Generate Code (19ms) [8.85s] Bundled "src/js" for production 2631 kb 197 internal modules 13 native modules 91 internal functions across 17 files [4/127] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) ^[[1m^[[92m Compiling^[[0m std v0.0.0 (/root/.rustup/toolchains/nigh ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/jsc/modules/NodeUtilTypesModule.cpp | 3 ++ test/js/node/util/util.test.js | 51 ++++++++++++++++++++++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-) ``` </details> **gate history** · 4 passed · 1 rejected · iteration 5 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/jsc/modules/NodeUtilTypesModule.cpp 5 6 0 test/js/node/util/util.test.js 5 7 0 ``` </details> <!-- robobun:evidence:end -->
…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>
1 participant
Problem
util.isError(new Proxy({}, { getPrototypeOf() { throw err; } }))crashes the process: release builds withSegmentation fault at address 0x0, debug builds with an abort, instead of throwingerrthe wayerr instanceof Error(what Node'sutil.isErroris built on) does.jsFunctionIsError(src/jsc/modules/NodeUtilTypesModule.cpp):PropertySlotcreated withInternalMethodType::VMInquiryfor theSymbol.toStringTagcheck stays alive until the end of the function, and a VMInquiry slot forbids entering the VM for as long as it exists. Theobject->getPrototype(globalObject)call further down runs the Proxy'sgetPrototypeOftrap, so in debug builds the VM entry check aborts; in release builds the trap is not run at all, the call yieldsundefined, and the Proxy turns that into aTypeError(so even a well-behaved trap madeutil.isErrorfail).getPrototype()is used without checking for that exception. A throwinggetPrototype()returns the emptyJSValue, which passesisCell(), soproto.inherits<ErrorInstance>()reads the structure of a null cell.Fix
toStringTagcheck so it is destroyed beforegetPrototype()is called, and addRETURN_IF_EXCEPTIONaftergetPrototype()so the trap's error propagates to the caller.util.isError(x)ends inx instanceof Error, which runs the same trap and throws the same error.test/js/node/util/util.test.js,util > isError > handles Proxy getPrototypeOf traps(spawned child): the thrown error is the trap's error, and non-throwing traps returningError.prototype/nullgivetrue/false. Without the fix the child exits 139 (release) or 134 (debug); with it the test passes and the rest ofutil.test.js(209 tests) still passes.Background
PropertySlotwithInternalMethodType::VMInquiryasks JSC for a side-effect-free lookup; JSC enforces that by holding aDisallowVMEntrytoken for the slot's lifetime, which makes any attempt to run JavaScript while the slot is alive fail (assert in debug, aTypeError-style failure in release).JSValue) and the caller must check the scope before using the result.History
This PR originally also fixed the stale-exception /
getPrototypecrash in theBun.inspectproperty walk, the matching check innapi_get_all_property_names, theutil.inspectlazy properties, and theBunObject.cppdebug-only report. Those are consolidated in #29642 (property walk, napi,BunObject.cpp) and #39382 (util.inspectlazy properties); thewindowsEnvchange only worked around the stale-structure assertion that #37001 fixes in JSC. This PR has been narrowed to theutil.isErrorfix, which none of those cover. The sameutil.isErrorfix was also part of #37175 and #39412, both closed.