Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions src/bundler/LinkerContext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -452,12 +452,19 @@ impl<'a> LinkerContext<'a> {
self.graph.arena()
}

/// `arena` must be the arena owned by the thread making this call — on
/// worker threads (chunk post-processing) that is `worker.arena()`, NOT
/// `self.arena()` (the bundle-thread graph arena). `generic_path_with_pretty_initialized`
/// allocates the duped display path from it, and `MimallocArena` asserts
/// single-thread ownership, so passing the wrong arena is a cross-thread
/// allocation (debug panic / release heap corruption).
pub fn path_with_pretty_initialized(
&mut self,
path: &bun_paths::fs::Path<'static>,
arena: &Bump,
) -> Result<bun_paths::fs::Path<'static>, BunError> {
let top_level_dir = bun_resolver::fs::FileSystem::get().top_level_dir;
generic_path_with_pretty_initialized(path, self.options.target, top_level_dir, self.arena())
generic_path_with_pretty_initialized(path, self.options.target, top_level_dir, arena)
}

pub fn should_include_part(&self, source_index: crate::IndexInt, part: &Part) -> bool {
Expand Down Expand Up @@ -1778,7 +1785,7 @@ pub(crate) fn crash_guard_for_part_range(
// and un-gate together with `LinkerGraph.rs`.

impl<'a> LinkerContext<'a> {
pub fn generate_isolated_hash(&mut self, chunk: &Chunk) -> u64 {
pub fn generate_isolated_hash(&mut self, chunk: &Chunk, arena: &Bump) -> u64 {
let _trace = bun::perf::trace("Bundler.generateIsolatedHash");

let mut hasher = ContentHasher::default();
Expand All @@ -1799,7 +1806,7 @@ impl<'a> LinkerContext<'a> {
// independent (relative paths and the "/" path separator)
if source.path.text.as_ptr() == source.path.pretty.as_ptr() {
source.path = self
.path_with_pretty_initialized(&source.path)
.path_with_pretty_initialized(&source.path, arena)
.expect("OOM");
}
// PORT NOTE: `Path::assert_pretty_is_valid` lives on the
Expand Down
10 changes: 5 additions & 5 deletions src/bundler/bundle_v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2468,7 +2468,7 @@ pub mod bv2_impl {
// HTML is only allowed at the entry point.
};
let mut tmp_source = bun_ast::Source {
path: path_as_static(&path.dupe_alloc().expect("oom")),
path: path_as_static(&path.dupe_alloc(self.arena()).expect("oom")),
Comment thread
robobun marked this conversation as resolved.
contents: std::borrow::Cow::Borrowed(&b""[..]),
..Default::default()
};
Expand Down Expand Up @@ -2674,7 +2674,7 @@ pub mod bv2_impl {
// surfacing as "Failed to load bundled module
// 'bun-framework-react/server.tsx'" when the worker can no longer match
// `built_in_modules`.
path = path.dupe_alloc().expect("oom");
path = path.dupe_alloc(self.arena()).expect("oom");
// PORT NOTE: Zig's `var path = result.path()` is a `*Fs.Path` *into*
// `result.path_pair`, so the `path.* = pathWithPrettyInitialized(...)`
// assignment mutates the resolver result in place. The borrowck-reshape
Expand Down Expand Up @@ -7565,7 +7565,7 @@ pub mod bv2_impl {
path: &bun_paths::fs::Path<'static>,
target: options::Target,
top_level_dir: &[u8],
_bump: &bun_alloc::Arena,
bump: &bun_alloc::Arena,
) -> Result<bun_paths::fs::Path<'static>, bun_core::Error> {
use crate::bun_fs::PathResolverExt as _;
use crate::bun_node_fallbacks;
Expand Down Expand Up @@ -7597,7 +7597,7 @@ pub mod bv2_impl {
} else {
path_clone.pretty = rel;
}
path_clone.dupe_alloc_fix_pretty()
path_clone.dupe_alloc_fix_pretty(bump)
} else {
let mut path_clone: crate::bun_fs::Path<'_> = *path;
let mut fbs = bun_io::FixedBufferStream::new_mut(&mut buf.0[..]);
Expand All @@ -7609,7 +7609,7 @@ pub mod bv2_impl {
let _ = fbs.write_all(path_clone.text);
let written = fbs.pos;
path_clone.pretty = &buf.0[..written];
path_clone.dupe_alloc_fix_pretty()
path_clone.dupe_alloc_fix_pretty(bump)
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/bundler/linker_context/postProcessCSSChunk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ pub fn post_process_css_chunk(
bun_core::handle_oom(c.break_output_into_pieces(alloc, &mut j, ctx.chunks.len() as u32));
// TODO: meta contents

chunk.isolated_hash = c.generate_isolated_hash(chunk);
chunk.isolated_hash = c.generate_isolated_hash(chunk, alloc);
// chunk.flags.is_executable = is_executable;

if c.options.source_maps != options::SourceMapOption::None {
Expand Down
2 changes: 1 addition & 1 deletion src/bundler/linker_context/postProcessHTMLChunk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ pub fn post_process_html_chunk(
)); // Zig: `catch |err| bun.handleOom(err)`

// PORT NOTE: reshaped for borrowck (compute hash before assigning into chunk)
let isolated_hash = c.generate_isolated_hash(chunk);
let isolated_hash = c.generate_isolated_hash(chunk, alloc);
chunk.isolated_hash = isolated_hash;

Ok(())
Expand Down
2 changes: 1 addition & 1 deletion src/bundler/linker_context/postProcessJSChunk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -857,7 +857,7 @@ pub fn post_process_js_chunk(

// TODO: meta contents

chunk.isolated_hash = c.generate_isolated_hash(chunk);
chunk.isolated_hash = c.generate_isolated_hash(chunk, worker_arena);
chunk
.flags
.set(crate::chunk::Flags::IS_EXECUTABLE, is_executable);
Expand Down
169 changes: 141 additions & 28 deletions src/resolver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -512,13 +512,33 @@ pub mod fs {
// `bun_wyhash`, `bun_options_types`) remain here as an extension trait.
pub use bun_paths::fs::{Path, PathName};

/// Intern a `Path.namespace` for `dupe_alloc`. The common `file`/empty
/// namespace is a static literal (no allocation); anything else is interned
/// into the process-lifetime `FilenameStore`.
#[inline]
fn dupe_namespace(namespace: &[u8]) -> Result<&'static [u8], bun_core::Error> {
match namespace {
b"" | b"file" => Ok(b"file"),
ns => FilenameStore::instance().append_slice(ns),
}
}
Comment thread
robobun marked this conversation as resolved.

/// Resolver-tier `fs.zig:Path` methods that pull deps `bun_paths` can't
/// reach (`FilenameStore`/`DirnameStore`, `bun_wyhash`, `bun_options_types`,
/// `bun_string`). Import this trait to call `.loader()` / `.dupe_alloc()` /
/// `.hash_key()` on a `Path`.
pub trait PathResolverExt<'a> {
fn dupe_alloc(&self) -> Result<Path<'static>, bun_core::Error>;
fn dupe_alloc_fix_pretty(&self) -> Result<Path<'static>, bun_core::Error>;
/// Intern `text`/`pretty` into the process-lifetime `FilenameStore`,
/// falling back to `alloc` (the per-build bundle arena) for the
/// disjoint-`text`/`pretty` case — see the impl for why.
fn dupe_alloc(
&self,
alloc: &bun_alloc::MimallocArena,
) -> Result<Path<'static>, bun_core::Error>;
fn dupe_alloc_fix_pretty(
&self,
alloc: &bun_alloc::MimallocArena,
) -> Result<Path<'static>, bun_core::Error>;
fn hash_key(&self) -> u64;
fn hash_for_kit(&self) -> u64;
fn package_name(&self) -> Option<&[u8]>;
Expand All @@ -528,36 +548,124 @@ pub mod fs {
impl<'a> PathResolverExt<'a> for Path<'a> {
/// Port of `Path.dupeAlloc` in `fs.zig` — interns `text`/`pretty` into the
/// process-static `FilenameStore` so the returned `Path` borrows `'static`
/// data. PORT NOTE: TYPE_ONLY shim — full overlap/slice-range
/// short-circuiting lives in the gated `fs_full::Path::dupe_alloc`; this
/// always interns.
fn dupe_alloc(&self) -> Result<Path<'static>, bun_core::Error> {
let text = FilenameStore::instance().append_slice(self.text)?;
let pretty: &'static [u8] = if core::ptr::eq(self.text.as_ptr(), self.pretty.as_ptr())
/// data.
///
/// Mirrors the Zig short-circuit: if `text` (and, where relevant,
/// `pretty`) already points into a process-lifetime store
/// (`FilenameStore` or `DirnameStore`), the slices are already `'static`
/// and we return the path unchanged instead of appending a duplicate.
/// Skipping this check makes the append-only `FilenameStore` grow without
/// bound across repeated in-process `Bun.build()` calls, eventually
/// tripping the overflow-block cap (index-out-of-bounds panic).
fn dupe_alloc(
&self,
alloc: &bun_alloc::MimallocArena,
) -> Result<Path<'static>, bun_core::Error> {
// Zig: `isSliceInBuffer` against both stores' backing buffers.
let is_interned = |slice: &[u8]| {
FilenameStore::instance().exists(slice) || DirnameStore::instance().exists(slice)
};
Comment thread
robobun marked this conversation as resolved.
// Returning `self` unchanged widens `text`/`pretty`/`namespace` to
// `'static`; the caller has already proven `text`/`pretty` are
// interned, so assert `namespace` is too (static literal or store-
// interned) — a transient namespace here would dangle.
let return_self_static = || {
debug_assert!(
matches!(self.namespace, b"" | b"file") || is_interned(self.namespace),
"dupe_alloc: returning interned path with transient namespace",
);
// SAFETY: `text`/`pretty` point into a process-lifetime store
// (checked by the caller), and `namespace` is static/interned
// (asserted above). All three outlive the program.
Ok(unsafe { (*self).into_static() })
};

if core::ptr::eq(self.text.as_ptr(), self.pretty.as_ptr())
&& self.text.len() == self.pretty.len()
{
text
if is_interned(self.text) {
return return_self_static();
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// `Path::init` sets `pretty == text`, matching the aliased input.
let text = FilenameStore::instance().append_slice(self.text)?;
let mut new_path = Path::<'static>::init(text);
new_path.namespace = dupe_namespace(self.namespace)?;
new_path.is_symlink = self.is_symlink;
new_path.is_disabled = self.is_disabled;
Ok(new_path)
} else if self.pretty.is_empty() {
b""
if is_interned(self.text) {
return return_self_static();
}
let text = FilenameStore::instance().append_slice(self.text)?;
let mut new_path = Path::<'static>::init(text);
new_path.pretty = b"";
new_path.namespace = dupe_namespace(self.namespace)?;
new_path.is_symlink = self.is_symlink;
new_path.is_disabled = self.is_disabled;
Ok(new_path)
} else if let Some([offset, len]) =
bun_alloc::range_of_slice_in_buffer(self.pretty, self.text)
{
// `pretty` is a sub-slice of `text`.
if is_interned(self.text) {
return return_self_static();
}
let text = FilenameStore::instance().append_slice(self.text)?;
let mut new_path = Path::<'static>::init(text);
new_path.pretty = &text[offset as usize..][..len as usize];
new_path.namespace = dupe_namespace(self.namespace)?;
new_path.is_symlink = self.is_symlink;
new_path.is_disabled = self.is_disabled;
Ok(new_path)
} else {
FilenameStore::instance().append_slice(self.pretty)?
};
let mut new_path = Path::<'static>::init(text);
new_path.pretty = pretty;
new_path.namespace = match self.namespace {
b"" | b"file" => b"file",
ns => FilenameStore::instance().append_slice(ns)?,
};
new_path.is_symlink = self.is_symlink;
new_path.is_disabled = self.is_disabled;
Ok(new_path)
if is_interned(self.text) && is_interned(self.pretty) {
return return_self_static();
}
let mut new_path =
if let Some(offset) = bun_core::strings::index_of(self.text, self.pretty) {
// `text` contains `pretty`; intern `text` once and re-slice.
let text = FilenameStore::instance().append_slice(self.text)?;
let mut p = Path::<'static>::init(text);
p.pretty = &text[offset..][..self.pretty.len()];
p
Comment thread
robobun marked this conversation as resolved.
} else {
// Disjoint `text`/`pretty`. Zig allocates one combined
// `text\0pretty\0` buffer from the per-build arena (NOT the
// process-lifetime `FilenameStore`): `pretty` here is a
// freshly-relativized display path recomputed every build, so
// interning it permanently would leak one copy per
// `Bun.build()` call. The arena is reset per build; every path
// that escapes to JS is copied into an owned buffer first.
let text_len = self.text.len();
let buf: &mut [u8] =
alloc.alloc_slice_fill_copy(text_len + self.pretty.len() + 2, 0u8);
Comment thread
robobun marked this conversation as resolved.
buf[..text_len].copy_from_slice(self.text);
buf[text_len + 1..text_len + 1 + self.pretty.len()]
.copy_from_slice(self.pretty);
// SAFETY: arena memory lives for the whole bundle pass; the
// consuming `Path` (graph/import-record) never outlives it.
let buf: &'static [u8] =
unsafe { core::slice::from_raw_parts(buf.as_ptr(), buf.len()) };
let mut p = Path::<'static>::init(&buf[..text_len]);
p.pretty = &buf[text_len + 1..text_len + 1 + self.pretty.len()];
p
};
new_path.namespace = dupe_namespace(self.namespace)?;
new_path.is_symlink = self.is_symlink;
new_path.is_disabled = self.is_disabled;
Ok(new_path)
}
}

/// Port of `Path.dupeAllocFixPretty` in `fs.zig`.
fn dupe_alloc_fix_pretty(&self) -> Result<Path<'static>, bun_core::Error> {
fn dupe_alloc_fix_pretty(
&self,
alloc: &bun_alloc::MimallocArena,
) -> Result<Path<'static>, bun_core::Error> {
#[cfg(not(windows))]
{
self.dupe_alloc()
self.dupe_alloc(alloc)
}
#[cfg(windows)]
{
Expand All @@ -566,14 +674,19 @@ pub mod fs {
// Short-circuiting preserves the `pretty.ptr == text.ptr` aliasing
// optimisation inside `dupe_alloc` and avoids a fresh FilenameStore alloc.
if !self.pretty.iter().any(|&b| b == b'\\') {
return self.dupe_alloc();
return self.dupe_alloc(alloc);
}
let mut new = self.clone();
new.pretty = b"";
let mut new = new.dupe_alloc()?;
let mut owned: Vec<u8> = self.pretty.to_vec();
bun_paths::resolve_path::platform_to_posix_in_place::<u8>(&mut owned);
new.pretty = FilenameStore::instance().append_slice(&owned)?;
let mut new = new.dupe_alloc(alloc)?;
// Zig: `allocator.dupe(u8, this.pretty)` — the posix-normalized
// display path goes into the per-build arena, not the
// process-lifetime `FilenameStore` (it is recomputed each build).
let pretty: &mut [u8] = alloc.alloc_slice_copy(self.pretty);
bun_paths::resolve_path::platform_to_posix_in_place::<u8>(pretty);
// SAFETY: arena memory lives for the whole bundle pass; the
// consuming `Path` never outlives it.
new.pretty = unsafe { core::slice::from_raw_parts(pretty.as_ptr(), pretty.len()) };
new.assert_pretty_is_valid();
Ok(new)
}
Expand Down
62 changes: 62 additions & 0 deletions test/bundler/bun-build-api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1329,3 +1329,65 @@ test.skip("Bun.build NumberRenamer does not leak intermediate NumberScope.name_c
expect(growth).toBeLessThan(48 * 1024 * 1024);
expect(exitCode).toBe(0);
}, 120_000);

// Regression: repeated in-process `Bun.build()` calls panicked with
// `index out of bounds: the len is 4095 but the index is 4095` (SIGTRAP) after
// a couple thousand builds. `Path.dupeAlloc` interns every module path into the
// process-lifetime `FilenameStore`. The Rust port had dropped two things the
// Zig original does: (1) the `isSliceInBuffer` short-circuit that returns an
// already-interned path unchanged, and (2) routing the disjoint `text`/`pretty`
// case (a freshly-relativized display path, recomputed every build) into the
// per-build arena instead of the store. Without them, each build re-appended
// every path, and once the store's overflow blocks filled
// (`OVERFLOW_GROUP_MAX` = 4095 blocks), the next append indexed one past the
// fixed-capacity pointer array and panicked.
//
// Many modules per build reaches the cap in far fewer builds: with 500 modules
// the broken binary panics roughly a third of the way through this loop, while
// the fixed binary keeps the store bounded and exits cleanly after all 400.
// (MODULES stays well under the ~550 where the unrelated recursive tree-shaker
// overflows its thread stack.) Not gated to debug/ASAN — the panic reproduces
// on release builds too.
//
// An explicit timeout is required (not optional): this runs hundreds of real
// bundles, far past bun:test's 5s default. The sibling leak tests above do the
// same. 180s matches the CI runner's own per-test ceiling.
test("Bun.build can be called thousands of times in one process without crashing", async () => {
const MODULES = 500;
const BUILDS = 400;
const files: Record<string, string> = {};
for (let i = 0; i < MODULES; i++) {
files[`m${i}.js`] =
`import { f${(i + 1) % MODULES} } from "./m${(i + 1) % MODULES}.js";\n` +
`export const v${i} = ${i};\n` +
`export function f${i}() { return v${i}; }\n`;
}
files["entry.js"] = Array.from(
{ length: MODULES },
(_, i) => `import { f${i} } from "./m${i}.js"; console.log(f${i}());`,
).join("\n");
files["run.ts"] = `
const entry = process.argv[2];
const BUILDS = ${BUILDS};
for (let i = 1; i <= BUILDS; i++) {
const res = await Bun.build({ entrypoints: [entry], minify: true, sourcemap: "external" });
if (!res.success) throw new AggregateError(res.logs, "build failed");
for (const o of res.outputs) await o.arrayBuffer();
}
console.log("OK " + BUILDS);
`;
const dir = tempDirWithFiles("bun-build-filename-store-overflow", files);

await using proc = Bun.spawn({
cmd: [bunExe(), join(dir, "run.ts"), join(dir, "entry.js")],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
// A crash surfaces as a non-zero (signal) exit and a panic on stderr; assert
// the run completed cleanly instead.
expect(stderr).toBe("");
expect(stdout.trim()).toBe("OK 400");
expect(exitCode).toBe(0);
}, 180_000);
Loading