Skip to content

fix(bundler): avoid aliased mutable borrows in chunk codegen#31267

Open
Dicklesworthstone wants to merge 1 commit into
oven-sh:mainfrom
Dicklesworthstone:claude/ub-fix-bundler-chunk-aliasing-v2
Open

fix(bundler): avoid aliased mutable borrows in chunk codegen#31267
Dicklesworthstone wants to merge 1 commit into
oven-sh:mainfrom
Dicklesworthstone:claude/ub-fix-bundler-chunk-aliasing-v2

Conversation

@Dicklesworthstone

@Dicklesworthstone Dicklesworthstone commented May 23, 2026

Copy link
Copy Markdown

Summary

Fixes the audit's EXP-111 / Bundler B-1..B-5 aliasing finding in the parallel chunk-codegen path.

The JS/CSS part-range worker callbacks used to rebuild &mut LinkerContext and &mut Chunk from shared raw pointers even though that phase only needs shared graph/chunk reads plus disjoint compile-result slot writes. Under Miri stacked borrows, the EXP-111 witness reports a data race between retag writes while reconstructing &mut Chunk on parallel worker threads.

This PR:

  • routes JS/CSS part-range codegen through shared &LinkerContext / &Chunk borrows
  • keeps per-task output writes behind Chunk::write_compile_result_slot, which writes only the task's disjoint compile_results_for_chunk[i] slot
  • keeps the slot writer on shared access to the boxed UnsafeCell slice instead of raw-reading the Box representation
  • makes the printer renamer view shared/read-only so part-range printers do not fabricate aliased &mut ChunkRenamer borrows
  • requires erased RequireOrImportMetaCallback::init contexts to be Sync, matching the shared worker-side callback use
  • makes SymbolMap::follow() read-only; follow_all(&mut self) remains the serial path-compression pass
  • preserves prior serial side effects by preparing browser transpiler state and Source.path.pretty before worker fan-out
  • propagates pretty-path initialization failures through the existing bundler Result path instead of panicking

Audit Evidence

  • Finding: EXP-111 / Bundler B-1..B-5
  • Witness: .ub-exorcism/2026-05-15-exhaustive/phase5_experiment_results/EXP-111-sb.log
  • Miri signal: stacked-borrows data race between retag writes while reconstructing &mut Chunk from shared worker raw pointers

Verification

  • cargo fmt -p bun_ast -p bun_js_printer -p bun_bundler
  • cargo fmt --check -p bun_ast -p bun_js_printer -p bun_bundler
  • env CARGO_TARGET_DIR=/tmp/bun-ub-fix-bundler-alias-v2-target cargo check -p bun_bundler
  • env CARGO_TARGET_DIR=/tmp/bun-ub-fix-bundler-alias-v2-target cargo check --workspace
  • bun bd test test/bundler/transpiler/transpiler.test.js
  • bun bd test test/bundler/bundler_barrel.test.ts
  • bun bd test test/bundler/bundler_html.test.ts
  • git diff --check origin/main...HEAD

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@Dicklesworthstone Dicklesworthstone force-pushed the claude/ub-fix-bundler-chunk-aliasing-v2 branch from 3003d75 to c766026 Compare May 23, 2026 05:02
@coderabbitai

coderabbitai Bot commented May 23, 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: a818ba53-725e-4066-aaab-a0608d368a43

📥 Commits

Reviewing files that changed from the base of the PR and between a70197b and c47a469.

📒 Files selected for processing (15)
  • src/ast/symbol.rs
  • src/bundler/Chunk.rs
  • src/bundler/LinkerContext.rs
  • src/bundler/linker_context/convertStmtsForChunk.rs
  • src/bundler/linker_context/convertStmtsForChunkForDevServer.rs
  • src/bundler/linker_context/generateChunksInParallel.rs
  • src/bundler/linker_context/generateCodeForFileInChunkJS.rs
  • src/bundler/linker_context/generateCompileResultForCssChunk.rs
  • src/bundler/linker_context/generateCompileResultForJSChunk.rs
  • src/bundler/linker_context/postProcessCSSChunk.rs
  • src/bundler/linker_context/postProcessHTMLChunk.rs
  • src/bundler/linker_context/postProcessJSChunk.rs
  • src/bundler/ungate_support.rs
  • src/js_printer/lib.rs
  • src/js_printer/renamer.rs

Walkthrough

Convert exclusive &mut borrows to shared & across symbol follow, renamer APIs, and chunk codegen; remove unsafe aliasing patterns; add a serial pre-initialization step before parallel chunk post-processing.

Changes

Immutability refactor for parallel chunk codegen

Layer / File(s) Summary
Symbol table read-only link following
src/ast/symbol.rs
Map::follow() is redefined as a read-only link chaser that no longer performs path compression; callers now call follow(ref_) before final get/get_const; follow_all(&mut self) remains the single-threaded compression pass.
Chunk safety & slot write
src/bundler/Chunk.rs
Clarify unsafe impl Send for Chunk comment and refactor Chunk::write_compile_result_slot to project to a shared slice and use UnsafeCell::get() for the destination pointer prior to write.
LinkerContext core method immutability
src/bundler/LinkerContext.rs
Multiple methods (e.g., path_with_pretty_initialized, generate_source_map_for_chunk, print_code_for_file_in_chunk_js, require_or_import_meta_for_source, GenerateChunkCtx::c) now take &self. Added prepare_pretty_paths_for_isolated_hash(&mut self, chunks: &[Chunk]) and changed generate_isolated_hash to &self -> Result<u64, BunError>.
Serial hash path preparation before parallel phase
src/bundler/linker_context/generateChunksInParallel.rs
Add serial pre-fan-out setup: eagerly initialize browser transpiler if needed and call prepare_pretty_paths_for_isolated_hash() before dispatching parallel chunk tasks.
Safe parts slicing and namespace export emission
src/bundler/linker_context/generateCodeForFileInChunkJS.rs
Eliminated unsafe raw-pointer SoA access; derive all_parts and parts as safe slices, iterate with parts.iter(), and use c.graph.parts_live for liveness checks. Function parameters now use &LinkerContext/&Chunk.
DeclCollector immutable renamer integration
src/bundler/linker_context/generateCodeForFileInChunkJS.rs
DeclCollector::collect_from_stmts and helpers (collect_from_binding, add_ref) now accept &renamer::Renamer; call sites pass &r.
JS chunk compile result generation with immutable borrows
src/bundler/linker_context/generateCompileResultForJSChunk.rs
Worker callback converts raw pointers to shared references and calls an impl that takes &LinkerContext/&Chunk; runtime scope access becomes immutable; call to generate_code_for_file_in_chunk_js passes chunk.renamer.as_renamer() directly.
JS chunk post-processing and entry point tail generation
src/bundler/linker_context/postProcessJSChunk.rs
post_process_js_chunk and runtime scope borrows are immutable; add_binding_vars_to_module_info takes &Renamer; generate_entry_point_tail_js signature updated to use &LinkerContext and non-mut Renamer; isolated-hash generation errors now propagate.
Statement conversion functions with immutable context
src/bundler/linker_context/convertStmtsForChunk.rs, src/bundler/linker_context/convertStmtsForChunkForDevServer.rs
Both functions now accept immutable &LinkerContext and &Chunk.
Ungate support renamer view
src/bundler/ungate_support.rs
ChunkRenamer::name_for_symbol and as_renamer now take &self and construct Renamer views from immutable access.
JS printer & renamer immutability
src/js_printer/lib.rs, src/js_printer/renamer.rs
RequireOrImportMetaSource trait uses &self; RequireOrImportMeta callback thunk uses shared deref. Renamer variants and helpers, InlineString::slice, and TinyString::slice switched to immutable receivers; several print code paths use non-mut renamer bindings.
Send safety and CSS compilation documentation updates
src/bundler/Chunk.rs, src/bundler/linker_context/generateCompileResultForCssChunk.rs
Updated comments to reflect that parallel printers only form shared borrows and CSS printer does not form &mut LinkerContext/&mut Chunk.

Possibly related PRs

  • oven-sh/bun#31040: Also modifies generate_entry_point_tail_js and related chunk post-processing code in overlapping areas.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and accurately summarizes the main change: avoiding aliased mutable borrows in the chunk codegen path, which is the central focus of this refactoring.
Description check ✅ Passed The PR description comprehensively addresses both required template sections: 'What does this PR do?' is thoroughly explained in the Summary and bullet points, and 'How did you verify your code works?' is detailed in the Verification section with specific commands and test runs.
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.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai 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.

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 `@src/bundler/LinkerContext.rs`:
- Around line 1785-1825: prepare_pretty_paths_for_isolated_hash currently calls
path_with_pretty_initialized(...).expect("OOM") and can abort; change the
function signature to return Result<(), ErrorType> (propagate the appropriate
bundler error type), replace the expect calls with ? so failures from
path_with_pretty_initialized are returned, and update callers (including
generate_isolated_hash) to propagate or handle that Result rather than
panicking; specifically modify prepare_pretty_paths_for_isolated_hash, any loop
branches that call path_with_pretty_initialized and the fallback in
generate_isolated_hash to thread the error via ? through
parse_graph_mut().input_files.items_source_mut() updates instead of calling
expect.
🪄 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: 2c90c1d6-e8a0-4efe-a922-d7d99a47aa21

📥 Commits

Reviewing files that changed from the base of the PR and between f8dcf1a and 3003d75.

📒 Files selected for processing (13)
  • src/ast/symbol.rs
  • src/bundler/Chunk.rs
  • src/bundler/LinkerContext.rs
  • src/bundler/linker_context/convertStmtsForChunk.rs
  • src/bundler/linker_context/convertStmtsForChunkForDevServer.rs
  • src/bundler/linker_context/generateChunksInParallel.rs
  • src/bundler/linker_context/generateCodeForFileInChunkJS.rs
  • src/bundler/linker_context/generateCompileResultForCssChunk.rs
  • src/bundler/linker_context/generateCompileResultForJSChunk.rs
  • src/bundler/linker_context/postProcessJSChunk.rs
  • src/bundler/ungate_support.rs
  • src/js_printer/lib.rs
  • src/js_printer/renamer.rs

Comment thread src/bundler/LinkerContext.rs Outdated
@Dicklesworthstone Dicklesworthstone force-pushed the claude/ub-fix-bundler-chunk-aliasing-v2 branch 2 times, most recently from 1d8fa5b to a70197b Compare May 23, 2026 05:16

@coderabbitai coderabbitai 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.

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 `@src/js_printer/lib.rs`:
- Around line 1479-1495: The init function for RequireOrImportMetaCallback
should require T: Sync to ensure the stored context pointer is safe to share
across threads; update the generic bound on RequireOrImportMetaCallback::init
from T: RequireOrImportMetaSource to T: RequireOrImportMetaSource + Sync and
adjust the inner thunk signature bounds if necessary so the NonNull::from(ctx)
cast and subsequent cross-thread use are covered by Sync; make the same T: Sync
addition wherever init is invoked (e.g., RequireOrImportMetaCallback::init(self)
with self: &LinkerContext) to keep bounds consistent.
🪄 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: dbc80e1c-16f4-47bd-9534-ca8e4e8e30ee

📥 Commits

Reviewing files that changed from the base of the PR and between 3003d75 and 1d8fa5b.

📒 Files selected for processing (13)
  • src/ast/symbol.rs
  • src/bundler/Chunk.rs
  • src/bundler/LinkerContext.rs
  • src/bundler/linker_context/convertStmtsForChunk.rs
  • src/bundler/linker_context/convertStmtsForChunkForDevServer.rs
  • src/bundler/linker_context/generateChunksInParallel.rs
  • src/bundler/linker_context/generateCodeForFileInChunkJS.rs
  • src/bundler/linker_context/generateCompileResultForCssChunk.rs
  • src/bundler/linker_context/generateCompileResultForJSChunk.rs
  • src/bundler/linker_context/postProcessJSChunk.rs
  • src/bundler/ungate_support.rs
  • src/js_printer/lib.rs
  • src/js_printer/renamer.rs

Comment thread src/js_printer/lib.rs Outdated
The UB audit flagged EXP-111 / Bundler B-1..B-5 as a structural aliasing
hazard in the parallel chunk-codegen fan-out. JS/CSS worker callbacks were
rebuilding &mut LinkerContext and &mut Chunk from shared raw pointers even
though the worker path only needs shared graph/chunk reads plus disjoint
compile-result slot writes.

Route JS/CSS part-range codegen through shared LinkerContext/Chunk borrows.
Per-task output still goes through Chunk::write_compile_result_slot, and
per-source byte counters remain atomic. Keep the truly per-chunk postprocess
callbacks as &mut Chunk because each worker owns a distinct chunk pointer.

Make the printer-side renamer view read-only so parallel part-range printers
can borrow the per-chunk ChunkRenamer through &Chunk. SymbolMap::follow() is
now read-only as well; the single-threaded follow_all(&mut self) pass remains
the path-compression point instead of racing on Symbol::link Cell writes.

Preserve the old serial side effects by initializing the browser transpiler and
Source.path.pretty values before worker fan-out. generate_isolated_hash() now
only takes &self during parallel post-processing, with a local fallback for any
future non-prepared call path. Pretty-path initialization failures are returned
through the existing bundler Result path instead of panicking.

Keep the disjoint slot writer on ordinary shared access to the boxed
UnsafeCell slice instead of raw-reading the Box representation. That preserves
the no-&mut-Chunk invariant without relying on a stronger layout assumption
than needed.

Audit witness:
- .ub-exorcism/2026-05-15-exhaustive/phase5_experiment_results/EXP-111-sb.log
- Miri stacked-borrows report: data race detected between retag writes while
  reconstructing &mut Chunk on parallel worker threads.

Verification:
- cargo fmt -p bun_ast -p bun_js_printer -p bun_bundler
- cargo fmt --check -p bun_ast -p bun_js_printer -p bun_bundler
- env CARGO_TARGET_DIR=/tmp/bun-ub-fix-bundler-alias-v2-target cargo check -p bun_bundler
- env CARGO_TARGET_DIR=/tmp/bun-ub-fix-bundler-alias-v2-target cargo check --workspace
- bun bd test test/bundler/transpiler/transpiler.test.js
- bun bd test test/bundler/bundler_barrel.test.ts
- bun bd test test/bundler/bundler_html.test.ts
- git diff --check origin/main...HEAD

Co-Authored-By: Gemini <bot@gemini.local>
@Dicklesworthstone Dicklesworthstone force-pushed the claude/ub-fix-bundler-chunk-aliasing-v2 branch from a70197b to c47a469 Compare May 23, 2026 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants