Skip to content

Fix use-after-free when building AggregateError from multiple module build errors#31284

Closed
robobun wants to merge 2 commits into
mainfrom
farm/ebb70652/fix-buildmessage-gc-uaf
Closed

Fix use-after-free when building AggregateError from multiple module build errors#31284
robobun wants to merge 2 commits into
mainfrom
farm/ebb70652/fix-buildmessage-gc-uaf

Conversation

@robobun

@robobun robobun commented May 23, 2026

Copy link
Copy Markdown
Collaborator

Fixes a heap-use-after-free found by fuzzing (ASAN fingerprint heap-use-after-free, READ of size 1 at offset 144 of a 152-byte allocation, hit while printing an unhandled module-load error).

Root cause

When a module fails to build with more than one log message, process_fetch_log (and Log::toJS in bun_jsc::lib) creates one BuildMessage/ResolveMessage JS wrapper per message and then wraps them in an AggregateError. The wrappers were accumulated in a heap Vec<JSValue>. JSC's conservative GC scan only sees the native stack, so between a wrapper's creation and createAggregateError copying it into a JS array, the only reference to it lived in unscanned heap memory. Creating each subsequent wrapper allocates (JSC cell + native BuildMessage box), which can trigger a GC; any already-created wrapper could be collected, running JSBuildMessage's finalizer and freeing its native BuildMessage (152 bytes). The dead cells were then stored in the AggregateError.errors array, and printing the unhandled rejection later read the freed allocation at build_error.logged (offset 144) in VirtualMachine::print_error_from_maybe_private_data:

READ of size 1 ... in core::cell::Cell<bool>::get
  print_error_from_maybe_private_data (VirtualMachine.rs:4976)
  print_errorlike_object -> agg_iter -> JSC__JSValue__forEach
freed by: JSBuildMessage::destroy -> BuildMessageClass__finalize -> Box<BuildMessage> drop (GC sweep)
allocated by: BuildMessage::create <- process_fetch_log <- module fetch

The Zig implementation used an on-stack [256]JSValue array precisely so the conservative stack scan would keep these values alive (the same idiom log_to_js in ast_jsc kept); the port switched it to a heap Vec, losing that guarantee.

Fix

Use the on-stack [JSValue; 256] array again in both places (process_fetch_log and LogJsc::to_js) so each wrapper remains GC-visible until the AggregateError owns it.

Reproduction / verification

import() of a module with ~80 build errors with BUN_JSC_collectContinuously=1 reliably reproduced the ASAN heap-use-after-free on an unpatched debug build (identical stack/object/offset to the fuzzer report, which hit the same path on a Worker thread). With this change the same repro exits cleanly (code 1) and prints every error; added it as a regression test in test/js/bun/resolve/build-error.test.ts.

…regateErrors

process_fetch_log and Log::toJS accumulated the BuildMessage/ResolveMessage
JS wrappers for a multi-error AggregateError in a heap Vec<JSValue>. The
conservative GC scan only covers the native stack, so a GC triggered while
creating the remaining wrappers could finalize the earlier ones and free
their native BuildMessage before the AggregateError was constructed. The
dead wrappers then ended up in the errors array and printing the unhandled
rejection read the freed allocation (build_error.logged).

Use the on-stack [JSValue; 256] array (same idiom as log_to_js in ast_jsc)
so every wrapper stays reachable until the AggregateError owns it.
@coderabbitai

coderabbitai Bot commented May 23, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, we couldn't start this review because you've used your available PR reviews for now.

Your plan currently allows 2 reviews/hour. Refill in 19 minutes and 6 seconds.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more review capacity refills, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 106f10c8-d0ac-41b2-953f-829a136ab48f

📥 Commits

Reviewing files that changed from the base of the PR and between f161e03 and f16e0cd.

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

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

@robobun

robobun commented May 23, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:52 PM PT - May 23rd, 2026

@robobun, your commit f16e0cd has 1 failures in Build #57331 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31284

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

bun-31284 --bun

Comment thread test/js/bun/resolve/build-error.test.ts Outdated
@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 - Same fix: replaces heap Vec<JSValue> with on-stack [JSValue; 256] in the same two functions (process_fetch_log and LogJsc::to_js) to keep BuildMessage cells GC-visible

🤖 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.

LGTM — restores the on-stack [JSValue; 256] idiom from the Zig source so the conservative GC scan keeps the wrappers alive; thanks for dropping the redundant ASAN assertion.

Extended reasoning...

Overview

This PR fixes a heap-use-after-free in two sites that build an AggregateError from multiple BuildMessage/ResolveMessage wrappers: process_fetch_log in src/jsc/VirtualMachine.rs and LogJsc::to_js in src/jsc/lib.rs. The Rust port had switched the original Zig on-stack [256]JSValue buffer to a heap Vec<JSValue>; JSC's conservative GC only scans the native stack, so wrappers held only in the heap Vec could be collected mid-loop when a later BuildMessage::create triggered a GC. The fix reverts both sites to a stack-resident [JSValue; 256] (2 KB), slicing to [..len] for the create_aggregate_error call. A regression test using BUN_JSC_collectContinuously=1 and ~80 build errors is added to test/js/bun/resolve/build-error.test.ts.

Security risks

None. This is a memory-safety hardening change with no new inputs, no auth/crypto/permission surface, and no user-controlled data flow changes. The only behavioral effect is that previously-freed JS cells are now kept alive until the AggregateError owns them.

Level of scrutiny

Moderate — module-loader / GC-interaction code is sensitive, but the change is small, mechanical, and restores the exact pattern the Zig spec used (and which the in-file comment had already documented as the intent). JSValue derives Default (zero), so the array init is sound, and only the populated prefix is passed across FFI. The 256-cap and min() clamp are unchanged from before. The PR description includes a precise ASAN trace matching the fix.

Other factors

  • My prior nit (the not.toContain("AddressSanitizer") anti-pattern) was addressed in commit f16e0cd; the test now relies solely on positive assertions (x0/x39 printed, exitCode === 1).
  • No CODEOWNERS cover these paths.
  • The bug-hunting system found no issues on the current revision.
  • A duplicate-PR bot flagged #30671 as the same fix; that is a process/triage matter and does not affect correctness of this change.
  • No outstanding human reviewer comments.

@robobun

robobun commented May 23, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as a duplicate of #30671, which contains the identical source fix (on-stack [JSValue; 256] in process_fetch_log and Log::to_js) and has been open longer. The module-loader regression test from this PR has been added to #30671 so the scenario from this fuzzer report stays covered.

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