diff --git a/src/install/PackageManager/PackageManagerEnqueue.rs b/src/install/PackageManager/PackageManagerEnqueue.rs index 72aa9715ba81..09f37e1a018d 100644 --- a/src/install/PackageManager/PackageManagerEnqueue.rs +++ b/src/install/PackageManager/PackageManagerEnqueue.rs @@ -1903,18 +1903,25 @@ fn enqueue_local_tarball( // other dependencies (e.g. `appendPackage` / `StringBuilder.allocate` // in `Package.fromNPM`). let mut abs_buf = PathBuffer::uninit(); - let (tarball_path, normalize): (&[u8], bool) = - match local_tarball_base_dir(&this.lockfile, dependency_id, path) { - None => (path, true), - Some(base_dir) => ( - Path::resolve_path::join_abs_string_buf::( - FileSystem::instance().top_level_dir(), - &mut abs_buf, - &[base_dir, path], + let tarball_path = match local_tarball_base_dir(&this.lockfile, dependency_id, path) { + None => Task::TarballPath::Url, + Some(base_dir) => { + match Path::resolve_path::join_abs_string_buf_checked::( + FileSystem::instance().top_level_dir(), + &mut abs_buf, + &[base_dir, path], + ) { + None => Task::TarballPath::TooLong, + Some(joined) => Task::TarballPath::Absolute( + StringOrTinyString::init_append_if_needed( + joined, + &mut crate::network_task::filename_store_appender(), + ) + .expect("unreachable"), ), - false, - ), - }; + } + } + }; // Build the `Task` value *before* claiming a hive slot — the `.expect()`s // below can unwind, and `Task` carries drop glue. See `enqueue_git_clone`. @@ -1949,12 +1956,7 @@ fn enqueue_local_tarball( skip_verify: false, in_trusted_dependencies: false, }, - tarball_path: StringOrTinyString::init_append_if_needed( - tarball_path, - &mut crate::network_task::filename_store_appender(), - ) - .expect("unreachable"), - normalize, + tarball_path, }), }, id: task_id, diff --git a/src/install/PackageManagerTask.rs b/src/install/PackageManagerTask.rs index 82d62768f65d..c4de135122a9 100644 --- a/src/install/PackageManagerTask.rs +++ b/src/install/PackageManagerTask.rs @@ -535,23 +535,12 @@ impl<'a> Task<'a> { this.status = Status::Success; } Tag::LocalTarball => { - // `tarball_path` and `normalize` are computed on the main thread when the - // task is enqueued. This callback runs on a ThreadPool worker and must not - // read `manager.lockfile.packages` / `manager.lockfile.buffers.string_bytes`: - // the main thread may reallocate those buffers concurrently while processing - // other dependencies. + // Worker thread: the request holds everything needed from the lockfile. // SAFETY: tag == LocalTarball discriminates the union - let req = unsafe { &mut *this.request.local_tarball }; - let tarball_path = req.tarball_path.slice(); - let normalize = req.normalize; - - let result = match read_and_extract( - &req.tarball, - tarball_path, - normalize, - &mut this.log, - ) { + let req = unsafe { &*this.request.local_tarball }; + + let result = match read_and_extract(req, &mut this.log) { Ok(v) => v, Err(err) => { this.err = Some(err); @@ -602,29 +591,26 @@ impl<'a> Task<'a> { } } -fn read_and_extract( - tarball: &ExtractTarball, - tarball_path: &[u8], - normalize: bool, - log: &mut Log, -) -> crate::Result { - let bytes = if normalize { +fn read_and_extract(req: &LocalTarballRequest, log: &mut Log) -> crate::Result { + let bytes = match &req.tarball_path { // Resolves // a user-provided relative path against `bun.fs.FileSystem.instance.top_level_dir` // (the absolute project root cached at startup — NOT the live process cwd). // `bun_sys::File::read_from_user_input` takes that base // explicitly (T1 `bun_sys` cannot depend on T5 `bun_resolver::fs`), so thread it // through here from the install crate's `FileSystem` shim. - File::read_from_user_input( + TarballPath::Url => File::read_from_user_input( Fd::cwd(), crate::bun_fs::FileSystem::instance().top_level_dir(), - tarball_path, - )? - } else { - File::read_from(Fd::cwd(), tarball_path)? + req.tarball.url.slice(), + )?, + TarballPath::Absolute(path) => File::read_from(Fd::cwd(), path.slice())?, + TarballPath::TooLong => { + return Err(crate::Error::Sys(bun_errno::SystemErrno::ENAMETOOLONG)); + } }; // `defer allocator.free(bytes)` → Vec drops at scope exit - tarball.run(log, &bytes) + req.tarball.run(log, &bytes) } #[repr(u8)] @@ -699,9 +685,16 @@ pub struct GitCheckoutRequest { pub struct LocalTarballRequest { pub(crate) tarball: ExtractTarball, - /// Resolved by `enqueue_local_tarball` on the main thread; the worker must not read the lockfile. - pub(crate) tarball_path: StringOrTinyString, - /// When true, `tarball_path` is a user-provided path resolved relative to - /// cwd. When false, it is already an absolute path. - pub(crate) normalize: bool, + /// Built on the main thread by `enqueue_local_tarball`; the worker must not read the lockfile. + pub(crate) tarball_path: TarballPath, +} + +/// Where a `LocalTarballRequest` reads its tarball from. +pub(crate) enum TarballPath { + /// `tarball.url` as written in the dependency, resolved against the project root. + Url, + /// A tarball declared by a workspace or `file:` folder package, already joined with that package's directory. + Absolute(StringOrTinyString), + /// The joined path does not fit a `PathBuffer`; the task fails with `ENAMETOOLONG`. + TooLong, } diff --git a/src/install/lockfile/Package.rs b/src/install/lockfile/Package.rs index c3f774d9d18c..1d34898e41d3 100644 --- a/src/install/lockfile/Package.rs +++ b/src/install/lockfile/Package.rs @@ -1968,14 +1968,27 @@ impl Package { } else { 'brk: { let mut buf2 = PathBuffer::uninit(); + let Some(joined) = resolve_path::join_abs_string_buf_checked::< + path::platform::Auto, + >( + FileSystem::instance().top_level_dir(), + &mut buf2.0, + &[source.path.name().dir, workspace], + ) else { + log.add_error_fmt( + source, + value_loc_of(source, key_loc), + format_args!( + "Dependency \"{}\" has an unsafe workspace path", + bstr::BStr::new(external_alias.slice(buf)), + ), + ); + return Err(crate::Error::InstallFailed); + }; let rel = resolve_path::relative_platform::( FileSystem::instance().top_level_dir(), - resolve_path::join_abs_string_buf::( - FileSystem::instance().top_level_dir(), - &mut buf2.0, - &[source.path.name().dir, workspace], - ), + joined, ); #[cfg(windows)] { diff --git a/src/sys/file.rs b/src/sys/file.rs index 42758c842443..c6229be3b9c9 100644 --- a/src/sys/file.rs +++ b/src/sys/file.rs @@ -361,6 +361,7 @@ impl File { /// Normalize a /// user-provided relative path against the resolver's cached /// `top_level_dir` (NOT a fresh `getcwd()`), then `readFrom`. + /// Fails with `ENAMETOOLONG` when the normalized path does not fit a `PathBuffer`. /// /// The cached `top_level_dir` lives in `bun_resolver::fs` (T5), which /// `bun_sys` (T1) must not depend on, so callers pass it explicitly. @@ -371,12 +372,12 @@ impl File { ) -> Maybe> { let dir = dir.as_fd(); let mut buf = bun_paths::PathBuffer::default(); - let normalized = bun_paths::resolve_path::join_abs_string_buf_z::( - top_level_dir, - &mut buf.0, - &[input_path], - ); - Self::read_from(dir, normalized.as_bytes()) + let Some(normalized) = bun_paths::resolve_path::join_abs_string_buf_checked::< + bun_paths::platform::Loose, + >(top_level_dir, &mut buf.0, &[input_path]) else { + return Err(Error::from_code(E::ENAMETOOLONG, Tag::open).with_path(input_path)); + }; + Self::read_from(dir, normalized) } /// `bun.sys.File.writeFile` — open + write + close. pub fn write_file(dir: impl AsFd, path: &ZStr, data: &[u8]) -> Maybe<()> { diff --git a/src/sys/lib.rs b/src/sys/lib.rs index 76c28c6b7102..6330ea682d79 100644 --- a/src/sys/lib.rs +++ b/src/sys/lib.rs @@ -6149,7 +6149,7 @@ pub fn openat_a(dir: impl AsFd, path: &[u8], flags: i32, perm: Mode) -> Maybe= buf.0.len() { - return Err(Error::from_code_int(libc::ENAMETOOLONG, Tag::open).with_path(path)); + return Err(Error::from_code(E::ENAMETOOLONG, Tag::open).with_path(path)); } buf.0[..path.len()].copy_from_slice(path); buf.0[path.len()] = 0; @@ -9209,7 +9209,7 @@ pub fn write_file_with_path_buffer( PathOrFileDescriptor::Fd(fd) => fd, PathOrFileDescriptor::Path(bytes) => { if bytes.len() >= path_buf.0.len() { - return Err(Error::from_code_int(libc::ENAMETOOLONG, Tag::open).with_path(bytes)); + return Err(Error::from_code(E::ENAMETOOLONG, Tag::open).with_path(bytes)); } path_buf.0[..bytes.len()].copy_from_slice(bytes); path_buf.0[bytes.len()] = 0; diff --git a/test/cli/install/bun-workspaces.test.ts b/test/cli/install/bun-workspaces.test.ts index bbfe6925b439..70a272494480 100644 --- a/test/cli/install/bun-workspaces.test.ts +++ b/test/cli/install/bun-workspaces.test.ts @@ -7,6 +7,8 @@ import { assertManifestsPopulated, bunEnv as baseEnv, bunExe, + isLinux, + isWindows, readdirSorted, runBunInstall, toMatchNodeModulesAt, @@ -653,6 +655,39 @@ test.concurrent("cwd in workspace script is not the symlink path on windows", as expect(await file(join(packageDir, "node_modules", "pkg1", "cwd")).text()).toBe(join(packageDir, "pkg1")); }); +// Local dependency paths are joined into a PathBuffer of MAX_PATH_BYTES: 4096 +// bytes on Linux, 1024 on the other POSIX platforms, 32767 * 3 + 1 on Windows. +const PATH_BUFFER_BYTES = isWindows ? 32767 * 3 + 1 : isLinux ? 4096 : 1024; +// Longer than the buffer on every platform. +const LONG_SPEC_BYTES = 100_000; +const WORKSPACE_DIR = "pkgs/pkg1"; + +// Writes a root package with one workspace at WORKSPACE_DIR, declaring `spec` +// as a dependency of either the root package or the workspace package. +async function writeDependency(packageDir: string, owner: "root" | "workspace", name: string, spec: string) { + await Promise.all([ + write( + join(packageDir, "package.json"), + JSON.stringify({ + name: "foo", + workspaces: ["pkgs/*"], + dependencies: owner === "root" ? { [name]: spec } : {}, + }), + ), + write( + join(packageDir, WORKSPACE_DIR, "package.json"), + JSON.stringify({ + name: "pkg1", + dependencies: owner === "workspace" ? { [name]: spec } : {}, + }), + ), + ]); +} + +function runFailingInstall(env: Record, packageDir: string) { + return runBunInstall(env, packageDir, { allowErrors: true, expectedExitCode: 1, savesLockfile: false }); +} + describe("relative tarballs", async () => { test.concurrent("from package.json", async () => { using ctx = await setupTest(); @@ -856,8 +891,111 @@ describe("relative tarballs", async () => { } } }); + + test.concurrent.each(["root", "workspace"] as const)( + "%s dependency on a tarball path longer than the path buffer fails with ENAMETOOLONG", + async owner => { + using ctx = await setupTest(); + const { packageDir, env } = ctx; + const spec = "file:./" + Buffer.alloc(LONG_SPEC_BYTES, "a").toString() + ".tgz"; + await writeDependency(packageDir, owner, "too-long", spec); + + const { err } = await runFailingInstall(env, packageDir); + + expect(err).toContain("error: ENAMETOOLONG extracting tarball from too-long"); + expect(err).toContain("failed to resolve"); + }, + ); + + // A relative tarball path of exactly `bytes` bytes made of one letter + // directories, so that a path which fits the buffer is looked up by the OS + // (ENOENT) instead of tripping its own limit on the length of a single name. + function tarballPathOfLength(bytes: number) { + const tail = bytes % 2 === 0 ? "dd.tgz" : "d.tgz"; + return Buffer.alloc(bytes - tail.length, "d/").toString() + tail; + } + + // packageDir is a real path and the cwd of the install, so the joined path + // is `${packageDir}/${rel}` for the root package and + // `${packageDir}/${WORKSPACE_DIR}/${rel}` for the workspace. One byte below + // the buffer size the path still reaches the OS; at the buffer size there is + // no room for the NUL terminator; above it the join itself overflows. The + // workspace rows also tell the workspace join apart from resolving the same + // path against the root, which fits the buffer and reports ENOENT instead. + test.concurrent.each([ + ["root", "one byte below", -1], + ["root", "exactly", 0], + ["root", "one byte above", 1], + ["workspace", "one byte below", -1], + ["workspace", "exactly", 0], + ["workspace", "one byte above", 1], + ] as const)("%s dependency whose joined tarball path is %s the path buffer size", async (owner, _, offset) => { + using ctx = await setupTest(); + const { packageDir, env } = ctx; + const joinedPrefixBytes = + Buffer.byteLength(packageDir) + 1 + (owner === "workspace" ? Buffer.byteLength(WORKSPACE_DIR) + 1 : 0); + const spec = "file:" + tarballPathOfLength(PATH_BUFFER_BYTES + offset - joinedPrefixBytes); + await writeDependency(packageDir, owner, "edge", spec); + + const { err } = await runFailingInstall(env, packageDir); + + // Windows never looks the fitting path up: it is longer than the 32767 + // UTF-16 units a Windows path may have. + const expected = offset < 0 && !isWindows ? "ENOENT" : "ENAMETOOLONG"; + expect(err).toContain(`error: ${expected} extracting tarball from edge`); + expect(err).toContain("failed to resolve"); + }); + + test.concurrent.each(["root", "workspace"] as const)( + "%s dependency on a tarball path that only fits the path buffer once normalized resolves", + async owner => { + using ctx = await setupTest(); + const { packageDir, env } = ctx; + // "x/../" repeated: longer than the path buffer as written, but it + // normalizes to the tarball next to the root package.json. + const detour = Buffer.alloc(LONG_SPEC_BYTES, "x/../").toString(); + const spec = "./" + detour + (owner === "root" ? "" : "../../") + "qux-0.0.2.tgz"; + await Promise.all([ + writeDependency(packageDir, owner, "qux", spec), + cp(join(import.meta.dir, "qux-0.0.2.tgz"), join(packageDir, "qux-0.0.2.tgz")), + ]); + + // Reading the tarball happens while resolving. Linking it into + // node_modules is skipped: the linker formats the resolution into a + // 512 byte buffer and cannot install a spec this long yet. + await using proc = spawn({ + cmd: [bunExe(), "install", "--lockfile-only"], + cwd: packageDir, + stdout: "pipe", + stderr: "pipe", + env, + }); + const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(err).not.toContain("error:"); + expect(err).toContain("Saved lockfile"); + expect(out).toContain("Saved bun.lock (3 packages)"); + expect(exitCode).toBe(0); + const qux = parseLockfile(packageDir).packages.find((pkg: { name: string }) => pkg.name === "qux"); + expect(qux.resolution).toMatchObject({ tag: "local_tarball", value: spec }); + }, + ); }); +test.concurrent.each(["root", "workspace"] as const)( + "%s dependency on a workspace: path longer than the path buffer fails to install", + async owner => { + using ctx = await setupTest(); + const { packageDir, env } = ctx; + const spec = "workspace:./" + Buffer.alloc(LONG_SPEC_BYTES, "a").toString(); + await writeDependency(packageDir, owner, "too-long", spec); + + const { err } = await runFailingInstall(env, packageDir); + + expect(err).toContain('Dependency "too-long" has an unsafe workspace path'); + }, +); + test.concurrent("$npm_package_config_ works in root", async () => { using ctx = await setupTest(); const { packageDir, env } = ctx;