Skip to content

bundler: copy a BuildMessage's namespace out of the bundle arena - #40722

Merged
Jarred-Sumner merged 1 commit into
mainfrom
farm/9e14bfc7/build-message-namespace-uaf
Aug 28, 2026
Merged

Jarred-Sumner merged 1 commit into
mainfrom
farm/9e14bfc7/build-message-namespace-uaf

Conversation

@robobun

@robobun robobun commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • Bun.build() with a plugin module in a non-file namespace: if that module has a build error, reading log.position.namespace from result.logs after the build reads freed memory. With MIMALLOC_PURGE_DELAY=0 it crashes: SEGV in simdutf::validate_ascii from BuildMessage::generate_position_object (src/jsc/BuildMessage.rs:133).
  • Cause: dev server: keep out-of-root watched paths alive across bundles #40640 moved a plugin module's namespace into the bundle's MimallocArena (src/resolver/lib.rs, the disjoint text/pretty branch of dupe_alloc). bun_ast::Location stores namespace as a bare &'static [u8], and its clone impls copy the pointer. The BuildMessage holds that Msg after the arena is destroyed.

Fix

  • Location.namespace becomes a Cow<'static, [u8]>. Both clone impls (Clone and clone_with_builder) deep-copy it, the same way they already copy file and line_text. The four constructors borrow, as before.
  • Correct because msg_to_js (src/jsc/lib.rs) clones the Msg while the bundle is still alive. The copy is taken from valid memory, and the BuildMessage then owns its bytes.
  • Verified: test/bundler/bun-build-api.test.ts, "a BuildMessage keeps the namespace of a plugin module after the build". On main it crashes with the SEGV above. Also green: the rest of bun-build-api.test.ts, test/js/bun/plugin/plugins.test.ts, test/bake/dev/html.test.ts.

Background

  • Path::dupe_alloc turns a resolver's or a plugin's Path into the one the bundle graph stores. text is the absolute path, pretty the display path, namespace is file or a plugin namespace. Since dev server: keep out-of-root watched paths alive across bundles #40640 the display path, and the namespace of a plugin module, live in the bundle's arena, which is destroyed when the bundle is done.
  • bun_ast::Location is the position attached to a log message. Bun.build() results and the dev server keep messages after the bundle is gone, which is why Location does not derive Clone and deep-copies instead.
  • A plugin module's pretty is <namespace>:<text>, which never contains text, so every module in a non-file namespace takes the arena branch.
Notes

Split out of #39456, which stops dupe_alloc from growing the FilenameStore on every bundle and makes more of the Path arena-backed. This change is needed on its own since #40640 and is the smaller fix, so it lands first. #39456 is stacked on it.

MIMALLOC_PURGE_DELAY=0 and MIMALLOC_ABANDONED_PAGE_PURGE=1 make mimalloc return the destroyed heap's pages to the OS at once. Without them the stale pointer reads the old bytes and the test passes by luck. ASAN does not see the arena (mimalloc manages its own segments), so the crash is a plain SEGV.


no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts

Since #40640 a plugin module's namespace lives in the bundle's arena.
Location copied it as a bare pointer, so a BuildMessage in result.logs
read freed memory when JS read position.namespace after the build.

Location.namespace is now a Cow that both clone impls deep-copy, like
file and line_text.
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 24 days. After that, they cost $0.25 per reviewed file.

Or wait 1 minute for your next included review.

View limit details

Limit details: You’ve used all 5 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6b907d92-eb88-4222-9f25-29826fba0e54

📥 Commits

Reviewing files that changed from the base of the PR and between e83de42 and ba22a15.

📒 Files selected for processing (5)
  • src/ast/lib.rs
  • src/css/error.rs
  • src/jsc/BuildMessage.rs
  • src/runtime/server/DevErrorPage.rs
  • test/bundler/bun-build-api.test.ts

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

@robobun

robobun commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Status: diff ready, waiting on CI.

Reproduced on main (e0a6d02128 and later) with a debug build: a Bun.build() whose plugin resolves a module into the virtual namespace and whose onLoad returns let = ;. Reading result.logs[0].position.namespace under MIMALLOC_PURGE_DELAY=0 MIMALLOC_ABANDONED_PAGE_PURGE=1 crashes with SEGV in simdutf::validate_ascii, called from BuildMessage::generate_position_object (src/jsc/BuildMessage.rs:133). Without the purge env the read returns the old bytes and looks correct. The release build before #40640 prints virtual.

The new test in test/bundler/bun-build-api.test.ts fails on main with that SEGV and passes with this change. The fix is the Location.namespace part of #39456, split out so it can land on its own.

@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 — mechanical extension of the existing file/line_text deep-copy pattern to namespace.

What was reviewed:

  • Location.namespace becomes Cow<'static, [u8]> and is deep-copied in both the manual Clone impl and clone_with_builder, mirroring how file and line_text are already handled in the same struct.
  • Checked that count(), Default, init, and both init_or_null construction sites were all updated consistently; the three consumer sites (BuildMessage.rs, DevErrorPage.rs, css/error.rs) are trivial borrow adjustments.
  • Test follows harness conventions (tempDir, bunEnv spread, concurrent pipe drain, stderr/stdout asserted before exitCode) and uses MIMALLOC_PURGE_DELAY=0 so the stale-pointer read faults instead of passing by luck.
Extended reasoning...

Overview

This PR fixes a use-after-free in bun_ast::Location.namespace. Previously the field was a Str (&'static [u8]) that could borrow into the bundle's MimallocArena; when a BuildMessage from Bun.build().logs outlives the arena, reading position.namespace would touch freed memory. The fix promotes the field to Cow<'static, [u8]> and deep-copies it in the manual Clone and clone_with_builder impls — exactly the same treatment already applied to file and line_text in the same struct for the same reason. Three call sites (src/css/error.rs, src/jsc/BuildMessage.rs, src/runtime/server/DevErrorPage.rs) are updated to borrow/deref the Cow. A subprocess test in test/bundler/bun-build-api.test.ts reproduces the crash by forcing mimalloc to return arena pages to the OS immediately.

Security risks

None. This is an internal representation change to fix a UAF; no user-facing input parsing, auth, or crypto paths are touched. The only externally observable effect is that log.position.namespace now returns the correct string instead of garbage or crashing.

Level of scrutiny

Moderate — memory safety in native code — but the change is small (~30 lines of Rust), applies an already-established pattern from the neighboring fields in the same struct, and the PR description correctly identifies the mechanism (msg_to_js clones the Msg while the bundle is still alive, so the deep-copy is taken from valid memory). All construction sites (Default, init, init_or_null's two branches, the CSS error builder) were updated to Cow::Borrowed, and both clone paths were updated to Cow::Owned(...to_vec()). The count() helper was updated to match file's treatment. No sites were missed based on a grep of location.namespace usages.

Other factors

The test is well-constructed per the repo's review rules: it uses tempDir, spreads bunEnv, drains stdout/stderr/exited concurrently, asserts output before exit code, uses test.concurrent, and sets MIMALLOC_PURGE_DELAY=0 / MIMALLOC_ABANDONED_PAGE_PURGE=1 so the freed-page read is a hard SEGV rather than a lucky pass on the old bytes. No CODEOWNERS entries cover the changed files. The bug hunt exited with dry_streak and no findings. No prior reviews or outstanding objections in the timeline.

@robobun

robobun commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:40 PM PT - Aug 27th, 2026

❌ @robobun, your commit ba22a15 has 2 failures in Build #107394 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 40722

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

bun-40722 --bun

@Jarred-Sumner
Jarred-Sumner merged commit 656023f into main Aug 28, 2026
10 of 12 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/9e14bfc7/build-message-namespace-uaf branch August 28, 2026 07:41
robobun added a commit that referenced this pull request Aug 28, 2026
ast/lib.rs: #40722 makes Location own its namespace on the lines where this
branch computes line and column from the tracker counts; main's Cow with this
branch's conversions. parse_entry.rs: #40594 tells a parser-generated import
record apart by its empty range, which on this branch is range.is_some().
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.

2 participants