diff --git a/src/bundler/LinkerContext.rs b/src/bundler/LinkerContext.rs index 98a8dd7261f4..37c3fc8347b5 100644 --- a/src/bundler/LinkerContext.rs +++ b/src/bundler/LinkerContext.rs @@ -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, 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 { @@ -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(); @@ -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 diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index c51715b569ce..3176b20bdf6b 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -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")), contents: std::borrow::Cow::Borrowed(&b""[..]), ..Default::default() }; @@ -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 @@ -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_core::Error> { use crate::bun_fs::PathResolverExt as _; use crate::bun_node_fallbacks; @@ -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[..]); @@ -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) } } diff --git a/src/bundler/linker_context/postProcessCSSChunk.rs b/src/bundler/linker_context/postProcessCSSChunk.rs index b19a7b2e4d8a..39867549c377 100644 --- a/src/bundler/linker_context/postProcessCSSChunk.rs +++ b/src/bundler/linker_context/postProcessCSSChunk.rs @@ -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 { diff --git a/src/bundler/linker_context/postProcessHTMLChunk.rs b/src/bundler/linker_context/postProcessHTMLChunk.rs index 94d121bf165e..8841bcf5742a 100644 --- a/src/bundler/linker_context/postProcessHTMLChunk.rs +++ b/src/bundler/linker_context/postProcessHTMLChunk.rs @@ -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(()) diff --git a/src/bundler/linker_context/postProcessJSChunk.rs b/src/bundler/linker_context/postProcessJSChunk.rs index 8b964c1f9852..d4d278f6e3fa 100644 --- a/src/bundler/linker_context/postProcessJSChunk.rs +++ b/src/bundler/linker_context/postProcessJSChunk.rs @@ -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); diff --git a/src/resolver/lib.rs b/src/resolver/lib.rs index 7b1a37ebe40a..6cd23eed3863 100644 --- a/src/resolver/lib.rs +++ b/src/resolver/lib.rs @@ -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), + } + } + /// 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, bun_core::Error>; - fn dupe_alloc_fix_pretty(&self) -> Result, 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, bun_core::Error>; + fn dupe_alloc_fix_pretty( + &self, + alloc: &bun_alloc::MimallocArena, + ) -> Result, bun_core::Error>; fn hash_key(&self) -> u64; fn hash_for_kit(&self) -> u64; fn package_name(&self) -> Option<&[u8]>; @@ -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, 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, bun_core::Error> { + // Zig: `isSliceInBuffer` against both stores' backing buffers. + let is_interned = |slice: &[u8]| { + FilenameStore::instance().exists(slice) || DirnameStore::instance().exists(slice) + }; + // 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(); + } + // `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 + } 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); + 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, bun_core::Error> { + fn dupe_alloc_fix_pretty( + &self, + alloc: &bun_alloc::MimallocArena, + ) -> Result, bun_core::Error> { #[cfg(not(windows))] { - self.dupe_alloc() + self.dupe_alloc(alloc) } #[cfg(windows)] { @@ -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 = self.pretty.to_vec(); - bun_paths::resolve_path::platform_to_posix_in_place::(&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::(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) } diff --git a/test/bundler/bun-build-api.test.ts b/test/bundler/bun-build-api.test.ts index 20068c9cff26..4487b0e273a3 100644 --- a/test/bundler/bun-build-api.test.ts +++ b/test/bundler/bun-build-api.test.ts @@ -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 = {}; + 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);