diff --git a/src/install/PackageInstall.rs b/src/install/PackageInstall.rs index 8cc7a2056efe..84114420cc99 100644 --- a/src/install/PackageInstall.rs +++ b/src/install/PackageInstall.rs @@ -5,7 +5,7 @@ use bun_core::Progress::Progress; use bun_core::{Global, Output}; use bun_core::{MutableString, ZStr}; use bun_paths::strings; -use bun_paths::{self as path, OSPathChar, OSPathSlice, PathBuffer, SEP, SEP_STR}; +use bun_paths::{self as path, OSPathChar, OSPathSlice, PathBuffer, SEP}; use bun_semver::String as SemverString; #[cfg(not(windows))] use bun_sys::OpenDirOptions; @@ -753,6 +753,48 @@ impl UninstallTask { } } +/// `/`, written in place after the alias in +/// `destination_dir_subpath_buf`. Dropping it restores the alias's NUL terminator. +struct DestinationSubpath<'b> { + buf: &'b mut [u8], + alias_len: usize, + len: usize, +} + +impl<'b> DestinationSubpath<'b> { + /// `None` when the path and its NUL terminator do not fit `buf`: the alias is + /// only required to fit the buffer by itself (`alias_is_safe_install_target`). + fn new(buf: &'b mut [u8], alias_len: usize, name: &[u8]) -> Option { + let name_start = alias_len + 1; + let len = name_start + name.len(); + if len >= buf.len() { + return None; + } + buf[alias_len] = SEP; + buf[name_start..len].copy_from_slice(name); + buf[len] = 0; + Some(Self { + buf, + alias_len, + len, + }) + } +} + +impl core::ops::Deref for DestinationSubpath<'_> { + type Target = ZStr; + + fn deref(&self) -> &ZStr { + ZStr::from_buf(self.buf, self.len) + } +} + +impl Drop for DestinationSubpath<'_> { + fn drop(&mut self) { + self.buf[self.alias_len] = 0; + } +} + // ───────────────────────────── impl PackageInstall ───────────────────────────── impl<'a> PackageInstall<'a> { @@ -791,28 +833,17 @@ impl<'a> PackageInstall<'a> { // 1. verify that .bun-tag exists (was it installed from bun?) // 2. check .bun-tag against the resolved version fn verify_git_resolution(&mut self, repo: &Repository, root_node_modules_dir: &Dir) -> bool { - let dest_len = self.destination_dir_subpath.len(); - let suffix: &[u8] = &[SEP, b'.', b'b', b'u', b'n', b'-', b't', b'a', b'g']; - // Reshaped for borrowck — write into buf via raw indices. - self.destination_dir_subpath_buf[dest_len..dest_len + suffix.len()].copy_from_slice(suffix); - self.destination_dir_subpath_buf[dest_len + SEP_STR.len() + b".bun-tag".len()] = 0; - // SAFETY: NUL written above. - let bun_tag_path = unsafe { - ZStr::from_raw_mut( - self.destination_dir_subpath_buf.as_mut_ptr(), - dest_len + SEP_STR.len() + b".bun-tag".len(), - ) + let Some(bun_tag_path) = DestinationSubpath::new( + self.destination_dir_subpath_buf, + self.destination_dir_subpath.len(), + b".bun-tag", + ) else { + return false; }; - let _restore = scopeguard::guard( - self.destination_dir_subpath_buf.as_mut_ptr(), - // SAFETY: p points into destination_dir_subpath_buf which outlives this scope; - // dest_len < buf capacity (was the prior NUL position). - move |p| unsafe { *p.add(dest_len) = 0 }, - ); let Ok(bun_tag_file) = self .node_modules - .read_small_file(root_node_modules_dir, bun_tag_path) + .read_small_file(root_node_modules_dir, &bun_tag_path) else { return false; }; @@ -872,30 +903,15 @@ impl<'a> PackageInstall<'a> { mutable.reset(); mutable.expand_to_capacity(); - let dest_len = self.destination_dir_subpath.len(); - // Write the literal directly into the path buffer; no intermediate Vec. - let suffix: &[u8] = &[ - SEP, b'p', b'a', b'c', b'k', b'a', b'g', b'e', b'.', b'j', b's', b'o', b'n', - ]; - self.destination_dir_subpath_buf[dest_len..dest_len + suffix.len()].copy_from_slice(suffix); - self.destination_dir_subpath_buf[dest_len + SEP_STR.len() + b"package.json".len()] = 0; - // SAFETY: NUL written above. - let package_json_path = unsafe { - ZStr::from_raw_mut( - self.destination_dir_subpath_buf.as_mut_ptr(), - dest_len + SEP_STR.len() + b"package.json".len(), - ) - }; - let _restore = scopeguard::guard( - self.destination_dir_subpath_buf.as_mut_ptr(), - // SAFETY: p points into destination_dir_subpath_buf which outlives this scope; - // dest_len < buf capacity (was the prior NUL position). - move |p| unsafe { *p.add(dest_len) = 0 }, - ); + let package_json_path = DestinationSubpath::new( + self.destination_dir_subpath_buf, + self.destination_dir_subpath.len(), + b"package.json", + )?; let package_json_file = self .node_modules - .open_file(root_node_modules_dir, package_json_path) + .open_file(root_node_modules_dir, &package_json_path) .ok()?; // defer package_json_file.close() diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index 0602dec36d5c..1cecc2d8c0bf 100644 --- a/test/cli/install/bun-install.test.ts +++ b/test/cli/install/bun-install.test.ts @@ -6,6 +6,7 @@ import { bunEnv, bunExe, bunEnv as env, + isLinux, isWindows, joinP, readdirSorted, @@ -897,6 +898,58 @@ describe.concurrent("bun-install", () => { expect(exitCode).toBe(1); }); + // When node_modules already exists, the hoisted installer first checks what is installed at + // node_modules/ by appending "/package.json" (or "/.bun-tag" for git dependencies) to + // the alias inside the path buffer holding it. The alias may be up to one byte short of the + // buffer (4096 bytes on Linux, 1024 on macOS), so aliases a few bytes short of it used to crash + // the install right there instead of failing like any other name the file system rejects. + // On Windows the buffer is far larger than any path the OS accepts. + describe.concurrent.skipIf(isWindows)("dependency alias that fills the path buffer", () => { + const alias = Buffer.alloc((isLinux ? 4096 : 1024) - 6, "a").toString(); + + async function installIntoExistingNodeModules(cwd: string) { + await using proc = spawn({ + // hardlink (the Linux default) creates node_modules/ itself on every platform, so + // the failure is reported the same way on macOS, whose default backend is clonefile. + cmd: [bunExe(), "install", "--linker", "hoisted", "--backend", "hardlink"], + cwd, + env: { ...env, BUN_INSTALL_CACHE_DIR: join(cwd, ".cache") }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toContain("ENAMETOOLONG: failed opening node_modules/package dir for package pkg"); + expect(stdout).toContain("Failed to install 1 package"); + expect(exitCode).toBe(1); + } + + it("file: dependency is verified through /package.json", async () => { + using dir = tempDir("long-alias-file-dep", { + "package.json": JSON.stringify({ name: "app", dependencies: { [alias]: "file:./pkg" } }), + "pkg/package.json": JSON.stringify({ name: "pkg", version: "1.0.0" }), + "node_modules": {}, + }); + + await installIntoExistingNodeModules(String(dir)); + }); + + it("git dependency is verified through /.bun-tag", async () => { + using dir = tempDir("long-alias-git-dep", { + "work/package.json": JSON.stringify({ name: "pkg", version: "1.0.0" }), + "app/node_modules": {}, + }); + await createDumbHttpGitRepo(String(dir), {}); + using server = serveDirectory(String(dir)); + const app = join(String(dir), "app"); + await writeFile( + join(app, "package.json"), + JSON.stringify({ name: "app", dependencies: { [alias]: `git+http://localhost:${server.port}/repo.git` } }), + ); + + await installIntoExistingNodeModules(app); + }); + }); + it("should handle empty string in dependencies", async () => { await withContext(defaultOpts, async ctx => { const urls: string[] = [];