Skip to content

Fix use-after-free when a GC runs while aggregating module build errors#31377

Closed
robobun wants to merge 1 commit into
mainfrom
farm/e1ad33a3/fix-buildmessage-aggregate-uaf
Closed

Fix use-after-free when a GC runs while aggregating module build errors#31377
robobun wants to merge 1 commit into
mainfrom
farm/e1ad33a3/fix-buildmessage-aggregate-uaf

Conversation

@robobun

@robobun robobun commented May 25, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a fuzzer-found heap-use-after-free (fingerprint Address:heap-use-after-free:bun-debug+0x11afa5ac, READ of size 1 at offset 144 of a freed 152-byte BuildMessage, on a Worker thread).

When a module fails to load with more than one log message, process_fetch_log (and Log::to_js) wrap each BuildMessage/ResolveMessage in a JS cell and aggregate them into an AggregateError. The freshly created cells were accumulated in a heap Vec<JSValue>, which JSC's conservative stack scan cannot see. Any allocation during the aggregation (another BuildMessage::create, or the JSArray allocated inside createAggregateError) can trigger a GC that sweeps those cells, destroying their native BuildMessage (152 bytes). The AggregateError then holds dangling cells, and printing the error afterwards reads the freed native object:

READ of size 1 thread T42 (Worker)
  #0 <core::cell::Cell<bool>>::get
  #1 VirtualMachine::print_error_from_maybe_private_data  src/jsc/VirtualMachine.rs:4960
  #2 VirtualMachine::print_errorlike_object
  #3 print_errorlike_object::agg_iter
  #4 JSC__JSValue__forEach

freed by GC sweep during the aggregation itself:
  ... JSBuildMessage::destroy ← Heap::collectInMutatorThread ← LocalAllocator::allocateSlowCase
  ← JSArray::tryCreateUninitializedRestricted ← JSC__JSGlobalObject__createAggregateError
  ← process_fetch_log ← Bun__transpileFile ← moduleLoaderFetch

The Zig implementation this was ported from used var errors_stack: [256]JSValue — a stack array — which was load-bearing: createAggregateError's contract expects a stack-rooted slice so the conservative scan keeps the cells alive. The port switched it to a heap Vec, losing that protection.

The fix restores the fixed stack array in both places that accumulate message cells before creating the AggregateError:

  • src/jsc/VirtualMachine.rs process_fetch_log (module loader fetch errors)
  • src/jsc/lib.rs Log::to_js (worker flush_logs and other Log.toJS users)

The fuzzer hit this on a Worker whose entry module failed to transpile (a Worker pointed at a non-JS file), where the worker thread aggregates and prints the error; the same window exists on the main thread.

How did you verify your code works?

  • Reproduced the original crash: a worker loading a file with many parse errors under BUN_JSC_collectContinuously=1 hits the exact UAF 10/10 times on an unfixed ASAN debug build (and the same report appears on the main thread when running such a file directly).
  • With this fix, the same repros pass 20/20 (worker exits cleanly, direct run exits 1 with the normal error listing, no ASAN report).
  • Added a regression test in test/js/bun/resolve/build-error.test.ts that runs both scenarios under BUN_JSC_collectContinuously=1. It fails on an unfixed ASAN debug build (AddressSanitizer report in stderr) and passes with this fix.
  • bun bd test test/js/bun/resolve/build-error.test.ts: 3 pass, 0 fail.

… is created

When a module fails to load with more than one log message, processFetchLog
and Log.toJS wrap each BuildMessage/ResolveMessage in a JS cell and collect
them into an AggregateError. The cells were accumulated in a heap Vec<JSValue>,
which JSC's conservative stack scan cannot see, so a GC triggered by any
allocation during aggregation could sweep them before the AggregateError
adopted them. Printing the error afterwards then read the freed native
BuildMessage (heap-use-after-free).

Use a fixed stack array for the accumulation, matching the stack-rooted
contract of createAggregateError.
@robobun

robobun commented May 25, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:06 AM PT - May 25th, 2026

@robobun, your commit 9e706dc has 1 failures in Build #57879 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31377

That installs a local version of the PR into your bun-31377 executable, so you can run:

bun-31377 --bun

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 68b35045-008c-4e46-91aa-e9997a2211d3

📥 Commits

Reviewing files that changed from the base of the PR and between 705c482 and 9e706dc.

📒 Files selected for processing (3)
  • src/jsc/VirtualMachine.rs
  • src/jsc/lib.rs
  • test/js/bun/resolve/build-error.test.ts

Walkthrough

This PR optimizes aggregate error handling by replacing heap-allocated Vec with fixed-size stack arrays in two error construction paths, then adds a GC stress test to validate the optimization remains safe under memory pressure.

Changes

Stack allocation for aggregate errors

Layer / File(s) Summary
VirtualMachine stack allocation
src/jsc/VirtualMachine.rs
process_fetch_log replaces Vec<JSValue> with stack-allocated [JSValue; 256], populates the first len.min(256) entries by draining messages, and passes the resulting slice to create_aggregate_error.
LogJsc stack allocation
src/jsc/lib.rs
LogJsc::to_js replaces Vec<JSValue> with stack-allocated [JSValue; 256] buffer, populates from the first count log entries via zipped iteration, and passes the stack slice to create_aggregate_error.
GC stress test for error creation
test/js/bun/resolve/build-error.test.ts
Test harness gains bunEnv and bunExe imports. New test generates fixture files with many transpile failures, spawns Bun subprocesses with platform-specific GC pressure, and verifies aggregated errors remain readable without AddressSanitizer violations.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly describes the main fix: preventing use-after-free during GC when aggregating module build errors, which is the core change across both modified source files.
Description check ✅ Passed The description follows the template with both required sections: 'What does this PR do?' provides detailed context on the UAF bug and the fix, and 'How did you verify your code works?' documents reproduction, testing, and regression test results.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Root BuildMessage cells on the stack in Log.to_js #30671 - Both fix the same use-after-free by replacing heap Vec<JSValue> with a stack array in process_fetch_log and Log::to_js so JSC's conservative GC stack scan can see the cells during AggregateError construction

🤖 Generated with Claude Code

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any issues — the fix correctly restores the stack-rooted [JSValue; 256] from the Zig spec so the conservative scanner keeps the cells alive through create_aggregate_error — but GC-rooting changes in the module loader path are worth a maintainer's eyes.

Extended reasoning...

Overview

This PR fixes a fuzzer-found heap-use-after-free in two places that aggregate BuildMessage/ResolveMessage cells into an AggregateError: process_fetch_log in src/jsc/VirtualMachine.rs and Log::to_js in src/jsc/lib.rs. The Rust port had replaced the original Zig var errors_stack: [256]JSValue stack array with a heap Vec<JSValue>, which JSC's conservative stack scanner cannot see; a GC triggered by any allocation during aggregation could sweep the freshly created cells before the AggregateError adopted them. The fix switches back to a fixed [JSValue::UNDEFINED; 256] stack array, fills the first len slots via iter_mut().zip(...), and passes &errors_stack[..len] to create_aggregate_error. A regression test exercises both the main-thread and Worker paths under BUN_JSC_collectContinuously=1.

Security risks

None introduced — this is a memory-safety improvement that eliminates a UAF. No new attack surface, input parsing, or auth/permission logic is touched.

Level of scrutiny

Medium-high. The diff is small and mechanical, and it restores the exact shape of the Zig spec the comment already referenced. The zip bound and [..len] slicing are correct, and the stack array is borrowed across the create_aggregate_error call so it stays live in the frame. That said, this is core JSC/module-loader code where correctness depends on a subtle runtime invariant (conservative stack scanning finding values in a Rust local array), and the original bug was itself a subtle port regression in exactly this spot. A maintainer familiar with the JSC GC-rooting conventions should confirm this is the preferred pattern (vs. e.g. MarkedArgumentBuffer).

Other factors

The PR includes a targeted regression test that the author reports fails 10/10 on an unfixed ASAN build and passes 20/20 with the fix, and bun bd test passes locally. The bug-hunting system found no issues. The 2 KiB stack array matches the original Zig footprint and is not a stack-depth concern in these call paths.

@robobun

robobun commented May 25, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as a duplicate of #30671, which makes the same change (stack-rooting the BuildMessage/ResolveMessage cells in process_fetch_log and Log::to_js) and has been open longer with broader test coverage.

For the record, this PR came from a separate fuzzer-found reproduction of the same bug: a Worker whose entry module fails to transpile with multiple parse errors hits the identical heap-use-after-free on the worker thread when the aggregated error is printed. Deterministic repro on an ASAN debug build:

# many-errors.js: ~60 lines that each produce a recoverable parse error
BUN_JSC_collectContinuously=1 bun-debug -e 'new Worker("./many-errors.js").addEventListener("error", () => {});'

I verified the stack-array fix (as in #30671) resolves that reproduction as well.

@robobun robobun closed this May 25, 2026
@robobun robobun deleted the farm/e1ad33a3/fix-buildmessage-aggregate-uaf branch May 25, 2026 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant