fix(error): don't capture stack trace on materialized ErrorInstance in appendStackTrace - #34713
fix(error): don't capture stack trace on materialized ErrorInstance in appendStackTrace#34713robobun wants to merge 6 commits into
Conversation
…n appendStackTrace Error.appendStackTrace(source, destination) called captureStackTrace on destination when it had no stack frames, without checking whether error info was already materialized. After accessing .sourceURL/.stack (which materializes and clears the frames), this left the ErrorInstance with m_errorInfoMaterialized=true and a non-null m_stackTrace. When GC's finalizeUnconditionally later found an unmarked frame, computeErrorInfo hit ASSERT(!m_errorInfoMaterialized). Skip captureStackTrace when destination is already materialized (matching errorConstructorFuncCaptureStackTrace), guard the append against a null destination trace, and skip when source and destination are the same object.
|
Updated 11:06 AM PT - Jul 19th, 2026
❌ @robobun, your commit dca95ca has 4 failures in
🧪 To try this PR locally: bunx bun-pr 34713That installs a local version of the PR into your bun-34713 --bun |
WalkthroughChangesError stack trace appending now skips destination stack capture when error information is already materialized. A regression test covers repeated appends, self-append, garbage collection, and subprocess completion. Error stack trace handling
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/bun/util/error-gc-test.test.js`:
- Around line 65-71: Update the combined assertion in the subprocess test to
include the drained stderr value alongside stdout and exitCode, expecting the
quiet stderr result provided by bunEnv. Preserve the existing Promise.all
collection and assert the complete { stdout, stderr, exitCode } structure.
- Around line 48-73: Extend the regression test’s generated script to cover the
self-append branch by calling Error.appendStackTrace on the same unmaterialized
Error object as both arguments, and retain that object through the existing GC
cycle. Keep the current a-to-b append coverage and ensure the self-appended
object remains reachable until both Bun.gc calls complete.
🪄 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: 67ef93b1-69a8-41fb-aa09-228d94809e03
📒 Files selected for processing (2)
src/jsc/bindings/FormatStackTraceForJS.cpptest/js/bun/util/error-gc-test.test.js
|
Applied both review suggestions in 78d6e60: added self-append coverage ( |
error-gc-test.test.js has unrelated tests that time out under the ASAN debug build; moving to a file that runs clean there.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/bun/util/error-gc-test.test.js:51-62— The new&& destination->stackTrace()guard on line 675 is load-bearing (it prevents a null-deref whensourceis unmaterialized butdestinationis materialized), but neither test shape exercises it: the(a, b)case short-circuits onsource->stackTrace()being null, and the(self, self)case has a non-nulldestination->stackTrace()and short-circuits onsource != destination. Consider adding an unmaterialized-source / materialized-destination pair inside the eval'd loop, e.g.const c = new Error(); const d = new Error(); d.sourceURL; Error.appendStackTrace(c, d); keep.push(c, d);— otherwise deleting that guard leaves the test green while re-introducing a null-deref.Extended reasoning...
What the bug is
The C++ change at
FormatStackTraceForJS.cpp:675adds three new guards toerrorConstructorFuncAppendStackTrace:&& !destination->hasMaterializedErrorInfo()on line 671 — skipcaptureStackTracewhen destination is already materialized&& destination->stackTrace()on line 675 — don't dereference a null destination trace&& source != destinationon line 675 — skip self-append
Guard (2) is load-bearing precisely because of guard (1): once guard (1) skips
captureStackTraceon a materialized destination,destination->stackTrace()remains null. Ifsourceis a fresh unmaterialized error (sosource->stackTrace()is non-null), the append block is entered and — without guard (2) —destination->stackTrace()->appendVector(...)dereferencesnullptr.The regression test added to
error-gc-test.test.jscovers guards (1) and (3), but not guard (2).Step-by-step proof that neither test shape reaches guard (2)
Shape A —
a.sourceURL; b.sourceURL; Error.appendStackTrace(a, b):a.sourceURLmaterializesa, clearing itsm_stackTrace→source->stackTrace()is null.- Line 675's condition:
source->stackTrace() && destination->stackTrace() && source != destinationshort-circuits on the first conjunct. - Guard (2) is never evaluated. Deleting it changes nothing here.
Shape B —
Error.appendStackTrace(self, self)withselfunmaterialized:selfis fresh;ErrorInstance::finishCreationsetsm_stackTrace, soself->stackTrace()is non-null.- Line 671:
!destination->stackTrace()is false, socaptureStackTraceis skipped (irrelevant to guard (2)). - Line 675:
source->stackTrace()is non-null (true),destination->stackTrace()is non-null (true — guard (2) is trivially satisfied),source != destinationis false → block skipped by guard (3). - Deleting guard (2) changes nothing here either — it evaluated to true anyway.
The uncovered shape —
d.sourceURL; Error.appendStackTrace(c, d)withcunmaterialized:source = c: fresh,source->stackTrace()non-null.destination = d: materialized viad.sourceURL, sohasMaterializedErrorInfo()is true andstackTrace()is null.- Line 671:
!destination->stackTrace()(true)&& !destination->hasMaterializedErrorInfo()(false) →captureStackTraceskipped by guard (1).destination->stackTrace()remains null. - Line 675 with guard (2) present:
source->stackTrace()(true)&& destination->stackTrace()(false) → block correctly skipped. - Line 675 with guard (2) deleted:
source->stackTrace()(true)&& source != destination(true) → block entered →destination->stackTrace()->appendVector(...)on a null pointer → segfault.
A grep of
test/shows no otherError.appendStackTracecoverage, so nothing else in the suite would catch a regression here.Why this matters
REVIEW.md ("Prove the test fails for the RIGHT reason") says: "Confirm deleting each load-bearing clause of your fix breaks at least one test — a test that passes both ways is worse than no test." Guard (2) can be removed today without any test failing, which means a future refactor could silently reintroduce a null-deref crash for the (unmaterialized source, materialized destination) input.
How to fix
Add a third pair inside the eval'd loop that materializes only the destination:
const c = new Error(); const d = new Error(); d.sourceURL; // materialize destination only Error.appendStackTrace(c, d); keep.push(c, d);
With this in place, deleting
&& destination->stackTrace()produces an immediate segfault (or ASAN null-deref) in the subprocess and the test fails onexitCode.Severity
nit — the C++ fix itself is correct and nothing breaks on merge; this is a test-coverage gap. It's worth adding since the fix is two lines, but it doesn't need to block.
Exercises the destination->stackTrace() null guard; removing it now fails the test with a UBSAN null-deref.
|
Added coverage for the unmaterialized-source / materialized-destination shape in 801a505. Verified that deleting the |
There was a problem hiding this comment.
No bugs found. The two-line guard change is well-explained and mutation-tested, but it touches JSC ErrorInstance state and GC finalization, and it makes Error.appendStackTrace a silent no-op when the destination is already materialized (rather than eagerly recomputing like errorConstructorFuncCaptureStackTrace does) — worth a maintainer glance to confirm that's the intended semantics.
What was reviewed:
- Confirmed the
hasMaterializedErrorInfo()guard mirrors the existing one inerrorConstructorFuncCaptureStackTracein the same file. - Confirmed
Error.appendStackTracehas no internal callers insrc/js/(private, non-enumerable, only registered in ZigGlobalObject.cpp), so the no-op-on-materialized behavior change has no downstream effect. - Test covers all three new guards (materialized dest, null dest trace, self-append); the
stderr: ""assertion underbunEnvwas examined and deemed acceptable.
Extended reasoning...
Overview
This PR fixes a Fuzzilli-discovered debug assertion (ASSERTION FAILED: !m_errorInfoMaterialized) in errorConstructorFuncAppendStackTrace. Two conditions are added in src/jsc/bindings/FormatStackTraceForJS.cpp: (1) skip destination->captureStackTrace() when the destination has already materialized its error info, and (2) guard the append against a null destination trace and against source == destination. A subprocess GC-stress test is added to test/js/node/v8/capture-stack-trace.test.js covering three input shapes, and the author mutation-tested each new guard.
Security risks
None. Error.appendStackTrace is a Bun-specific, non-enumerable, ImplementationVisibility::Private function on the Error constructor with no callers in src/js/. The change strictly narrows when captureStackTrace/appendVector run; no new allocation, user-callback, or untrusted-input handling is introduced.
Level of scrutiny
Medium. The diff is tiny and defensive, but it sits at the intersection of JSC ErrorInstance internal state (m_stackTrace, m_errorInfoMaterialized) and GC's finalizeUnconditionally — the exact area REVIEW.md flags as the most-blocked category. The added hasMaterializedErrorInfo() guard is copied directly from errorConstructorFuncCaptureStackTrace a few lines below, which lends confidence, but that sibling function eagerly computes and installs .stack when materialized, whereas this change makes appendStackTrace silently do nothing. That's likely fine for an unused/undocumented API, but it's a design choice a maintainer should ratify rather than a bot.
Other factors
The evidence block shows the test fails on the unfixed ASAN debug build (exit 134 with the expected assertion) and passes on both ASAN debug and release with the fix. CodeRabbit's two suggestions (self-append coverage, stderr in the combined assertion) were addressed in follow-up commits, and a third commit added coverage for the null-destination-trace guard. The verifier agents examined the stderr: "" assertion against REVIEW.md's "never assert stderr is exactly empty" guidance and ruled it out — bunEnv quiets debug logs and the combined-object form is what the guideline recommends. I have no prior review on this PR.
There was a problem hiding this comment.
LGTM — narrow guard addition that mirrors the existing hasMaterializedErrorInfo() check in errorConstructorFuncCaptureStackTrace.
What was reviewed:
- Confirmed the new guards only skip work (no new state written), so no new UAF/leak surface; the null-check and self-append guards are each mutation-tested per the PR evidence.
Error.appendStackTraceis registered withImplementationVisibility::Private, undocumented, and unused insrc/js/, so the behavioral change (append becomes a no-op when destination is materialized) has no internal callers to affect.- Test follows harness conventions (subprocess,
bunEnv, concurrent pipe drain, combined assertion) and CodeRabbit feedback was applied.
Extended reasoning...
Overview
Two-line change to errorConstructorFuncAppendStackTrace in src/jsc/bindings/FormatStackTraceForJS.cpp plus a subprocess regression test. The fix adds three guards: (1) skip captureStackTrace on the destination if its error info is already materialized, (2) skip the append if the destination has no stack trace vector, and (3) skip if source and destination are the same object. Guard (1) is the actual crash fix — it prevents installing a fresh m_stackTrace on an ErrorInstance whose m_errorInfoMaterialized is already true, which trips ASSERT(!m_errorInfoMaterialized) in computeErrorInfo during finalizeUnconditionally. Guards (2) and (3) are defensive follow-ons that fall out of (1): once capture can be skipped, the destination trace can legitimately be null at the append site.
Security risks
None. This is a debug-assertion / GC-consistency fix on a private, undocumented static method on the Error constructor. No untrusted input parsing, no allocation size arithmetic, no new code paths that write memory.
Level of scrutiny
Moderate. JSC ErrorInstance / GC-finalizer interaction is memory-safety-adjacent, but this change is strictly subtractive: every added condition only skips an operation that was previously unconditional. Skipping captureStackTrace cannot leak (nothing was allocated) and cannot UAF (nothing is freed). Skipping appendVector/clear likewise. The pattern exactly mirrors the existing hasMaterializedErrorInfo() branch in errorConstructorFuncCaptureStackTrace roughly 120 lines below in the same file, so it's a known-good idiom here rather than a novel design.
Other factors
- The affected API (
Error.appendStackTrace) is registeredImplementationVisibility::Private+DontEnumand has zero callers insrc/js/, so the only observable behavior change (append becoming a no-op when the destination's info is already materialized) affects nothing shipped. - Evidence in the PR body is strong: fails-on-main / passes-on-PR under ASAN debug, plus per-guard mutation testing (removing
!hasMaterializedErrorInfo()restores the original assertion; removingdestination->stackTrace()trips a UBSAN null-member-call). - The test covers all three guard shapes (materialized/materialized, unmaterialized/materialized, self-append) and was iterated in response to CodeRabbit feedback, all of which is now resolved.
- The
stderr: ""assertion is slightly stricter than REVIEW.md recommends, but was explicitly requested by CodeRabbit and verified green under ASAN debug withbunEnv, so it's not a practical concern.
|
CI status:
This PR only adds two guard conditions in |
|
Consolidated into #37370, which now also carries the cases from this PR: it returns early for a materialized destination and for |
1 participant
Fuzzilli found a debug assertion failure (fingerprint
667a4a9c2a9a6191):Cause
Error.appendStackTrace(source, destination)callsdestination->captureStackTrace()wheneverdestination->stackTrace()is null, without checkinghasMaterializedErrorInfo(). Once an error's.sourceURL/.stack/etc has been accessed, its error info is materialized and its internalm_stackTraceis cleared. CallingcaptureStackTraceat that point installs a freshm_stackTracewhilem_errorInfoMaterializedstaystrue, which is an inconsistent state.Later, during GC,
ErrorInstance::finalizeUnconditionallysees the non-nullm_stackTrace, finds an unmarked frame, and callscomputeErrorInfo, which asserts!m_errorInfoMaterialized.Repro:
Fix
Skip
captureStackTracewhendestinationhas already materialized its error info, matching the existing guard inerrorConstructorFuncCaptureStackTrace. Also guard the append against a null destination trace and skip whensource == destination(self-appending aWTF::Vectorthen clearing it is pointless at best).How did you verify your code works?
Added a subprocess test in
test/js/node/v8/capture-stack-trace.test.jsthat exercises three shapes:source != destinationguard)Verified the test crashes with
exitCode: 134andASSERTION FAILED: !m_errorInfoMaterializedon the unfixed debug build and passes with the fix. Also mutation-tested each new guard: removing!hasMaterializedErrorInfo()restores the original assertion, and removingdestination->stackTrace()trips a UBSAN null-pointer member call. All 44 tests in the file pass under both ASAN debug and release builds.[stamp-90s] gate passed · iteration 3 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 2 rejected · iteration 3
evidence per changed file