diff --git a/src/install/PackageManager/patchPackage.rs b/src/install/PackageManager/patchPackage.rs index 04dfd2bc0100..48819662409c 100644 --- a/src/install/PackageManager/patchPackage.rs +++ b/src/install/PackageManager/patchPackage.rs @@ -371,6 +371,7 @@ pub fn do_patch_commit( random_tempdir.as_bytes(), sys::RenameOptions { move_fallback: true, + ..Default::default() }, ) .is_err() @@ -426,6 +427,7 @@ pub fn do_patch_commit( patch_tag_tmpname.as_bytes(), sys::RenameOptions { move_fallback: true, + ..Default::default() }, ) { bun_core::warn!( @@ -458,7 +460,7 @@ pub fn do_patch_commit( random_tempdir.as_bytes(), new_folder_handle.fd, b"node_modules", - sys::RenameOptions { move_fallback: true }, + sys::RenameOptions { move_fallback: true, ..Default::default() }, ) { bun_core::warn!("failed renaming nested node_modules folder, this may cause issues: {}", e); } @@ -470,7 +472,7 @@ pub fn do_patch_commit( patch_tag_tmpname.as_bytes(), new_folder_handle.fd, patch_tag, - sys::RenameOptions { move_fallback: true }, + sys::RenameOptions { move_fallback: true, ..Default::default() }, ) { bun_core::warn!("failed renaming the bun patch tag, this may cause issues: {}", e); } @@ -621,6 +623,7 @@ pub fn do_patch_commit( path_in_patches_dir, sys::RenameOptions { move_fallback: true, + ..Default::default() }, ) { Output::err(e, "failed renaming patch file to patches dir", ()); diff --git a/src/install/extract_tarball.rs b/src/install/extract_tarball.rs index dca3b11ca75d..68b94b60feef 100644 --- a/src/install/extract_tarball.rs +++ b/src/install/extract_tarball.rs @@ -258,6 +258,11 @@ impl ExtractTarball { let mut resolved: &'static [u8] = b""; let tmpname = FileSystem::tmpname(tmpname_suffix, &mut tmpname_buf.0, bun_core::fast_random())?; + // Delete the temp dir if extraction fails before it's renamed into the + // cache; defused on success. + let tmpdir_cleanup = scopeguard::guard((), |()| { + let _ = Dir::borrow(&self.temp_dir).delete_tree(tmpname.as_bytes()); + }); { let extract_destination = match bun_sys::make_path::make_open_path( tmpdir, @@ -468,7 +473,11 @@ impl ExtractTarball { } } - self.move_to_cache_directory(log, tmpname, name, basename, resolved) + let result = self.move_to_cache_directory(log, tmpname, name, basename, resolved); + if result.is_ok() { + scopeguard::ScopeGuard::into_inner(tmpdir_cleanup); + } + result } /// Rename the freshly-extracted temp directory into the cache, read @@ -544,6 +553,27 @@ impl ExtractTarball { } let cache_dir = Dir::borrow(&self.cache_dir); + // An existing npm cache entry without package.json is invalid (the + // same check `package_missing_from_cache` uses), so no concurrent + // install reads from it. Delete it so the fresh copy below replaces + // it instead of being kept as an equivalent existing destination. + if self.resolution.tag == ResolutionTag::Npm { + let mut folder_name_z_buf = PathBuffer::uninit(); + folder_name_z_buf[..folder_name.len()].copy_from_slice(folder_name); + folder_name_z_buf[folder_name.len()] = 0; + let folder_name_z = ZStr::from_buf(&folder_name_z_buf, folder_name.len()); + if sys::directory_exists_at(cache_dir.fd(), folder_name_z).unwrap_or(false) { + let mut json_buf = PathBuffer::uninit(); + let json_z = path::resolve_path::join_z_buf::( + &mut json_buf.0, + &[folder_name, b"package.json"], + ); + if !sys::exists_at(cache_dir.fd(), json_z) { + let _ = cache_dir.delete_tree(folder_name); + } + } + } + // e.g. @next // if it's a namespace package, we need to make sure the @name folder exists let create_subdir = basename.len() != name.len() && !self.resolution.tag.is_git(); @@ -609,39 +639,22 @@ impl ExtractTarball { | sys::Errno::PERM | sys::Errno::BUSY | sys::Errno::EXIST => { - // before we attempt to delete the destination, let's close the source dir. let _ = sys::close(dir_to_move); - // We tried to move the folder over - // but it didn't work! - // so instead of just simply deleting the folder - // we rename it back into the temp dir - // and then delete that temp dir - // The goal is to make it more difficult for an application to reach this folder - let mut tempdest_buf = PathBuffer::uninit(); - tempdest_buf[0..tmpname.len()] - .copy_from_slice(tmpname.as_bytes()); - tempdest_buf[tmpname.len()..][0..4] - .copy_from_slice(&[b't', b'm', b'p', 0]); - let tempdest = - ZStr::from_buf(&tempdest_buf, tmpname.len() + 3); - let mut folder_name_z_buf = PathBuffer::uninit(); - folder_name_z_buf[0..folder_name.len()] - .copy_from_slice(folder_name); - folder_name_z_buf[folder_name.len()] = 0; - let folder_name_z = - ZStr::from_buf(&folder_name_z_buf, folder_name.len()); - match sys::renameat( + // A concurrent install sharing the cache published + // this entry first. Keep it (it may already have + // readers) and drop our equivalent copy instead of + // renaming it out from under them. + if sys::directory_exists_at_w( Fd::from_std_dir(cache_dir), - folder_name_z, - Fd::from_std_dir(tmpdir), - tempdest, - ) { - bun_sys::Result::Err(_) => {} - bun_sys::Result::Ok(_) => { - let _ = tmpdir.delete_tree(tempdest.as_bytes()); - } + path_to_use, + ) + .unwrap_or(false) + { + let _ = tmpdir.delete_tree(tmpname.as_bytes()); + break; } + retries += 1; // 10ms, 20ms, 40ms, 80ms — long enough // for a concurrent close to land, @@ -683,9 +696,9 @@ impl ExtractTarball { // // By: // 1. Rename from temporary directory to cache directory and fail if it already exists - // 2a. If the rename fails, swap the cache directory with the temporary directory version - // 2b. Delete the temporary directory version ONLY if we're not using a provided temporary directory - // 3. If rename still fails, fallback to racily deleting the cache directory version and then renaming the temporary directory version again. + // 2. If the rename fails because the destination exists, a concurrent install + // published an equivalent copy first: keep it and delete the temporary + // directory version (`keep_existing_destination`). // if create_subdir { @@ -701,6 +714,7 @@ impl ExtractTarball { folder_name, sys::RenameatConcurrentlyOptions { move_fallback: true, + keep_existing_destination: true, }, ) { log.add_error_fmt( diff --git a/src/install/patch_install.rs b/src/install/patch_install.rs index 8f3b3012f57e..a7f4a102c97e 100644 --- a/src/install/patch_install.rs +++ b/src/install/patch_install.rs @@ -452,6 +452,12 @@ impl PatchTask { let system_tmpdir = self.tempdir; + // Delete the temp dir on failure; after a successful rename into the + // cache this is a no-op. + scopeguard::defer! { + let _ = sys::Dir::borrow(&system_tmpdir).delete_tree(tempdir_name.as_bytes()); + } + let pkg_name = patch.pkgname; let dummy_node_modules = crate::package_installer::NodeModulesFolder { @@ -606,7 +612,7 @@ impl PatchTask { cache_dir_subpath_z, sys::RenameOptions { move_fallback: true, - ..Default::default() + keep_existing_destination: true, }, ) { log.add_error_fmt_opts( diff --git a/src/sys/lib.rs b/src/sys/lib.rs index 68546dbdcd82..1ad81854d7ed 100644 --- a/src/sys/lib.rs +++ b/src/sys/lib.rs @@ -9225,7 +9225,13 @@ pub fn exists(path: &[u8]) -> bool { /// retries; on EXDEV falls back to the slow open+copy path. Only opens the /// source inside the EXDEV branch. pub fn move_file_z(from_dir: Fd, filename: &ZStr, to_dir: Fd, destination: &ZStr) -> Maybe<()> { - match renameat_concurrently_without_fallback(from_dir, filename, to_dir, destination) { + match renameat_concurrently_without_fallback( + from_dir, + filename, + to_dir, + destination, + Default::default(), + ) { Ok(()) => Ok(()), // allow over-writing an empty directory Err(e) if e.get_errno() == E::EISDIR => { @@ -9302,6 +9308,12 @@ pub fn renameat_z(from_dir: impl AsFd, from: &ZStr, to_dir: impl AsFd, to: &ZStr #[derive(Default, Clone, Copy)] pub struct RenameatConcurrentlyOptions { pub move_fallback: bool, + /// If the destination already exists (a concurrent process published an + /// equivalent tree first, e.g. a shared package cache entry), keep it and + /// delete the source instead of atomically replacing it. Replacing would + /// yank entries out from under processes still reading the existing tree, + /// and swapping it into the source leaks the temp directory (#33977). + pub keep_existing_destination: bool, } /// Alias: `bun_install` call sites spell this `RenameOptions`. pub type RenameOptions = RenameatConcurrentlyOptions; @@ -9328,7 +9340,7 @@ pub fn renameat_concurrently( to: &ZStr, opts: RenameatConcurrentlyOptions, ) -> Maybe<()> { - match renameat_concurrently_without_fallback(from_dir_fd, from, to_dir_fd, to) { + match renameat_concurrently_without_fallback(from_dir_fd, from, to_dir_fd, to, opts) { Ok(()) => Ok(()), Err(e) => { if opts.move_fallback && e.get_errno() == E::EXDEV { @@ -9348,7 +9360,15 @@ pub fn renameat_concurrently_without_fallback( from: &ZStr, to_dir_fd: Fd, to: &ZStr, + opts: RenameatConcurrentlyOptions, ) -> Maybe<()> { + let delete_source = || { + if from_dir_fd.is_valid() { + let _ = Dir::borrow(&from_dir_fd).delete_tree(from.as_bytes()); + } else { + let _ = delete_tree_absolute(from.as_bytes()); + } + }; 'attempt: { { // Happy path: the folder doesn't exist in the cache dir, so we can @@ -9373,6 +9393,14 @@ pub fn renameat_concurrently_without_fallback( Ok(()) => break 'attempt, }; + if opts.keep_existing_destination && matches!(err.get_errno(), E::EEXIST | E::ENOTEMPTY) + { + // Another process won the race; its tree is equivalent to + // ours and may already have readers, so drop our copy. + delete_source(); + break 'attempt; + } + // Windows doesn't have any equivalent of renameat with swap #[cfg(not(windows))] { @@ -9401,6 +9429,21 @@ pub fn renameat_concurrently_without_fallback( } // sad path: let's try to delete the folder and then rename it + if opts.keep_existing_destination { + // The errno didn't tell us whether the destination exists (e.g. + // EOPNOTSUPP when the filesystem lacks RENAME_NOREPLACE); check + // before deleting a tree another process may be reading. Windows + // `exists_at` is file-only, so ask for the directory explicitly. + let dir_fd = if to_dir_fd.is_valid() { + to_dir_fd + } else { + Fd::cwd() + }; + if directory_exists_at(dir_fd, to).unwrap_or(false) { + delete_source(); + break 'attempt; + } + } if to_dir_fd.is_valid() { let _ = Dir::borrow(&to_dir_fd).delete_tree(to.as_bytes()); } else { @@ -9742,6 +9785,7 @@ mod owned_handle_tests { b"sub", RenameatConcurrentlyOptions { move_fallback: true, + ..Default::default() }, ) .expect("rename"); @@ -9757,6 +9801,51 @@ mod owned_handle_tests { let _ = close(root); let _ = Dir::open(&tmp).map(|d| d.delete_tree(b".")); } + + /// With `keep_existing_destination`, losing the publish race keeps the + /// destination untouched and deletes the source instead of swapping the + /// two (which stranded the swapped-out tree in the temp dir, #33977). + #[test] + fn renameat_concurrently_keep_existing_destination() { + let _g = crate::file::tests::FD_TEST_LOCK.lock(); + let mut tmp = std::env::temp_dir().as_os_str().as_encoded_bytes().to_vec(); + tmp.extend_from_slice(b"/bun_sys_renameat_keep_test"); + let _ = open_dir_at(Fd::cwd(), &tmp).map(close); + let _ = mkdir_recursive_at(Fd::cwd(), &tmp); + let root = open_dir_at(Fd::cwd(), &tmp).expect("open root"); + let _ = mkdir_recursive_at(root, b"from/sub"); + let _ = mkdir_recursive_at(root, b"to/sub"); + let to_dir = open_dir_at(root, b"to").expect("open to"); + File::write_file(root, ZStr::from_static(b"to/sub/winner\0"), b"").expect("marker"); + + renameat_concurrently_a( + root, + b"from/sub", + to_dir, + b"sub", + RenameatConcurrentlyOptions { + move_fallback: true, + keep_existing_destination: true, + }, + ) + .expect("rename"); + + // The existing destination survives with its contents... + assert!( + exists_at(to_dir, ZStr::from_static(b"sub/winner\0")), + "existing destination was replaced" + ); + // ...and the source was cleaned up rather than left (or swapped) + // behind. Windows `exists_at` is file-only, so check the directory. + assert!( + !directory_exists_at(root, ZStr::from_static(b"from/sub\0")).unwrap_or(false), + "source left behind" + ); + + let _ = close(to_dir); + let _ = close(root); + let _ = Dir::open(&tmp).map(|d| d.delete_tree(b".")); + } } #[cfg(all(test, windows))] diff --git a/test/cli/install/bun-install-tempdir-cleanup.test.ts b/test/cli/install/bun-install-tempdir-cleanup.test.ts new file mode 100644 index 000000000000..1fbf5e964b96 --- /dev/null +++ b/test/cli/install/bun-install-tempdir-cleanup.test.ts @@ -0,0 +1,259 @@ +// https://github.com/oven-sh/bun/issues/33977 +// `bun install` must not leave package-extraction temp directories behind in +// the install temp dir ($BUN_TMPDIR / $TMPDIR): neither when two concurrent +// installs race the same cache entry (the RENAME_EXCHANGE fallback used to +// strand the loser's copy), nor when extraction or patching fails partway. + +import { expect, setDefaultTimeout, test } from "bun:test"; +import { bunEnv, bunExe, readdirSorted, tempDir } from "harness"; +import { createHash } from "node:crypto"; +import { rm } from "node:fs/promises"; +import { join } from "node:path"; +import { gzipSync } from "node:zlib"; + +setDefaultTimeout(1000 * 60 * 5); + +// --------------------------------------------------------------------------- +// Minimal in-process tarball + registry helpers (no binary fixtures). +// --------------------------------------------------------------------------- + +function octal(n: number, width: number): string { + return n.toString(8).padStart(width - 1, "0") + "\0"; +} + +function tarHeader(name: string, size: number, type: "0" | "5"): Buffer { + const buf = Buffer.alloc(512, 0); + buf.write(name, 0, 100, "utf8"); + buf.write(octal(0o644, 8), 100); // mode + buf.write(octal(0, 8), 108); // uid + buf.write(octal(0, 8), 116); // gid + buf.write(octal(size, 12), 124); // size + buf.write(octal(0, 12), 136); // mtime + buf.fill(" ", 148, 156); // checksum placeholder + buf.write(type, 156); + buf.write("ustar\0", 257); + buf.write("00", 263); + let sum = 0; + for (let i = 0; i < 512; i++) sum += buf[i]; + buf.write(octal(sum, 8), 148); + return buf; +} + +function pad512(len: number): Buffer { + return Buffer.alloc((512 - (len % 512)) % 512, 0); +} + +function buildTarball(entries: { path: string; body: Buffer }[]): { tgz: Buffer; integrity: string } { + const blocks: Buffer[] = []; + for (const { path, body } of entries) { + blocks.push(tarHeader(`package/${path}`, body.length, "0"), body, pad512(body.length)); + } + blocks.push(Buffer.alloc(1024, 0)); // end-of-archive + const tgz = gzipSync(Buffer.concat(blocks)); + return { tgz, integrity: "sha512-" + createHash("sha512").update(tgz).digest("base64") }; +} + +// A package with enough files that extraction takes long enough for two +// concurrent installs to reliably race the same cache entry. +function makePackageTarball(name: string, fileCount: number) { + const entries = [{ path: "package.json", body: Buffer.from(JSON.stringify({ name, version: "1.0.0" }) + "\n") }]; + for (let i = 0; i < fileCount; i++) { + // Incompressible content so gzip can't collapse the files away. + const body = Buffer.alloc(1024); + let seed = createHash("sha256").update(`${name}-${i}`).digest(); + for (let off = 0; off < body.length; off += 32) { + seed.copy(body, off); + seed = createHash("sha256").update(seed).digest(); + } + entries.push({ path: `files/f${i}.bin`, body }); + } + return buildTarball(entries); +} + +// Serves packuments at / and tarballs at //-/-1.0.0.tgz. +function makeRegistry(packages: Record) { + const server = Bun.serve({ + port: 0, + fetch(req) { + const { pathname } = new URL(req.url); + for (const [name, { tgz, integrity }] of Object.entries(packages)) { + if (pathname === `/${name}`) { + return Response.json({ + name, + "dist-tags": { latest: "1.0.0" }, + versions: { + "1.0.0": { + name, + version: "1.0.0", + dist: { + integrity, + tarball: `${server.url}${name}/-/${name}-1.0.0.tgz`, + }, + }, + }, + }); + } + if (pathname === `/${name}/-/${name}-1.0.0.tgz`) { + return new Response(tgz); + } + } + return new Response("not found", { status: 404 }); + }, + }); + return server; +} + +async function runInstall(cwd: string, cacheDir: string, tmpDir: string, extraArgs: string[] = ["--no-save"]) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "install", "--linker=hoisted", ...extraArgs], + cwd, + env: { + ...bunEnv, + BUN_INSTALL_CACHE_DIR: cacheDir, + BUN_TMPDIR: tmpDir, + }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; +} + +test.concurrent("concurrent installs sharing a cache do not leak temp directories", async () => { + const packageCount = 8; + const packages: Record = {}; + for (let i = 0; i < packageCount; i++) { + packages[`leaky-pkg-${i}`] = makePackageTarball(`leaky-pkg-${i}`, 150); + } + using server = makeRegistry(packages); + + const dependencies = Object.fromEntries(Object.keys(packages).map(name => [name, "1.0.0"])); + const files: Record = { "tmp/.keep": "", "cache/.keep": "" }; + for (const proj of ["proj1", "proj2"]) { + files[`${proj}/package.json`] = JSON.stringify({ name: proj, version: "1.0.0", dependencies }); + files[`${proj}/bunfig.toml`] = `[install]\nregistry = "${server.url}"\n`; + } + using dir = tempDir("tempdir-leak", files); + const tmpDir = join(String(dir), "tmp"); + const cacheDir = join(String(dir), "cache"); + + for (let iteration = 0; iteration < 5; iteration++) { + // Evict the cache so both installs extract (and race) every package again. + await Promise.all([ + rm(cacheDir, { recursive: true, force: true }), + rm(join(String(dir), "proj1", "node_modules"), { recursive: true, force: true }), + rm(join(String(dir), "proj2", "node_modules"), { recursive: true, force: true }), + ]); + + const [r1, r2] = await Promise.all([ + runInstall(join(String(dir), "proj1"), cacheDir, tmpDir), + runInstall(join(String(dir), "proj2"), cacheDir, tmpDir), + ]); + expect({ stderr: r1.stderr, exitCode: r1.exitCode }).toMatchObject({ exitCode: 0 }); + expect({ stderr: r2.stderr, exitCode: r2.exitCode }).toMatchObject({ exitCode: 0 }); + } + + expect(await readdirSorted(tmpDir)).toEqual([".keep"]); +}); + +test.concurrent("a tarball that fails to extract does not leak its temp directory", async () => { + // Valid integrity (computed over the bytes) but not a gzip stream, so the + // failure happens during extraction, after the temp dir was created. + const tgz = Buffer.from("this is definitely not a gzipped tarball"); + using server = makeRegistry({ + "corrupt-pkg": { tgz, integrity: "sha512-" + createHash("sha512").update(tgz).digest("base64") }, + }); + + using dir = tempDir("tempdir-leak-corrupt", { + "proj/package.json": JSON.stringify({ + name: "proj", + version: "1.0.0", + dependencies: { "corrupt-pkg": "1.0.0" }, + }), + "proj/bunfig.toml": `[install]\nregistry = "${server.url}"\n`, + "tmp/.keep": "", + "cache/.keep": "", + }); + const tmpDir = join(String(dir), "tmp"); + + const { stderr, exitCode } = await runInstall(join(String(dir), "proj"), join(String(dir), "cache"), tmpDir); + expect(stderr).toContain("corrupt-pkg"); + + expect(await readdirSorted(tmpDir)).toEqual([".keep"]); + expect(exitCode).not.toBe(0); +}); + +test.concurrent("a patch that fails to apply does not leak its temp directory", async () => { + using server = makeRegistry({ "patched-pkg": makePackageTarball("patched-pkg", 3) }); + + // Parses fine, but targets a file the package doesn't contain. + const patch = [ + "diff --git a/missing.txt b/missing.txt", + "index 0000000..1111111 100644", + "--- a/missing.txt", + "+++ b/missing.txt", + "@@ -1 +1 @@", + "-old", + "+new", + "", + ].join("\n"); + + using dir = tempDir("tempdir-leak-patch", { + "proj/package.json": JSON.stringify({ + name: "proj", + version: "1.0.0", + dependencies: { "patched-pkg": "1.0.0" }, + patchedDependencies: { "patched-pkg@1.0.0": "patches/patched-pkg.patch" }, + }), + "proj/patches/patched-pkg.patch": patch, + "proj/bunfig.toml": `[install]\nregistry = "${server.url}"\n`, + "tmp/.keep": "", + "cache/.keep": "", + }); + const tmpDir = join(String(dir), "tmp"); + + const { stderr, exitCode } = await runInstall(join(String(dir), "proj"), join(String(dir), "cache"), tmpDir); + expect(stderr).toContain("failed applying patch file"); + + expect(await readdirSorted(tmpDir)).toEqual([".keep"]); + expect(exitCode).not.toBe(0); +}); + +test.concurrent("re-extracting replaces an invalid cache entry", async () => { + using server = makeRegistry({ "heal-pkg": makePackageTarball("heal-pkg", 3) }); + + using dir = tempDir("tempdir-leak-heal", { + "proj/package.json": JSON.stringify({ + name: "proj", + version: "1.0.0", + dependencies: { "heal-pkg": "1.0.0" }, + }), + "proj/bunfig.toml": `[install]\nregistry = "${server.url}"\n`, + "tmp/.keep": "", + "cache/.keep": "", + }); + const tmpDir = join(String(dir), "tmp"); + const cacheDir = join(String(dir), "cache"); + const proj = join(String(dir), "proj"); + + // Save a lockfile: with one, the reinstall below skips re-resolution and + // relies on the package.json-exists cache check that triggers re-extract. + const first = await runInstall(proj, cacheDir, tmpDir, []); + expect(first.exitCode).toBe(0); + + const [cacheEntry] = (await readdirSorted(cacheDir)).filter(name => name.startsWith("heal-pkg@")); + expect(cacheEntry).toBeDefined(); + + // A cache entry without package.json is invalid; a reinstall whose + // node_modules copy also fails verification must replace it, not keep it. + await Promise.all([ + rm(join(cacheDir, cacheEntry, "package.json")), + rm(join(proj, "node_modules", "heal-pkg", "package.json")), + ]); + + const second = await runInstall(proj, cacheDir, tmpDir, []); + expect(await readdirSorted(join(cacheDir, cacheEntry))).toContain("package.json"); + expect(await readdirSorted(join(proj, "node_modules", "heal-pkg"))).toContain("package.json"); + expect(await readdirSorted(tmpDir)).toEqual([".keep"]); + expect(second.exitCode).toBe(0); +});