Fix use-after-free in Error.appendStackTrace when source is destination - #32098
Fix use-after-free in Error.appendStackTrace when source is destination#32098robobun wants to merge 6 commits into
Conversation
Error.appendStackTrace(e, e) called appendVector on the error's stack trace vector with itself as the source. When the append grows the vector past its capacity, WTF::Vector frees the old buffer and the span append path does not adjust the source pointer, so the copy reads from freed memory. Skip the append when source and destination are the same error.
|
Updated 12:14 AM PT - Jun 11th, 2026
❌ @robobun, your commit 5f49d6d has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32098That installs a local version of the PR into your bun-32098 --bun |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe patch prevents self-appending an Error's stack trace by returning early when source and destination are identical, and adds integration and unit tests validating self-append safety and normal append behavior. ChangesError.appendStackTrace self-append safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
The heavy GC loop tests in error-gc-test.test.js exceed the per-test timeout on slow debug ASAN runs regardless of this change, so the new tests now live in their own file. Also pass symbolize=0 to the spawned child so an ASan abort on the unfixed build fails the test quickly instead of hitting the test timeout during report symbolization.
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 `@test/js/bun/util/error-append-stack-trace.test.ts`:
- Around line 32-34: The test currently ignores proc.stderr and checks
stdout/exitCode only; change the assertion order to first capture and assert
proc.stderr (the stderr variable from proc.stderr.text()) for ASAN/sanitizer
signatures (e.g., "ERROR: AddressSanitizer" or "SUMMARY: AddressSanitizer") and
fail if present, then proceed to assert stdout and exitCode; locate the
Promise.all usage that assigns [stdout, , exitCode] and modify it to capture
stderr (e.g., [stdout, stderr, exitCode]) and add a short assertion that fails
on sanitizer output before the existing expect(stdout).toBe("ok\n") and
expect(exitCode).toBe(0).
🪄 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: fb4a20e8-c8a1-47f1-8523-e30683c29d15
📒 Files selected for processing (1)
test/js/bun/util/error-append-stack-trace.test.ts
Checking stderr first surfaces the ASan report in the failure diff instead of an empty-stdout mismatch, and still fails if a future ASan configuration reports without aborting. Matches only report markers so benign ASan warnings on debug builds cannot trip it.
With Malloc=1 the child's JSC allocations go through the system allocator, so LeakSanitizer reports JSC's intentional exit-time allocations on the release ASan build and the stderr assertion matched that unrelated report. detect_leaks=0 silences LSan; heap-use-after-free detection is unaffected.
Forcing the system allocator made the spawned child produce no output on the Windows release build, and only ASan builds can observe the use-after-free anyway. Non-ASan builds now spawn with plain bunEnv.
|
Consolidated into #37370, which now also carries the |
Fixes a use-after-free found by fuzzing (fingerprint
35392e9d08621196).Root cause
Error.appendStackTrace(source, destination)merges stack traces with:When
source === destination,appendVectorappends the vector to itself.Vector::appendVectorgoes through the span-basedappend, and when the append grows the vector past its capacity,reserveCapacityallocates a new buffer and frees the old one. The span path passes aconst T*source pointer, which binds to the genericexpandCapacity(size_t, U*)overload that returns the pointer unadjusted (only the non-constT*overload relocates interior pointers). The subsequent copy then reads the StackFrames from the freed buffer.Reallocation only happens once the trace has 9 or more frames (initial capacity is 16), and WTF allocations normally come from bmalloc where ASan cannot see the freed memory, which is why the fuzzer only hit this intermittently: the stale data is usually copied back intact before the block is reused. When the block does get reused or decommitted in that window, the copy constructs
StackFrames (aVariantholding aRefPtrin its wasm alternative) from garbage, corrupting memory.With
Malloc=1(forces bmalloc through the system allocator) the unfixed ASan build reports it deterministically:Fix
The fixing change is the
source == destinationearly return inerrorConstructorFuncAppendStackTrace: appending an error's trace to itself and then clearing it has no useful effect, so it is now a no-op that leaves the trace unchanged.Tests
Added
test/js/bun/util/error-append-stack-trace.test.ts:Malloc=1so ASan builds catch the use-after-free (symbolize=0keeps the failing child fast); it fails on the unfixed ASan build and passes with the fixError.appendStackTracestill merges the source frames into the destinationRepro script: