error: read name/message via full [[Get]] for the .stack header; drop inspect.js workaround - #34868
error: read name/message via full [[Get]] for the .stack header; drop inspect.js workaround#34868robobun wants to merge 9 commits into
Conversation
…eaders The native .stack getter already emits the correct first line (name followed by ': message' only when message is non-empty) for subclassed errors and empty-message errors, so the 2023 stack.replace workaround in formatError is no longer needed. Worse, it now corrupts output: when an Error's message is cleared after .stack was read, util.inspect rendered 'Errormsg' instead of 'Error: msg', and a user-assigned stack starting with 'Error: ' on an empty-message Error lost its ': '. Node never rewrites the stack header in util.inspect. Remove the workaround and restore the upstream Node assertion in util-inspect.test.js that it replaced. Add a test covering the cases the workaround mangled plus the subclass/empty-message cases it originally papered over.
|
Updated 10:39 PM PT - Jul 20th, 2026
✅ @robobun, your commit 1a36d38cb6ea6a77dc1e344080bc0cf176dc43e4 passed in 🧪 To try this PR locally: bunx bun-pr 34868That installs a local version of the PR into your bun-34868 --bun |
WalkthroughError stack formatting now preserves materialized headers, reads ChangesError stack formatting
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced with Self-review surfaced the sanitized-accessor gaps (deep-proto / accessor / non-primitive name), the CI: |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — deletes a stale workaround that now only fires when it corrupts output, and restores the upstream Node assertion it had relaxed. The inline nit about the sibling //! temp bug workaround in util-format.test.js is cosmetic (that .replace() is now a no-op) and doesn't block.
Extended reasoning...
Overview
Two files touched: src/js/internal/util/inspect.js deletes a single stack.replace(/^Error: /, ...) line and its //! temp fix comment from formatError; test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js restores the upstream Node assertion util.inspect(err) === err.stack (previously relaxed with a matching workaround) and adds a new test block covering the corruption cases plus the two scenarios the workaround originally papered over.
Security risks
None. This is purely a change to how util.inspect renders an error's already-materialized .stack string — no parsing of untrusted input, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-medium. The runtime change is a two-line deletion that removes a Bun-specific divergence from Node's formatError, which per REVIEW.md is the reference implementation for node:* compat. I traced the remaining path: getStackString → removeDuplicateErrorKeys → improveStack is now byte-for-byte the Node flow. I walked each new test case through improveStack (e.g. the err.name = "Renamed" case: name doesn't end with "Error" and constructor isn't null, so the stack passes through untouched and firstLine sees "Error: x") and they hold. The restored upstream loop assertion is strictly stronger than what it replaces.
Other factors
The PR description documents USE_SYSTEM_BUN=1 fails / bun bd passes for the new test, and that the surrounding inspect suites still pass. The one finding — a sibling //! temp bug workaround at util-format.test.js:436 — is a dead no-op (/^Error/ can't match a stack that now starts with CustomError), so it's stale-comment cleanup, not a behavioral gap. Worth sweeping in a follow-up commit but not a reason to hold the fix.
The sibling '//! temp bug workaround' in util-format.test.js papered over the same native stack-header bug. With the header now correct, customError.stack already begins with 'CustomError: bar', so the anchored /^Error/ replace was a dead no-op.
sanitizedNameString walks only self+prototype with VMInquiry and rejects non-primitive values, so a name set on an intermediate prototype, defined as an accessor, or requiring ToString fell back to 'Error'. The removed formatError workaround had been masking this in util.inspect by re-reading err.name. computeErrorInfoWithoutPrepareStackTrace now reads name and message via an ordinary [[Get]] with ToString, defaulting to 'Error' and the empty string for undefined, matching V8's ErrorUtils::ToString. The GC-finalizer path reaches this function with errorInstance == nullptr, so the [[Get]] only runs from a mutator. Root the captured frames' cells in errorConstructorFuncCaptureStackTrace's eager path before computing, since reading name/message can now run a user getter.
robobun
changed the title
There was a problem hiding this comment.
The util-format.test.js sibling from my earlier comment is now addressed. Beyond the inline finding, I checked the other three paths that reach the new full-[[Get]] header read: errorInstanceLazyStackCustomGetter already has the same protectedFrameCells rooting; computeErrorInfoWrapperToJSValue operates on ErrorInstance::m_stackTrace (visited by ErrorInstance); and the GC-finalizer hook computeErrorInfoWrapperToString passes a null errorInstance so the new branch never runs there.
Extended reasoning...
This run's inline finding covers the one remaining unrooted eager-compute path in errorConstructorFuncCaptureStackTrace (the non-ErrorInstance else-branch). I audited every other caller that now reaches errorInstance->get(name/message) via computeErrorInfoWithoutPrepareStackTrace and confirmed each either already roots the frame cells or holds them via a GC-visited owner, so hoisting the MarkedArgumentBuffer above the dynamicDowncast is the only remaining gap. The earlier util-format.test.js cleanup I flagged has been applied. Deferring rather than approving because this is native JSC/GC-adjacent code with a still-open memory-safety asymmetry.
…branch The non-ErrorInstance else-branch now also reads name/message via [[Get]] before formatting, so it needs the same rooting. Hoist the MarkedArgumentBuffer so both eager-compute paths share it.
The native .stack header now reads name via full [[Get]], so the BazError get-name case and the Foo-extends-TypeError cases produce the upstream Node output and the disabled assertion passes.
![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
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/FormatStackTraceForJS.cpp (1)
795-846: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftConsider a memory-safety regression test for this rooting fix.
The rooting added here protects
stackTrace'scallee()/codeBlock()cells from GC whilecomputeErrorInfoToJSValueruns username/messagegetters (this is most reachable via thehasMaterializedErrorInfo()branch at lines 812-824, wherestackTracestays local and isn't otherwise reachable). The added tests cover header-preservation and[[Get]]value semantics, but none appear to force a GC from inside aname/messagegetter duringError.captureStackTraceon an already-materialized error to actually exercise the UAF this rooting prevents.
As per path instructions,**/*.{test,spec}.{js,jsx,ts,tsx}: "crash, UAF, leak, and issue reproductions require the corresponding spawned fixture or memory-safety regression test." Would you like me to draft aBun.gc(true)-based regression test that triggers this path?🤖 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/FormatStackTraceForJS.cpp` around lines 795 - 846, Add a memory-safety regression test for the already-materialized ErrorInstance path in FormatStackTraceForJS, using Error.captureStackTrace and a name/message getter that forces GC via Bun.gc(true) while computeErrorInfoToJSValue is processing stackTrace. Ensure the fixture exercises the protected callee/codeBlock cells and verifies execution completes without a crash or UAF.Source: Path instructions
🤖 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/FormatStackTraceForJS.cpp`:
- Around line 795-811: Extract the duplicated frame-cell rooting sequence into a
shared helper near the existing stack-trace formatting utilities, including
capacity reservation, callee/codeBlock iteration, overflow detection, and OOM
handling. Replace the inline logic in both errorInstanceLazyStackCustomGetter
and the shown eager-compute path with calls to that helper, preserving each
caller’s existing failure behavior and rooted buffer lifetime.
---
Outside diff comments:
In `@src/jsc/bindings/FormatStackTraceForJS.cpp`:
- Around line 795-846: Add a memory-safety regression test for the
already-materialized ErrorInstance path in FormatStackTraceForJS, using
Error.captureStackTrace and a name/message getter that forces GC via
Bun.gc(true) while computeErrorInfoToJSValue is processing stackTrace. Ensure
the fixture exercises the protected callee/codeBlock cells and verifies
execution completes without a crash or UAF.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bafc4826-1090-43f7-8b58-299548dd21fb
📒 Files selected for processing (4)
src/js/internal/util/inspect.jssrc/jsc/bindings/FormatStackTraceForJS.cpptest/js/node/util/node-inspect-tests/parallel/util-format.test.jstest/js/node/util/node-inspect-tests/parallel/util-inspect.test.js
💤 Files with no reviewable changes (1)
- src/js/internal/util/inspect.js
Both call sites (errorInstanceLazyStackCustomGetter and errorConstructorFuncCaptureStackTrace) now go through one helper.
formatStackTraceToJSValue (the Error.prepareStackTrace default string)
still hardcoded 'Error' as the name, so removing the inspect.js workaround
regressed util.inspect when prepareStackTrace was set. Both paths now go
through one computeErrorHeader that does the full [[Get]].
A name/message getter that called Error.captureStackTrace(this) recursed
unboundedly because the already-materialized branch re-ran the [[Get]].
Guard with isComputingErrorStackHeader on Zig::GlobalObject; the inner
call falls back to the side-effect-free sanitized accessors so the cycle
terminates after one level, matching Node's single getter invocation.
Tests cover the prepareStackTrace path, the re-entry case, and
Error.captureStackTrace on a {name, message} literal.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/jsc/bindings/FormatStackTraceForJS.cpp:423-434— The newerrorInstance->get(name/message)can run a user getter that readsthis.stack; whenError.captureStackTrace(e)previously installed the lazy CustomAccessor on an unmaterializedErrorInstance, that reentrant read reacheserrorInstanceLazyStackCustomGetter, whosesetStackFrames(vm, {})reassignsm_stackTraceand destructs theVector<StackFrame>that the outermaterializeErrorInfoIfNeededcall'sstackTrace&still binds to —formatStackTraceat :442 then walks freed memory. Before this PR the header usedsanitizedNameString/sanitizedMessageString(VMInquiry, no accessors), so no user JS ran between bindingstackTrace&and formatting;DeferGCForAWhileand theMarkedArgumentBufferrooting don't help because this is a C++unique_ptrreassignment, not GC. One fix: havematerializeErrorInfoIfNeededmovem_stackTraceinto a local before invoking the callback (so the reentrant getter seesstackTrace() == nullptr), or copystackTraceinto a localVectorhere before the[[Get]]calls.Extended reasoning...
What the bug is
computeErrorInfoWithoutPrepareStackTracenow composes the.stackheader via full[[Get]]onname/message(FormatStackTraceForJS.cpp:423, :430). When this function is reached fromErrorInstance::materializeErrorInfoIfNeeded, itsVector<StackFrame>& stackTraceparameter is bound to*m_stackTrace.get()— the Vector owned by theErrorInstance'sunique_ptr<Vector<StackFrame>> m_stackTrace. A username/messagegetter that readsthis.stackcan reentererrorInstanceLazyStackCustomGetter, which callserrorObject->setStackFrames(vm, {}).setStackFramesreassignsm_stackTrace = WTF::move(newUniquePtr), destructing the previousVector<StackFrame>object. The outerstackTrace&now dangles, andBun::formatStackTrace(..., stackTrace, errorInstance)at :442 reads.size()/.at(i)on freed memory.Step-by-step proof
let n = 0; class T extends Error { get name() { if (n++ === 0) void this.stack; return "T"; } } const e = new T("m"); Error.captureStackTrace(e); // (0) e.stack; // (1) — UAF
(0)
errorConstructorFuncCaptureStackTrace:eis anErrorInstancewithhasMaterializedErrorInfo() == false, so it takes the lazy branch —instance->setStackFrames(vm, WTF::move(stackTrace))populatesm_stackTrace, thenJSObject::deleteProperty(instance, ...)(a direct static call, soErrorInstance::deleteProperty'smaterializeErrorInfoIfNeededis not invoked andm_errorInfoMaterializedstaysfalse), thenputDirectCustomAccessor(stack, m_lazyStackCustomGetterSetter)installs the lazy getter as an own property.(1)
e.stack→ErrorInstance::getOwnPropertySlotcallsmaterializeErrorInfoIfNeeded(vm, "stack")first. That function (oven-sh/WebKitErrorInstance.cpp,BUN_JSC_ADDITIONSbranch) seesm_errorInfoMaterialized == falseandm_stackTracenon-null/non-empty, so it setsm_errorInfoMaterialized = truebefore invoking the callback, then underDeferGCForAWhilecallsfn(vm, *m_stackTrace.get(), line, column, sourceURL, this, m_bunErrorData).m_stackTraceis only nulled afterfnreturns.(2)
fn→computeErrorInfoWrapperToJSValue→computeErrorInfoToJSValue→computeErrorInfoToJSValueWithoutSkipping→computeErrorInfoWithoutPrepareStackTrace, all threading the sameVector<StackFrame>&bound to*m_stackTrace. At :423,errorInstance->get(lexicalGlobalObject, vm.propertyNames->name)walks toT.prototypeand invokes the user getter withn == 0.(3) The getter reads
this.stack→ErrorInstance::getOwnPropertySlot→materializeErrorInfoIfNeedednow seesm_errorInfoMaterialized == trueand returnsfalseimmediately → falls through toBase::getOwnPropertySlot, which finds the own CustomAccessor installed in step (0) (the outerputDirect(stack, ...)hasn't run yet — we're still insidefn) → invokeserrorInstanceLazyStackCustomGetter.(4)
errorInstanceLazyStackCustomGetter:errorObject->stackTrace()returnsm_stackTrace.get(), which is still non-null. It doesauto ownedStackTrace = makeUnique<Vector<StackFrame>>(WTF::move(*stackTrace))(empties the outer Vector's contents), roots the frame cells, callscomputeErrorInfoToJSValue(withn == 1the getter returns"T"immediately, no further recursion), thenerrorObject->setStackFrames(vm, {}).ErrorInstance::setStackFramesdoesm_stackTrace = WTF::move(makeUnique<Vector<StackFrame>>({}))— the previousunique_ptr'sVector<StackFrame>object is destructed. The outerstackTrace&from step (2) now points to freed heap memory.(5) The getter returns
"T"; back in the outercomputeErrorInfoWithoutPrepareStackTrace, :430 readsmessage(own data property, no reentry), then :442 callsBun::formatStackTrace(..., stackTrace, errorInstance).formatStackTraceimmediately readsstackTrace.size()and iteratesstackTrace.at(i)— use-after-free.Why nothing existing prevents it
DeferGCForAWhileinmaterializeErrorInfoIfNeededguards against JSC GC; this is a plain C++unique_ptrreassignment on the WTF heap.- The PR's
MarkedArgumentBufferrooting inerrorConstructorFuncCaptureStackTrace(a) is on a stack frame that has already returned beforee.stackis read, and (b) protects the frames'JSCell*against GC, not theWTF::Vectorcontainer againstunique_ptrreassignment. m_errorInfoMaterializedcorrectly gates a reentrantError.captureStackTrace(this)(that would take the eager branch, which never touchesm_stackTrace), but it does not gate the CustomAccessor thatcaptureStackTrace's lazy branch installed before materialization —Base::getOwnPropertySlotfinds it and invokes it regardless.errorInstanceLazyStackCustomGetter'sstackTrace == nullptrcheck doesn't help:m_stackTraceis only nulled afterfnreturns.
Why this is new to the PR
Before, this branch called
instance->sanitizedNameString(lexicalGlobalObject)/sanitizedMessageString(...), which use aPropertySlotinVMInquirymode — accessors are never invoked, so no user JS could run between bindingstackTrace&and passing it toformatStackTrace. This PR replaces those with full[[Get]], opening the reentrancy window.Impact and fix
Memory-safety UAF reachable from plain user JS (
nameaccessor on an Error subclass +Error.captureStackTrace+.stack). Per REVIEW.md this is the most-blocked category: "Anything that can run user JS can synchronously free your state — re-validate liveness guards after every callback" / "Never let a pointer or slice outlive the memory it points into."Two viable fixes:
- WebKit side (preferred): in
materializeErrorInfoIfNeeded,auto owned = WTF::move(m_stackTrace)before callingfn, and pass*owned. The reentranterrorInstanceLazyStackCustomGetterthen seesstackTrace() == nullptrand takes the empty-trace branch; the outer reference stays valid for the callback's duration. - Bun side: in
computeErrorInfoWithoutPrepareStackTrace, whenerrorInstanceis anErrorInstancewhosestackTrace()aliases the incomingstackTrace&, move-copy the frames into a localVector<StackFrame>(with a localMarkedArgumentBufferrooting them) before the[[Get]]calls, and pass the local toformatStackTrace.
Either way, add an ASAN regression test exercising the repro above (per REVIEW.md: "UAF/leak fixes need an ASan repro on the unfixed build").
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/FormatStackTraceForJS.cpp`:
- Around line 49-52: Compress the comment immediately above the stack-formatting
logic to three lines or fewer while preserving its description of V8
name/message coercion, default values, and the re-entrant getter fallback that
terminates cycles after one level.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5aa5320d-ce6b-4295-b58e-8e82cb42380a
📒 Files selected for processing (3)
src/jsc/bindings/FormatStackTraceForJS.cppsrc/jsc/bindings/ZigGlobalObject.htest/js/node/util/node-inspect-tests/parallel/util-inspect.test.js
… header [[Get]]
A name/message getter that read this.stack after Error.captureStackTrace
had installed the lazy CustomAccessor reached
errorInstanceLazyStackCustomGetter while the outer materialize still held
a reference into *m_stackTrace; the inner move + setStackFrames({})
destructed that Vector and the outer formatStackTrace read freed memory
(ASAN heap-use-after-free under Malloc=1). Under isComputingErrorStackHeader
the inner call now formats from the existing Vector without moving or
reassigning it; computeErrorHeader already falls back to sanitized reads
under the same guard so no user code runs there.
Add a spawned Malloc=1 regression test in capture-stack-trace.test.js,
re-enable the second upstream [WOW] subclass-header assertion in
util-inspect.test.js, and trim two comments to three lines.
|
The UAF from a |
There was a problem hiding this comment.
No new issues found on 4cfb8ae; the four earlier inline findings are all addressed. Deferring to a human because this changes when arbitrary user JS can run during .stack materialization (full [[Get]] on name/message), with GC rooting, a per-global re-entry guard, and a UAF-avoidance branch in the lazy getter — memory-safety-critical enough to want maintainer eyes on the guard design and the materializeErrorInfoIfNeeded re-entry path.
What was reviewed:
computeErrorHeader: exception scopes after eachget/toWTFString, undefined-defaulting matches V8ErrorUtils::ToString, guard cleared viamakeScopeExiton all paths.- Frame-cell rooting:
protectStackFrameCellsnow covers both eagercaptureStackTracebranches and the lazy getter; the re-entry branch relies onErrorInstance::visitChildrenvisitingm_stackTrace(not moved/reset there). - The lazy-getter re-entry branch does not move out of or reset
m_stackTrace, so the outermaterializeErrorInfoIfNeeded'sVector&stays valid;Malloc=1regression test covers it. - Swept for remaining
//! temp/Bun messes withmarkers in the inspect test suite — none left.
Extended reasoning...
Overview
The PR replaces the sanitized (own + one prototype level, VMInquiry, no accessors) name/message reads in the .stack header with V8-semantics full [[Get]] + ToString, shared between computeErrorInfoWithoutPrepareStackTrace and formatStackTraceToJSValue via a new computeErrorHeader. Because that now runs arbitrary user JS before the frames are formatted, it adds (a) protectStackFrameCells rooting via MarkedArgumentBuffer at both eager captureStackTrace paths and the existing lazy getter, (b) a per-Zig::GlobalObject isComputingErrorStackHeader re-entry guard that falls back to sanitized reads on the inner level, and (c) a re-entry branch in errorInstanceLazyStackCustomGetter that formats from *m_stackTrace in place without moving/resetting it, avoiding a heap-use-after-free when a name getter reads this.stack while the outer materializeErrorInfoIfNeeded still holds a Vector<StackFrame>& into the same storage. The now-correct native header lets the 2023 stack.replace(/^Error: /, ...) workaround in inspect.js be deleted and three previously-relaxed/disabled upstream Node assertions be restored verbatim.
Security risks
No auth/crypto/permissions surface. The user-facing risk is memory safety: opening a GC window (user getters, toString) between capturing frames and formatting them, and the m_stackTrace re-entry lifetime. Both are addressed with explicit rooting and the guard, and the ASAN Malloc=1 regression test in capture-stack-trace.test.js pins the UAF case. The re-entry branch's computeErrorInfoToJSValue can still reach prepareStackTrace (user code), but the frames it passes are *m_stackTrace, which ErrorInstance::visitChildren visits — so they remain rooted through the error object as long as nothing under the guard moves/resets m_stackTrace; I did not find a path that does.
Level of scrutiny
High. This is C++ in src/jsc/bindings/ that (1) newly runs user JS from a path that previously did not, (2) touches GC rooting and ErrorInstance lifetime, and (3) adds mutable state to ZigGlobalObject. Four earlier review rounds on this PR surfaced a real GC-rooting gap (non-ErrorInstance branch) and the UAF, both now fixed — which is exactly why a maintainer should confirm the final shape, particularly the per-global (not per-object) guard design that was acknowledged as an over-approximation and left for follow-up.
Other factors
Test coverage is thorough: two new targeted test blocks in util-inspect.test.js covering deep-prototype/accessor/non-primitive/undefined/null/throwing name and message plus re-entrant getters, three upstream Node assertions restored, and a spawned Malloc=1 ASAN regression for the UAF. All prior inline review threads (mine and CodeRabbit's) are resolved. The one remaining acknowledged limitation — the per-global guard over-approximates and downgrades an unrelated inner error's header to sanitized reads inside another error's getter — is not a regression (pre-PR every path was sanitized) and mirrors the sibling isInsideErrorPrepareStackTraceCallback design, but is worth a maintainer nod.
There was a problem hiding this comment.
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/FormatStackTraceForJS.cpp (1)
834-842: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the lazy-path rooting comment match the actual operation.
The comment says rooting is a no-op for lazy capture, but
protectStackFrameCellsstill allocates, appends roots, and can throw OOM beforesetStackFrames. Clarify that temporary protection is required until the frames are installed.As per coding guidelines, comments should document durable, non-obvious safety invariants.
Proposed comment fix
- // Both eager-compute paths below read name/message via [[Get]] before formatting, - // which may allocate or run a user getter. The lazy path moves the frames into the - // ErrorInstance, which visits them; rooting is a no-op there. + // Formatting may allocate or run a user getter. Keep frame cells rooted until + // lazy frames are installed on the ErrorInstance or eager formatting completes.🤖 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/FormatStackTraceForJS.cpp` around lines 834 - 842, Update the comment immediately above protectedFrameCells and protectStackFrameCells to accurately state that temporary rooting remains necessary for the lazy path until the frames are installed in ErrorInstance, and that protection may allocate or throw OOM. Remove the claim that rooting is a no-op while preserving the existing protection and error-handling logic.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.
Outside diff comments:
In `@src/jsc/bindings/FormatStackTraceForJS.cpp`:
- Around line 834-842: Update the comment immediately above protectedFrameCells
and protectStackFrameCells to accurately state that temporary rooting remains
necessary for the lazy path until the frames are installed in ErrorInstance, and
that protection may allocate or throw OOM. Remove the claim that rooting is a
no-op while preserving the existing protection and error-handling logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 944f0f9a-68bb-48e6-9eb8-3fc89ab4b88e
📒 Files selected for processing (3)
src/jsc/bindings/FormatStackTraceForJS.cpptest/js/node/util/node-inspect-tests/parallel/util-inspect.test.jstest/js/node/v8/capture-stack-trace.test.js
1 participant
Repro
Cause
formatErrorinsrc/js/internal/util/inspect.jscarried a 2023 workaround that rewrote^Error:in the stack string to${err.name}${err.message ? ": " : ""}. It compensated for native bugs in the.stackheader:sanitizedNameStringonly reads own + one prototype level withVMInquiry, skips accessors, and rejects non-primitives;sanitizedMessageStringreads own only; andformatStackTraceToJSValue(theprepareStackTracedefault string) hardcoded"Error".For the common cases (own data
name, empty message) the native header is correct, so the workaround now only fires when the currentname/messagedisagree with the materialized stack, where it drops^Error:but leaves the old message text and producesErrormsg. Deleting it alone regresses cases (2)-(4).Node's
formatErrorhas no such rewrite. V8 composes the header withErrorUtils::ToString: ordinary[[Get]]onnameandmessage,undefineddefaulting to"Error"/ empty,ToStringotherwise.Fix
Both header-composing paths (
computeErrorInfoWithoutPrepareStackTracefor the direct.stackread,formatStackTraceToJSValuefor theprepareStackTracedefault string) now go through onecomputeErrorHeaderthat followsErrorUtils::ToString. The GC-finalizer path reaches the former witherrorInstance == nullptr, so the[[Get]]only runs from a mutator.Running user code there opens two hazards this PR closes:
name/messagegetter that callsError.captureStackTrace(this)or readsthis.stackwould re-enter unboundedly.computeErrorHeaderguards withisComputingErrorStackHeaderonZig::GlobalObject; the inner call falls back to the side-effect-free sanitized accessors so the cycle terminates after one level, matching Node's single outer getter invocation.namegetter that readthis.stackafterError.captureStackTracehad installed the lazyCustomAccessorreachederrorInstanceLazyStackCustomGetterwhile the outermaterializeErrorInfoIfNeededstill held aVector<StackFrame>&into*m_stackTrace; the inner move +setStackFrames({})destructed that Vector and the outerformatStackTraceread freed memory (ASAN heap-use-after-free underMalloc=1). Under the same guard the inner call now formats from the existing Vector without moving out of it or reassigningm_stackTrace.errorConstructorFuncCaptureStackTracenow roots the captured frames' cells in aMarkedArgumentBufferbefore computing (both eager paths), since the header[[Get]]can run a user getter before the frames are formatted; the existing rooting inerrorInstanceLazyStackCustomGetterand this site share aprotectStackFrameCellshelper.As a side effect of dropping the
dynamicDowncast<ErrorInstance>gate,Error.captureStackTrace({name:"X",message:"Y"})now labels its stack"X: Y"(previously"Error"), which is the V8 behavior.With the native header correct, the
stack.replaceline informatErroris removed, the relaxed upstream assertions inutil-inspect.test.js/util-format.test.jsare restored verbatim, and the two previously-disabled subclass-header assertions inutil-inspect.test.js(BazErroraccessor-name and the[WOW]tag variant) are re-enabled.Verification
New tests:
util-inspect.test.js:error inspect preserves stack header when name/message change after materialization(the cases the workaround corrupted plus the subclass/empty-message cases it papered over) anderror stack header reads name/message via full [[Get]](deep-prototypename, accessorname, non-primitivename, accessor/deep-prototypemessage,undefined/nullname, throwing name getter, theprepareStackTracedefault string, a re-entrant message getter called once, andcaptureStackTraceon a{name, message}literal). Every expected value was taken from Node v26.3.0.capture-stack-trace.test.js: spawnedMalloc=1regression for the lazy-getter UAF.test/js/node/v8/capture-stack-trace.test.js(44 pass incl. the#34095test, withBUN_JSC_validateExceptionChecks=1),test/js/node/util/node-inspect-tests/,test/js/node/v8/,test/js/bun/sourcemap/,test/js/bun/util/fuzzy-wuzzy.test.ts,test/js/bun/util/inspect.test.js,test/js/node/assert/all pass. A handful of pre-existing debug+ASAN timeouts (util-inspect.test.js"no assertion failures 2",error-gc-test.test.js,inspect-error-leak.test.js) and the twoinspect-error.test.jssnapshot mismatches reproduce identically on unmodifiedmain.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js