diff --git a/src/install/TarballStream.rs b/src/install/TarballStream.rs index 81357db9ad4..18012b05b02 100644 --- a/src/install/TarballStream.rs +++ b/src/install/TarballStream.rs @@ -151,6 +151,9 @@ pub struct TarballStream { bytes_received: usize, entry_count: u32, + /// Headers `read_next_header` has returned; compared against + /// `archive_file_count()` in `check_no_header_discarded`. + headers_read: c_int, fail: Option, invalid_name: bool, @@ -262,6 +265,7 @@ impl TarballStream { deferred_symlinks: Vec::new(), bytes_received: 0, entry_count: 0, + headers_read: 0, fail: None, invalid_name: false, drain_task: thread_pool::Task { @@ -542,9 +546,14 @@ impl TarballStream { Phase::WantHeader => { let mut entry: *mut lib::Entry = core::ptr::null_mut(); match archive.read_next_header(&mut entry) { + // Either an out-of-input yield from the read callback + // or a block libarchive discarded; the latter fails + // the extraction in `check_no_header_discarded` once + // the next header (or EOF) is reached. lib::Result::Retry if (*this).archive_holds_reading => continue, lib::Result::Retry => return Ok(()), lib::Result::Eof => { + Self::check_no_header_discarded(this, archive)?; #[cfg(unix)] { let dest = (*this).dest.unwrap(); @@ -557,6 +566,8 @@ impl TarballStream { return Ok(()); } lib::Result::Ok | lib::Result::Warn => { + (*this).headers_read += 1; + Self::check_no_header_discarded(this, archive)?; // libarchive returned OK/WARN with a valid entry // pointer owned by `archive`; it stays valid until // the next `read_next_header`. No other Rust @@ -605,6 +616,35 @@ impl TarballStream { } // unsafe } + /// Fail the extraction if libarchive started a header it never handed to + /// `step()`. The only way that happens is a block with a bad header + /// checksum: the tar reader discards it and reports `Retry`, the status + /// the read callback also uses for "out of input", so `step()` cannot + /// fail on the `Retry` itself. Reading on from there resyncs on the next + /// intact header and drops the damaged member from the package, which is + /// why the count is checked at every header and at EOF instead. + /// `Archiver::extract_to_dir` fails on the same tarball. + /// + /// # Safety + /// `this` must be live; raw-ptr field read only. + unsafe fn check_no_header_discarded( + this: *mut Self, + archive: &lib::Archive, + ) -> crate::Result<()> { + let started = archive.file_count(); + // SAFETY: see fn-level # Safety. + let returned = unsafe { (*this).headers_read }; + if started == returned { + return Ok(()); + } + bun_output::scoped_log!( + TarballStream, + "libarchive discarded {} damaged header block(s)", + started - returned + ); + Err(crate::Error::Fail) + } + /// # Safety /// `this` must be live and rooted at the Box allocation (i.e. the /// pointer threaded from `drain_callback` → `drain` → `step`, NOT a diff --git a/src/libarchive/lib.rs b/src/libarchive/lib.rs index 23027c114d9..b69d51a3f53 100644 --- a/src/libarchive/lib.rs +++ b/src/libarchive/lib.rs @@ -52,6 +52,14 @@ pub mod lib { pub enum Result { Eof = 1, Ok = 0, + /// From `read_next_header` on a blocking source (`read_open_memory`) + /// this has exactly one meaning: the tar reader discarded a 512-byte + /// block whose header checksum did not match and is positioned on the + /// block after it. Calling again resyncs on whatever header comes next, + /// so the member that block belonged to is silently lost. Header loops + /// therefore treat it as a corrupt archive, the same as `Fatal`. + /// `TarballStream`'s non-blocking read callback also produces it when + /// it runs out of input (see the BUN PATCHes in `vendor/libarchive`). Retry = -10, Warn = -20, Failed = -25, @@ -90,6 +98,7 @@ pub mod lib { offset: *mut la_int64_t, ) -> Result; fn archive_error_string(a: *mut Archive) -> *const c_char; + fn archive_file_count(a: *mut Archive) -> c_int; // streaming-read setup (used by TarballStream's resumable extractor) pub fn archive_read_set_format(a: *mut Archive, code: c_int) -> c_int; pub fn archive_read_append_filter(a: *mut Archive, code: c_int) -> c_int; @@ -365,6 +374,17 @@ pub mod lib { unsafe { ZStr::from_c_ptr(p) }.as_bytes() } + /// `archive_file_count`: the number of headers `read_next_header` has + /// started so far. libarchive counts a header when it starts reading + /// it, so a block it then discards (see [`Result::Retry`]) is counted + /// even though no entry is returned for it; the final call that + /// returns `Eof` is not counted. A non-blocking yield and its resume + /// count once (`archive_read.c`, `read_header_in_progress`). + pub fn file_count(&self) -> c_int { + // SAFETY: `self` is a live archive handle. + unsafe { archive_file_count(self.as_mut_ptr()) } + } + // ── write side ───────────────────────────────────────────────────── pub fn write_new() -> *mut Archive { // SAFETY: FFI call with no preconditions. @@ -723,7 +743,6 @@ pub mod lib { let mut entry: *mut Entry = core::ptr::null_mut(); loop { return match a.read_next_header(&mut entry) { - Result::Retry => continue, Result::Eof => IteratorResult::init_res(None), // `Warn` still yields a fully populated entry; see `Result::succeeded`. Result::Ok | Result::Warn => { @@ -1292,8 +1311,7 @@ impl Archiver { match r { lib::Result::Eof => break 'loop_, - lib::Result::Retry => continue 'loop_, - lib::Result::Failed | lib::Result::Fatal => { + lib::Result::Retry | lib::Result::Failed | lib::Result::Fatal => { return Err(crate::Error::Fail); } _ => { @@ -1438,8 +1456,17 @@ impl Archiver { match r { lib::Result::Eof => break 'loop_, - lib::Result::Retry => continue 'loop_, - lib::Result::Failed | lib::Result::Fatal => { + lib::Result::Retry | lib::Result::Failed | lib::Result::Fatal => { + if options.log { + // SAFETY: `archive` is the live `read_new()` handle this + // extraction loop is iterating. + let archive_error = slice_to_nul(unsafe { &*archive }.error_string()); + Output::err( + "libarchive error", + "reading next header: {}", + (bstr::BStr::new(archive_error),), + ); + } return Err(crate::Error::Fail); } _ => { diff --git a/test/cli/install/bun-install-streaming-extract.test.ts b/test/cli/install/bun-install-streaming-extract.test.ts index 8e77b744d45..e35a1be701f 100644 --- a/test/cli/install/bun-install-streaming-extract.test.ts +++ b/test/cli/install/bun-install-streaming-extract.test.ts @@ -26,7 +26,7 @@ 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" | "x" | "g"): Buffer { +function tarHeader(name: string, size: number, type: "0" | "5" | "x" | "g" | "L"): Buffer { const buf = Buffer.alloc(512, 0); buf.write(name, 0, 100, "utf8"); buf.write(octal(0o644, 8), 100); // mode @@ -568,73 +568,6 @@ describe("streaming tarball extraction", () => { }); }); -// ------------------------------------------------------------------- -// Regression: the nonblocking-read patch routed upstream libarchive's -// pre-existing damaged-block ARCHIVE_RETRY through the same `bun_retry` -// path as a non-blocking yield, so `seen_headers` / entry state leaked -// across the retry. A second pax 'g' global header after the damaged -// block would then trip "Redundant 'g' header" → ARCHIVE_FATAL even -// though upstream libarchive (and a `tar` CLI) accepts this layout. -// -// This test goes through the buffered extractor only: local `file:` -// tarballs are read fully into memory by PackageManagerTask.readAndExtract -// and handed to Archiver.extractToDir, which loops on readNextHeader with -// `.retry => continue`. The streaming reader is never involved, so any -// behaviour change here is the libarchive patch leaking into the shared -// buffered codepath. -// ------------------------------------------------------------------- -test("buffered extract: damaged-block retry resets header state (upstream semantics)", async () => { - // One pax 'g' extended-header payload. libarchive's header_pax_global - // just skips it, but parsing it sets `seen_headers |= seen_g_header`; - // seeing a second one without an intervening state reset is what - // triggers the "Redundant 'g' header" FATAL. - const pax = Buffer.from("16 comment=test\n", "utf8"); - expect(pax.length).toBe(16); - const paxEntry = () => [tarHeader("pax_global_header", pax.length, "g"), pax, pad512(pax.length)]; - - // A 512-byte block that is neither all-zero (would be treated as the - // end-of-archive marker) nor has a valid checksum: upstream tar emits - // "Damaged tar archive (bad header checksum)" and returns - // ARCHIVE_RETRY, which the Zig extract loop handles as `continue`. - const damaged = Buffer.alloc(512, 0); - damaged.write("junk", 0, "utf8"); - damaged.fill(" ", 148, 156); // checksum field left as spaces → guaranteed mismatch - - const fileBody = Buffer.from("damaged-block-retry ok\n", "utf8"); - const file = [tarHeader("package/index.js", fileBody.length, "0"), fileBody, pad512(fileBody.length)]; - - const pkgJson = Buffer.from(JSON.stringify({ name: "damaged-pkg", version: "1.0.0", main: "index.js" }) + "\n"); - const pkgJsonEntry = [tarHeader("package/package.json", pkgJson.length, "0"), pkgJson, pad512(pkgJson.length)]; - - // [g][damaged][g][package.json][index.js][EOF EOF] - const tar = Buffer.concat([...paxEntry(), damaged, ...paxEntry(), ...pkgJsonEntry, ...file, Buffer.alloc(1024, 0)]); - const tgz = gzipSync(tar); - - using dir = tempDir("damaged-block-retry", { - "package.json": JSON.stringify({ - name: "app", - version: "1.0.0", - dependencies: { "damaged-pkg": "file:./damaged-pkg.tgz" }, - }), - }); - writeFileSync(join(String(dir), "damaged-pkg.tgz"), tgz); - - const { stderr, exitCode } = await runInstall(String(dir)); - - // With the broken patch the second 'g' header trips - // "Redundant 'g' header" → ARCHIVE_FATAL inside libarchive; the Zig - // extract loop surfaces that as `error.Fail` → "Fail extracting - // tarball". With upstream semantics restored the damaged block is - // skipped, state is fully reset, and the file following the second - // 'g' header is extracted normally. - expect(stderr).not.toContain("Fail extracting tarball"); - expect(stderr).not.toContain("failed to resolve"); - expect(exitCode).toBe(0); - - const extracted = readFileSync(join(String(dir), "node_modules", "damaged-pkg", "index.js")); - expect(extracted.equals(fileBody)).toBe(true); -}); - // ------------------------------------------------------------------- // Buffered extract: the decompressed tar is never materialised in // memory. libarchive gunzips on the fly, so a highly compressible .tgz @@ -714,62 +647,170 @@ test("buffered extract does not hold the decompressed local tarball in memory", expect(maxRssBytes).toBeLessThan(2 * PAYLOAD_SIZE); }); -test("streaming extract skips a damaged header block and extracts the entries after it byte-for-byte while more data is still arriving", async () => { - const pkgJson = Buffer.from(JSON.stringify({ name: "stream-pkg", version: "1.0.0" }) + "\n"); - const before = Buffer.alloc(4 * 1024 * 1024, 0); - const after = Buffer.alloc(16 * 1024 * 1024, 0); - const tail = Buffer.alloc(8 * 1024 * 1024); - let seed = createHash("sha512").update("tail.bin").digest(); - for (let off = 0; off < tail.length; off += seed.length) { - seed.copy(tail, off); - seed = createHash("sha512").update(seed).digest(); +// ------------------------------------------------------------------- +// libarchive discards a header block whose checksum does not match, +// reports ARCHIVE_RETRY, and resyncs on the next intact header. Both +// extractors used to take that status as "call again", so one flipped +// byte in a member's header installed the package without that member +// and exited 0. A damaged header has to fail the install like every +// other corruption of the tarball does. +// +// The two tests that used to sit here asserted the skip: one pinned the +// libarchive patch's state reset across the retry (#29430), the other +// the streaming reader keeping its input buffer alive across it +// (#37669). The streaming cases below still drive libarchive past the +// discarded block while the rest of the body is arriving; the outcome +// they assert is the failure. +// ------------------------------------------------------------------- +describe.concurrent("a member whose tar header is damaged fails the install", () => { + // Incompressible bytes, so the .tgz arrives in many chunks. + function noise(label: string, size: number): Buffer { + const out = Buffer.alloc(size); + let seed = createHash("sha512").update(label).digest(); + for (let off = 0; off < size; off += seed.length) { + seed.copy(out, off, 0, Math.min(seed.length, size - off)); + seed = createHash("sha512").update(seed).digest(); + } + return out; } - const damaged = Buffer.alloc(512, 0); - damaged.write("junk", 0, "utf8"); - damaged.fill(" ", 148, 156); + // A complete member (header, body, padding) with bit 0 of one header + // byte flipped. Every offset used below is an octal digit of a numeric + // field, so the header stays well-formed and only its checksum is wrong. + function damagedFile(name: string, body: Buffer, headerOffset: number): Buffer[] { + const header = tarHeader(name, body.length, "0"); + expect(String.fromCharCode(header[headerOffset])).toMatch(/^[0-7]$/); + header[headerOffset] ^= 0x01; + return [header, body, pad512(body.length)]; + } - const damagedEntries: Entry[] = [ - { path: "package.json", body: pkgJson }, - { path: "before.bin", body: before }, - { path: "after.bin", body: after }, - { path: "tail.bin", body: tail }, - ]; - const tar = Buffer.concat([ - ...tarFile("package/package.json", pkgJson), - ...tarFile("package/before.bin", before), - damaged, - ...tarFile("package/after.bin", after), - ...tarFile("package/tail.bin", tail), - Buffer.alloc(1024, 0), - ]); - const damagedTgz = gzipSync(tar); - const damagedShasum = createHash("sha1").update(damagedTgz).digest("hex"); - const damagedIntegrity = "sha512-" + createHash("sha512").update(damagedTgz).digest("base64"); - expect(damagedTgz.length).toBeGreaterThan(2 * 1024 * 1024); - - await using reg = await makeRegistry(damagedTgz, damagedShasum, damagedIntegrity, 4096); - const registry = reg.url; - - using dir = tempDir("streaming-extract-damaged-block", { - "package.json": JSON.stringify({ - name: "app", - version: "1.0.0", - dependencies: { "stream-pkg": "1.0.0" }, - }), - "bunfig.toml": Bun.TOML.stringify({ install: { registry } }), + // `file:` tarballs always take the buffered extractor (Archiver.extractToDir) + // and have no integrity to fall back on: extraction is the only check. + test.each([ + ["mode", 104], + ["size", 130], + ["checksum", 150], + ] as const)("buffered: %s field of bin.js corrupted", async (_field, offset) => { + const pkgJson = Buffer.from(JSON.stringify({ name: "damaged-pkg", version: "1.0.0", bin: "bin.js" }) + "\n"); + const tar = Buffer.concat([ + ...tarFile("package/package.json", pkgJson), + ...damagedFile("package/bin.js", Buffer.from("#!/usr/bin/env node\nconsole.log('bin');\n"), offset), + ...tarFile("package/index.js", Buffer.from("module.exports = 'index';\n")), + Buffer.alloc(1024, 0), + ]); + + using dir = tempDir("damaged-header-buffered", { + "package.json": JSON.stringify({ + name: "app", + version: "1.0.0", + dependencies: { "damaged-pkg": "file:./damaged-pkg.tgz" }, + }), + "damaged-pkg.tgz": gzipSync(tar), + }); + + const { stderr, exitCode } = await runInstall(String(dir)); + // The first line is `--verbose` output from the extractor, the second + // is the regular install error. + expect(stderr).toContain("libarchive error: reading next header: Damaged tar archive (bad header checksum)"); + expect(stderr).toContain("error: Fail extracting tarball from damaged-pkg"); + expect(existsSync(join(String(dir), "node_modules", "damaged-pkg"))).toBe(false); + expect(exitCode).toBe(1); }); - const { stderr, exitCode } = await runInstall(String(dir)); - expect(stderr).not.toContain("extracting tarball for"); - expect(stderr).not.toContain("error:"); - expect(stderr).toContain("Streamed "); - expect(reg.tarballHits).toBe(1); + const streamPkgJson = Buffer.from(JSON.stringify({ name: "stream-pkg", version: "1.0.0" }) + "\n"); + + function appDir(label: string) { + return tempDir(label, { + "package.json": JSON.stringify({ name: "app", version: "1.0.0", dependencies: { "stream-pkg": "1.0.0" } }), + }); + } - const pkgRoot = join(String(dir), "node_modules", "stream-pkg"); - for (const { path, body } of damagedEntries) { - const got = readFileSync(join(pkgRoot, path)); - expect([path, got.length, got.equals(body)]).toEqual([path, body.length, true]); + // Serves `tar` (gzipped, advertised with its own integrity so only the + // archive contents can fail the install) in `chunk`-byte pieces, and + // installs it through the streaming extractor: the threshold is lowered + // so this small tarball streams, and a drain runs after every chunk so + // libarchive keeps running out of input in the middle of headers. + async function installStreamed(dir: string, tar: Buffer, chunk: number) { + const tgz = gzipSync(tar); + expect(tgz.length).toBeGreaterThan(16 * chunk); + const shasum = createHash("sha1").update(tgz).digest("hex"); + const integrity = "sha512-" + createHash("sha512").update(tgz).digest("base64"); + await using reg = await makeRegistry(tgz, shasum, integrity, chunk); + writeFileSync(join(dir, "bunfig.toml"), Bun.TOML.stringify({ install: { registry: reg.url } })); + const { stderr, exitCode } = await runInstall(dir, { + BUN_INSTALL_STREAMING_MIN_SIZE: String(chunk), + BUN_INSTALL_STREAMING_DRAIN_THRESHOLD: String(chunk), + }); + return { stderr, exitCode, tarballHits: reg.tarballHits }; } - expect(exitCode).toBe(0); + + // The streaming extractor (TarballStream) also gets ARCHIVE_RETRY when + // its read callback runs out of input, so it notices the discarded block + // at the next header libarchive does return ("middle") or at the end of + // the archive when the damaged member was the last one ("last"). + test.each([ + ["middle", true], + ["last", false], + ] as const)("streaming: damaged member in the %s of the archive", async (_position, membersFollow) => { + const tar = Buffer.concat([ + ...tarFile("package/package.json", streamPkgJson), + ...tarFile("package/before.bin", noise("before.bin", 64 * 1024)), + ...damagedFile("package/damaged.bin", noise("damaged.bin", 64 * 1024), 104), + ...(membersFollow ? tarFile("package/after.bin", noise("after.bin", 64 * 1024)) : []), + Buffer.alloc(1024, 0), + ]); + + using dir = appDir("damaged-header-streaming"); + const { stderr, exitCode, tarballHits } = await installStreamed(String(dir), tar, 4096); + // This wording is TarballStream's; the buffered path says "from". + expect(stderr).toContain('error: Fail extracting tarball for "stream-pkg"'); + expect(tarballHits).toBe(1); + expect(existsSync(join(String(dir), "node_modules", "stream-pkg"))).toBe(false); + expect(exitCode).toBe(1); + }); + + // The streaming check counts the headers libarchive starts. Extension + // headers (pax 'g' as at the top of every GitHub tarball, pax 'x', GNU + // 'L') are read as part of the member that follows them, also when the + // body runs dry between them, so an intact archive full of them must + // still install. + test("streaming: intact members behind pax global, pax and GNU long-name headers still install", async () => { + const global = Buffer.from("16 comment=test\n"); + // Both names are longer than the 100 bytes a ustar header holds. + const paxName = "package/" + Buffer.alloc(96, "p").toString() + "/pax.bin"; + const gnuName = "package/" + Buffer.alloc(96, "g").toString() + "/gnu.bin"; + const gnuNameZ = Buffer.from(gnuName + "\0"); + const entries: Entry[] = [ + { path: "package.json", body: streamPkgJson }, + { path: "plain.bin", body: noise("plain.bin", 1536) }, + { path: paxName.slice("package/".length), body: noise("pax.bin", 1536) }, + { path: gnuName.slice("package/".length), body: noise("gnu.bin", 1536) }, + ]; + const gnuBody = entries[3].body; + const tar = Buffer.concat([ + tarHeader("pax_global_header", global.length, "g"), + global, + pad512(global.length), + ...tarFile("package/package.json", entries[0].body), + ...tarFile("package/plain.bin", entries[1].body), + ...tarFile(paxName, entries[2].body), + tarHeader("././@LongLink", gnuNameZ.length, "L"), + gnuNameZ, + pad512(gnuNameZ.length), + tarHeader(gnuName.slice(0, 99), gnuBody.length, "0"), + gnuBody, + pad512(gnuBody.length), + Buffer.alloc(1024, 0), + ]); + + using dir = appDir("extension-headers-streaming"); + const { stderr, exitCode } = await installStreamed(String(dir), tar, 64); + expect(stderr).not.toContain("error:"); + expect(stderr).toContain("Streamed "); + const pkgRoot = join(String(dir), "node_modules", "stream-pkg"); + for (const { path, body } of entries) { + expect([path, readFileSync(join(pkgRoot, path)).equals(body)]).toEqual([path, true]); + } + expect(exitCode).toBe(0); + }); }); diff --git a/test/cli/install/bun-publish.test.ts b/test/cli/install/bun-publish.test.ts index c701b322806..e4094dca716 100644 --- a/test/cli/install/bun-publish.test.ts +++ b/test/cli/install/bun-publish.test.ts @@ -14,6 +14,7 @@ import { tmpdirSync, } from "harness"; import { delimiter, join } from "path"; +import { gunzipSync, gzipSync } from "zlib"; const registry = new VerdaccioRegistry(); @@ -551,6 +552,28 @@ test("can publish from a tarball", async () => { await runBunInstall(env, packageDir, { savesLockfile: false }); expect(await file(join(packageDir, "node_modules", "publish-pkg-2", "package.json")).json()).toEqual(json); }); +test("refuses to publish a tarball in which a member's header is damaged", async () => { + // libarchive discards the damaged block and resyncs on the next header; + // `bun publish` used to list the tarball without that member and carry on. + using dir = tempDir("publish-damaged-header", { + "package.json": JSON.stringify({ name: "damaged-header-pkg", version: "1.0.0" }), + "index.js": "module.exports = 1;\n", + }); + await pack(String(dir), env); + const tgz = join(String(dir), "damaged-header-pkg-1.0.0.tgz"); + const tar = gunzipSync(readFileSync(tgz)); + // A header block starts with the name field, so this is index.js's header. + const header = tar.indexOf("package/index.js\0"); + expect(header).toBeGreaterThan(0); + expect(String.fromCharCode(tar[header + 104])).toMatch(/^[0-7]$/); + tar[header + 104] ^= 0x01; // one digit of the mode field: the stored checksum no longer matches + writeFileSync(tgz, gzipSync(tar)); + + const { out, err, exitCode } = await publish(env, String(dir), tgz, "--registry", "http://127.0.0.1:1/"); + expect(err).toBe("error: failed to read archive header: Damaged tar archive (bad header checksum)\n"); + expect(out).not.toContain("Total files"); + expect(exitCode).toBe(1); +}); test("can publish scoped packages", async () => { const { packageDir, packageJson } = await registry.createTestDir(); const bunfig = await registry.authBunfig("scoped-pkg"); diff --git a/test/js/bun/archive.test.ts b/test/js/bun/archive.test.ts index fcbdc1a2204..ad681fdb5c1 100644 --- a/test/js/bun/archive.test.ts +++ b/test/js/bun/archive.test.ts @@ -610,6 +610,27 @@ describe("Bun.Archive", () => { }).toThrow(); }); + test("rejects when a member's header checksum does not match instead of skipping the member", async () => { + // libarchive discards the damaged block and would resync on last.txt; + // extraction used to report success with broken.txt silently missing. + const broken = ustarEntry("broken.txt", Buffer.from("lost")); + broken[104] ^= 0x01; // one digit of the mode field + const tarball = new Uint8Array( + Buffer.concat([ + ustarEntry("first.txt", Buffer.from("first")), + broken, + ustarEntry("last.txt", Buffer.from("last")), + Buffer.alloc(1024), + ]), + ); + const archive = new Bun.Archive(tarball); + + using dir = tempDir("archive-damaged-header", {}); + + await expect(archive.extract(String(dir))).rejects.toThrow("ReadError"); + expect(readdirSync(String(dir))).toEqual(["first.txt"]); + }); + test("throws when extracting random bytes as archive", async () => { // Generate random bytes const randomBytes = new Uint8Array(1024);