diff --git a/src/install/PackageManager/patchPackage.rs b/src/install/PackageManager/patchPackage.rs index a416be8d9293..c3212eb711df 100644 --- a/src/install/PackageManager/patchPackage.rs +++ b/src/install/PackageManager/patchPackage.rs @@ -1231,8 +1231,8 @@ fn overwrite_package_in_node_modules_folder( let mut copier: FileCopier = FileCopier::init( cached_package_folder.fd, - src_path, - dest_subpath, + src_path.into_checked(), + dest_subpath.into_checked(), ignore_directories, )?; diff --git a/src/install/isolated_install/FileCopier.rs b/src/install/isolated_install/FileCopier.rs index 2cbed827c689..a0f7090374e9 100644 --- a/src/install/isolated_install/FileCopier.rs +++ b/src/install/isolated_install/FileCopier.rs @@ -2,8 +2,7 @@ use core::ptr; use bun_alloc::AllocError; -#[cfg(not(windows))] -use bun_core::{Global, fmt as bun_fmt}; +use bun_paths::path_options::{CheckLength, Kind, PathSeparators}; use bun_paths::{self, OSPathChar, OSPathSlice}; use bun_sys::{self as sys, Dir, E, EntryKind, Fd, walker_skippable, walker_skippable::Walker}; @@ -11,13 +10,11 @@ use bun_sys::{self as sys, Dir, E, EntryKind, Fd, walker_skippable, walker_skipp // u16 on Windows — encoded via `OSPathChar` so `slice()`/`slice_z()` produce // the platform-native width. The auto separator mode normalizes `/` → `\` on Windows // during `from`/`append`, which is load-bearing for the Win32 calls below. +// Length-checked like the Hardlinker's paths: the walker entries appended on Windows are unbounded. type AbsPathAutoOs = - bun_paths::AbsPath; -type PathAutoOs = bun_paths::Path< - OSPathChar, - { bun_paths::path_options::Kind::ANY }, - { bun_paths::path_options::PathSeparators::AUTO }, ->; + bun_paths::AbsPath; +type PathAutoOs = + bun_paths::Path; pub(crate) struct FileCopier { pub(crate) src_path: AbsPathAutoOs, @@ -110,15 +107,17 @@ impl FileCopier { // A `path.save()` ResetScope would hold `&mut Path` and keep // `self.src_path` / `self.dest_subpath` exclusively borrowed - // for the rest of the iteration. Capture the saved length and + // for the rest of the iteration. Capture the saved lengths and // restore via `set_length` after the body instead. let src_saved_len = self.src_path.len(); - let _ = self.src_path.append(entry.path.as_slice()); - let dest_saved_len = self.dest_subpath.len(); - let _ = self.dest_subpath.append(entry.path.as_slice()); + let appended = self.src_path.append(entry.path.as_slice()).is_ok() + && self.dest_subpath.append(entry.path.as_slice()).is_ok(); let result: sys::Result<()> = match entry.kind { + _ if !appended => { + sys::Result::Err(sys::Error::from_code(E::ENAMETOOLONG, sys::Tag::copyfile)) + } EntryKind::Directory => { // SAFETY: FFI — both `slice_z()` are NUL-terminated WStrs. if unsafe { @@ -129,12 +128,11 @@ impl FileCopier { ) } == 0 { - let _ = bun_sys::make_path::make_path::( - &dest_dir, - entry.path.as_slice(), - ); + // Also taken when the directory exists; make_path treats that as success. + bun_sys::make_path::make_path::(&dest_dir, entry.path.as_slice()) + } else { + sys::Result::Ok(()) } - sys::Result::Ok(()) } EntryKind::File => { match bun_sys::copy_file::copy_file( @@ -175,9 +173,7 @@ impl FileCopier { self.src_path.set_length(src_saved_len); self.dest_subpath.set_length(dest_saved_len); - if let sys::Result::Err(err) = result { - return sys::Result::Err(err); - } + result?; } #[cfg(not(windows))] { @@ -194,7 +190,7 @@ impl FileCopier { let dest = match dest_dir.create_file_z(entry.path, Default::default()) { Ok(f) => f, - Err(_) => 'dest: { + Err(_) => { if let Some(entry_dirname) = bun_paths::Dirname::dirname::(entry.path) { @@ -203,27 +199,13 @@ impl FileCopier { entry_dirname, ); } - - match dest_dir.create_file_z(entry.path, Default::default()) { - Ok(f) => break 'dest f, - Err(err) => { - bun_core::pretty_errorln!( - "{}: copy file {}", - bstr::BStr::new(err.name()), - bun_fmt::fmt_os_path(entry.path, Default::default()), - ); - Global::exit(1); - } - } + dest_dir.create_file_z(entry.path, Default::default())? } }; #[cfg(unix)] { - let stat = match bun_sys::fstat(src.handle()) { - sys::Result::Ok(s) => s, - sys::Result::Err(_) => continue, - }; + let stat = bun_sys::fstat(src.handle())?; // SAFETY: fchmod is safe to call with any fd + mode; errors are ignored (`_ =`). unsafe { let _ = bun_sys::c::fchmod(dest.handle().native(), stat.st_mode); diff --git a/src/install/isolated_install/Hardlinker.rs b/src/install/isolated_install/Hardlinker.rs index 4856f2de0c90..2fafc8eedc17 100644 --- a/src/install/isolated_install/Hardlinker.rs +++ b/src/install/isolated_install/Hardlinker.rs @@ -7,16 +7,12 @@ use bun_sys::{self as sys, EntryKind, Fd, FdExt}; // on Windows — encoded here via the `OSPathChar` type alias so the struct's // `slice()`/`slice_z()` produce the platform-native width without per-field // `#[cfg]` divergence. -#[cfg(windows)] -use bun_paths::path_options::AssumeOk as _; +use bun_paths::path_options::{CheckLength, Kind, PathSeparators}; use bun_paths::{AbsPath, OSPathChar, OSPathSlice, Path}; -type OsAbsPath = AbsPath; -type OsPath = Path< - OSPathChar, - { bun_paths::path_options::Kind::ANY }, - { bun_paths::path_options::PathSeparators::AUTO }, ->; +// Length-checked: the walker opens each directory relative to its parent, so entry paths are unbounded. +type OsAbsPath = AbsPath; +type OsPath = Path; pub(crate) struct Hardlinker { pub(crate) src: OsAbsPath, @@ -24,6 +20,10 @@ pub(crate) struct Hardlinker { pub(crate) walker: Walker, } +fn name_too_long() -> sys::Error { + sys::Error::from_code(sys::E::ENAMETOOLONG, sys::Tag::link) +} + impl Hardlinker { pub(crate) fn init( folder_dir: Fd, @@ -97,25 +97,27 @@ impl Hardlinker { // A `path.save()` ResetScope would hold `&mut Path` and keep // `self.src`/`self.dest` exclusively borrowed for the rest of - // the iteration. Capture the saved length directly and restore + // the iteration. Capture the saved lengths directly and restore // via `set_length` after the body (and before any error return) // so the truncation happens on every exit. let src_saved_len = self.src.len(); - // `OsAbsPath`/`OsPath` use `CheckLength::ASSUME`, so `append`'s - // `Err(MaxPathExceeded)` arm is statically unreachable -- see - // `path_options::AssumeOk`. - self.src.append(entry.path.as_slice()).assume_ok(); - let dest_saved_len = self.dest.len(); - self.dest.append(entry.path.as_slice()).assume_ok(); let err: Option = 'body: { + if self.src.append(entry.path.as_slice()).is_err() + || self.dest.append(entry.path.as_slice()).is_err() + { + break 'body Some(name_too_long()); + } + match entry.kind { EntryKind::Directory => { - let _ = sys::make_path::make_path::( + if let sys::Result::Err(mkdir_err) = sys::make_path::make_path::( &sys::Dir::cwd(), self.dest.slice(), - ); + ) { + break 'body Some(mkdir_err); + } } EntryKind::File => { let mut destfile_path_buf = bun_paths::w_path_buffer_pool::get(); @@ -136,6 +138,13 @@ impl Hardlinker { dest_slice, ] }; + // The join and the NT prefix below write into the pooled buffers unchecked. + let needed_len = + dest_parts.iter().map(|part| part.len() + 1).sum::() + + bun_paths::windows::NT_OBJECT_PREFIX.len(); + if needed_len > destfile_path_buf2.len() { + break 'body Some(name_too_long()); + } let joined = bun_paths::resolve_path::join_string_buf_w_same::< bun_paths::platform::Windows, >( @@ -246,12 +255,19 @@ impl Hardlinker { // `len()` and restore via `set_length()` after the body so the // truncation runs on every exit. let dest_saved_len = self.dest.len(); - let _ = self.dest.append(entry.path.as_bytes()); // OOM/capacity: fire-and-forget let err: Option = 'body: { + if self.dest.append(entry.path.as_bytes()).is_err() { + break 'body Some(name_too_long()); + } + match entry.kind { EntryKind::Directory => { - let _ = Fd::cwd().make_path(self.dest.slice()); + if let sys::Result::Err(mkdir_err) = + Fd::cwd().make_path(self.dest.slice()) + { + break 'body Some(mkdir_err); + } } EntryKind::File => { match sys::linkat( diff --git a/src/install/isolated_install/Installer.rs b/src/install/isolated_install/Installer.rs index 6b31bbfd28a5..f34a634e2bbe 100644 --- a/src/install/isolated_install/Installer.rs +++ b/src/install/isolated_install/Installer.rs @@ -917,8 +917,8 @@ impl Task { let mut hardlinker = Hardlinker::init( folder_dir, - src, - dest, + src.into_checked(), + dest.into_checked(), &[bun_paths::os_path_literal!("node_modules")], )?; @@ -1029,8 +1029,8 @@ impl Task { let mut file_copier = FileCopier::init( folder_dir, - src_path.into_sep::<{ PathSeparators::AUTO }>(), - dest.into_sep::<{ PathSeparators::AUTO }>(), + src_path.into_checked(), + dest.into_checked(), &[bun_paths::os_path_literal!("node_modules")], )?; @@ -1338,8 +1338,8 @@ impl Task { let mut hardlinker = Hardlinker::init( cached_package_dir.unwrap(), - src, - dest_subpath, + src.into_checked(), + dest_subpath.into_checked(), &[], )?; @@ -1420,8 +1420,8 @@ impl Task { let mut file_copier = FileCopier::init( cached_package_dir.unwrap(), - src_path.into_sep::<{ PathSeparators::AUTO }>(), - dest_subpath.into_sep::<{ PathSeparators::AUTO }>(), + src_path.into_checked(), + dest_subpath.into_checked(), &[], )?; diff --git a/src/paths/Path.rs b/src/paths/Path.rs index dbfaf781fe3a..d19fc3b31f00 100644 --- a/src/paths/Path.rs +++ b/src/paths/Path.rs @@ -35,7 +35,7 @@ pub mod options { } #[derive(PartialEq, Eq, Clone, Copy, Debug)] - pub(crate) enum CheckLength { + pub enum CheckLength { AssumeAlwaysLessThanMaxPath, CheckForGreaterThanMaxPath, } @@ -80,7 +80,8 @@ pub mod options { } impl CheckLength { pub(crate) const ASSUME: u8 = 0; - pub(crate) const CHECK: u8 = 1; + /// For paths that get unbounded input appended; see also [`Path::into_checked`]. + pub const CHECK: u8 = 1; #[inline(always)] pub(crate) const fn from_u8(v: u8) -> Self { if v == 0 { @@ -852,6 +853,19 @@ impl /// nominally distinct, hence this explicit conversion. #[inline] pub fn into_sep(self) -> Path { + self.reinterpret() + } + + /// [`Self::into_sep`] for `CHECK`: from here on over-long input is `Err(MaxPathExceeded)`. + #[inline] + pub fn into_checked(self) -> Path { + self.reinterpret() + } + + #[inline] + fn reinterpret( + self, + ) -> Path { // Explicit field move (not `transmute`): `Path`/`Buf` are `repr(Rust)`, so // Rust gives no layout-compat guarantee between distinct const-generic // instantiations. Rebuilding field-by-field is layout-agnostic and diff --git a/src/paths/lib.rs b/src/paths/lib.rs index 27218cb3dd16..ef92b6eca491 100644 --- a/src/paths/lib.rs +++ b/src/paths/lib.rs @@ -436,8 +436,7 @@ pub use env_path::{EnvPath, EnvPathInput, PathComponentBuilder}; // ────────────────────────────────────────────────────────────────────────── pub mod windows { /// `\??\` — NT object-manager prefix (UTF-16). - pub(crate) const NT_OBJECT_PREFIX: [u16; 4] = - ['\\' as u16, '?' as u16, '?' as u16, '\\' as u16]; + pub const NT_OBJECT_PREFIX: [u16; 4] = ['\\' as u16, '?' as u16, '?' as u16, '\\' as u16]; /// `\??\UNC\` — NT object-manager UNC prefix (UTF-16). pub(crate) const NT_UNC_OBJECT_PREFIX: [u16; 8] = [ '\\' as u16, diff --git a/test/cli/install/isolated-install-long-paths.test.ts b/test/cli/install/isolated-install-long-paths.test.ts new file mode 100644 index 000000000000..97fd24b77385 --- /dev/null +++ b/test/cli/install/isolated-install-long-paths.test.ts @@ -0,0 +1,216 @@ +// The isolated linker installs a package with a directory walk that opens every +// directory relative to its parent, so it reaches entries whose path is longer +// than the buffer each entry's destination path is built in. Such an entry has +// to fail the package with ENAMETOOLONG (as it does with the hoisted linker) +// instead of crashing or exiting the install, whichever backend ends up +// materializing the package. +import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe, isLinux, isWindows, tempDir } from "harness"; +import { cpSync, mkdirSync, readdirSync, readFileSync, realpathSync, renameSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; + +// Size of the buffers the destination paths are built in: PATH_MAX bytes on +// POSIX (bun_core::MAX_PATH_BYTES), 32767 UTF-16 units on Windows. +const PATH_BUFFER_LEN = isWindows ? 32767 : isLinux ? 4096 : 1024; +// Where the isolated linker materializes the `file:./pkg` dependency used +// below, relative to the project. The destination of an entry is this, a +// separator and the entry's path inside the package. +const STORE_PKG_DIR = join("node_modules", ".bun", "pkg@file+pkg", "node_modules", "pkg"); + +const SEGMENT_LEN = 100; +const SEGMENT = Buffer.alloc(SEGMENT_LEN, "d").toString(); +// createDeepChain never names more directories of a chain than this in one +// path, which keeps everything it hands to the filesystem well below the 1024 +// bytes of macOS. +const CHUNK_DEPTH = 4; + +type Leaf = { name: string; contents: string }; +const LEAF: Leaf = { name: "leaf.js", contents: "module.exports = 'deep';" }; + +/** Directory names whose joined relative path is exactly `joinedLen` long. */ +function chainDirs(joinedLen: number): string[] { + const dirs: string[] = []; + let remaining = joinedLen; + while (remaining > 2 * SEGMENT_LEN + 1) { + dirs.push(SEGMENT); + remaining -= SEGMENT_LEN + 1; + } + dirs.push(Buffer.alloc(remaining, "e").toString()); + expect(dirs.join("/")).toHaveLength(joinedLen); + return dirs; +} + +/** + * Nests `dirs` inside `pkgDir`, optionally with a file in the deepest one. The + * chain may be longer than any path the filesystem accepts: it is created as + * chunks of CHUNK_DEPTH directories side by side in `staging`, and the chunks + * are renamed into each other from the deepest one up, so no single operation + * names more than one chunk of it. + */ +function createDeepChain(pkgDir: string, staging: string, dirs: string[], leaf?: Leaf) { + const chunks: string[][] = []; + for (let i = 0; i < dirs.length; i += CHUNK_DEPTH) { + chunks.push(dirs.slice(i, i + CHUNK_DEPTH)); + } + for (let i = 0; i < chunks.length; i++) { + mkdirSync(join(staging, String(i), ...chunks[i]), { recursive: true }); + } + if (leaf !== undefined) { + const deepest = chunks.length - 1; + writeFileSync(join(staging, String(deepest), ...chunks[deepest], leaf.name), leaf.contents); + } + for (let i = chunks.length - 1; i > 0; i--) { + renameSync(join(staging, String(i), chunks[i][0]), join(staging, String(i - 1), ...chunks[i - 1], chunks[i][0])); + } + renameSync(join(staging, "0", chunks[0][0]), join(pkgDir, chunks[0][0])); +} + +/** A project depending on `file:./pkg`, with `dirs` (and `leaf`) inside pkg. */ +function projectWithDeepFolderDependency(name: string, dirs: string[], leaf?: Leaf) { + const dir = tempDir(name, { + "package.json": JSON.stringify({ name: "proj", dependencies: { pkg: "file:./pkg" } }), + "pkg/package.json": JSON.stringify({ name: "pkg", version: "1.0.0" }), + }); + createDeepChain(join(String(dir), "pkg"), join(String(dir), "staging"), dirs, leaf); + return dir; +} + +async function bunInstall(projectDir: string, ...args: string[]) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "install", "--linker", "isolated", ...args], + cwd: projectDir, + env: { ...bunEnv, BUN_INSTALL_CACHE_DIR: join(projectDir, ".cache") }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; +} + +type InstallResult = Awaited>; + +function expectInstalled({ stdout, stderr, exitCode }: InstallResult) { + expect(stderr).not.toContain("ENAMETOOLONG"); + expect(stdout).toMatch(/\d+ packages? installed/); + expect(exitCode).toBe(0); +} + +function expectPackageFailure({ stdout, stderr, exitCode }: InstallResult, packageName: string) { + expect(stderr).toContain("ENAMETOOLONG"); + expect(stderr).toContain(`failed to link package: ${packageName}@`); + expect(stdout).toContain("Failed to install 1 package"); + expect(exitCode).toBe(1); +} + +/** + * Reads an installed file by its project-relative path from a process running + * in the project, for files whose path joined to the project directory is + * longer than what this process may hand to the filesystem. + */ +async function readInstalledFile(projectDir: string, relativePath: string) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", `process.stdout.write(require("fs").readFileSync(${JSON.stringify(relativePath)}, "utf8"))`], + cwd: projectDir, + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + return stdout; +} + +test.concurrent("a folder dependency with a deep tree that fits installs", async () => { + // Past the 260 characters of Windows' legacy MAX_PATH, well inside the buffer everywhere. + const dirs = chainDirs(isWindows ? 4096 : PATH_BUFFER_LEN / 2); + using dir = projectWithDeepFolderDependency("isolated-deep-fits", dirs, LEAF); + + expectInstalled(await bunInstall(String(dir))); + expect(readFileSync(join(String(dir), "node_modules", "pkg", ...dirs, LEAF.name), "utf8")).toBe(LEAF.contents); +}); + +// On Windows the buffer holds as much as the filesystem does, so no tree that +// exists overflows it with a store-relative path; the Windows test at the +// bottom overflows with the absolute path instead. +describe.skipIf(isWindows)("a folder dependency with a tree deeper than PATH_MAX", () => { + test.concurrent("fails with ENAMETOOLONG when a directory does not fit", async () => { + using dir = projectWithDeepFolderDependency("isolated-deep-dir", chainDirs(PATH_BUFFER_LEN + 256)); + expectPackageFailure(await bunInstall(String(dir)), "pkg"); + }); + + // The two trees below differ by one byte of destination path. Their + // directories fit either way, so the file is the entry that decides. + const leaf: Leaf = { name: "f", contents: LEAF.contents }; + const dirsForDestinationLength = (destinationLen: number) => + chainDirs(destinationLen - (STORE_PKG_DIR.length + 1) - (1 + leaf.name.length)); + + test.concurrent("installs a file whose destination path is one byte short of PATH_MAX", async () => { + const dirs = dirsForDestinationLength(PATH_BUFFER_LEN - 1); + const destination = join(STORE_PKG_DIR, ...dirs, leaf.name); + expect(destination).toHaveLength(PATH_BUFFER_LEN - 1); + using dir = projectWithDeepFolderDependency("isolated-exact-fits", dirs, leaf); + + expectInstalled(await bunInstall(String(dir))); + expect(await readInstalledFile(String(dir), destination)).toBe(leaf.contents); + }); + + test.concurrent("fails with ENAMETOOLONG for a file whose destination path is exactly PATH_MAX", async () => { + const dirs = dirsForDestinationLength(PATH_BUFFER_LEN); + expect(join(STORE_PKG_DIR, ...dirs, leaf.name)).toHaveLength(PATH_BUFFER_LEN); + using dir = projectWithDeepFolderDependency("isolated-exact-over", dirs, leaf); + + expectPackageFailure(await bunInstall(String(dir)), "pkg"); + }); +}); + +// Folder dependencies are always hardlinked; --backend selects how packages +// from the cache are materialized. Install once to populate the cache, add the +// deep tree to the cached copy, and reinstall from it. +describe.skipIf(isWindows)("a cached package with a tree deeper than PATH_MAX", () => { + for (const backend of ["hardlink", "copyfile"]) { + test.concurrent(`fails with ENAMETOOLONG with the ${backend} backend`, async () => { + using dir = tempDir(`isolated-cached-${backend}`, { + "package.json": JSON.stringify({ name: "proj", dependencies: { bar: "file:./bar-0.0.2.tgz" } }), + }); + cpSync(join(import.meta.dir, "bar-0.0.2.tgz"), join(String(dir), "bar-0.0.2.tgz")); + expectInstalled(await bunInstall(String(dir))); + + const cacheEntries = join(String(dir), ".cache", "bar"); + const [cachedPackage] = readdirSync(cacheEntries).map(entry => realpathSync(join(cacheEntries, entry))); + createDeepChain(cachedPackage, join(String(dir), "staging"), chainDirs(PATH_BUFFER_LEN + 256), LEAF); + rmSync(join(String(dir), "node_modules"), { recursive: true }); + + expectPackageFailure(await bunInstall(String(dir), "--backend", backend), "bar"); + }); + } +}); + +// On Windows a file is linked by its absolute destination, `\??\`, the working +// directory and the store path, assembled in a buffer of PATH_BUFFER_LEN units +// after the store-relative part passed its own length check. The package's own +// paths are shorter than their destinations by the store prefix, so a package +// that exists on disk can still have a destination that does not fit. The +// entry below sits in the middle of that window. +test.skipIf(!isWindows)( + "a folder dependency whose absolute destination does not fit fails with ENAMETOOLONG", + async () => { + using dir = tempDir("isolated-absolute-over", { + "package.json": JSON.stringify({ name: "proj", dependencies: { pkg: "file:./pkg" } }), + "pkg/package.json": JSON.stringify({ name: "pkg", version: "1.0.0" }), + }); + const projectDir = realpathSync.native(String(dir)); + const destinationPrefix = `\\??\\${projectDir}\\${STORE_PKG_DIR}\\`; + const sourcePrefix = `\\\\?\\${projectDir}\\pkg\\`; + const entryLen = PATH_BUFFER_LEN - Math.ceil((destinationPrefix.length + sourcePrefix.length) / 2); + expect(destinationPrefix.length + entryLen).toBeGreaterThanOrEqual(PATH_BUFFER_LEN); + expect(sourcePrefix.length + entryLen).toBeLessThan(PATH_BUFFER_LEN); + // A long file name keeps every directory of the chain addressable, so the + // file is the entry that does not fit. + const leaf: Leaf = { name: Buffer.alloc(SEGMENT_LEN, "f").toString(), contents: LEAF.contents }; + const dirs = chainDirs(entryLen - 1 - leaf.name.length); + createDeepChain(join(String(dir), "pkg"), join(String(dir), "staging"), dirs, leaf); + + expectPackageFailure(await bunInstall(String(dir)), "pkg"); + }, +);