install: fail extraction when libarchive discards a damaged tar header

[robobun]

Problem

• bun install of a tarball with one corrupt byte in a member's tar header exits 0. The installed package lacks that member.
• libarchive discards a header block with a bad checksum and returns ARCHIVE_RETRY. Both extractors took that as "call again": src/libarchive/lib.rs:1441 (buffered) and src/install/TarballStream.rs:545 (streaming).

Fix

• The buffered loops read from memory, where ARCHIVE_RETRY has only this meaning. They now fail like ARCHIVE_FATAL. bun create, Bun.Archive.extract(), bun publish <tgz> and bun pm diff share them.
• The streaming read callback also yields with ARCHIVE_RETRY. So that extractor compares the headers it received with archive_file_count() at each header and at EOF. libarchive counts a discarded block as a started header, so a gap means a dropped member.
• The member is lost either way. file:, git, GitHub and URL tarballs have no integrity value on the first install, so extraction is the only check, and bun already fails on every other corruption. bsdtar and GNU tar also exit non-zero (npm warns, see Notes).
• Verified: test/cli/install/bun-install-streaming-extract.test.ts (6 new cases, 5 fail on the released bun), test/js/bun/archive.test.ts, test/cli/install/bun-publish.test.ts.

Background

• A tar member starts with a 512-byte header block that carries a checksum of itself. On a mismatch, upstream libarchive consumes the block and returns ARCHIVE_RETRY, and the caller decides.
• patches/libarchive/nonblocking-read.patch lets the streaming read callback return ARCHIVE_RETRY when no bytes are buffered yet. Such a yield and its resume increment archive_file_count() once (read_header_in_progress in archive_read.c).
• Two tests asserted the skip (libarchive: keep upstream damaged-block retry semantics on the buffered path #29430: state reset across the retry, Robustness and input-handling pass across install, shell, TLS/QUIC, HTTP/3, SQL and crypto #37669: input buffer alive across it). Failure tests replace them and still drive libarchive past the block while the body arrives.

Notes

Source: fuzz ledger entry 16545 (14 of 166 mutation cells installed silently, on bun 1.3.14 and 1.4.0). GNU tar on the same bytes prints "Skipping to next header" and exits 2. The other header corruptions in the ledger hit package.json: that member was dropped too, and the install then failed because the extracted package had no package.json.

Repro used for this report: bun pm pack a package with bin.js and index.js, gunzip, flip one byte at offset 104, 134, or 150 of the package/bin.js header, gzip, install as file:. On bun 1.4.0: exit 0, node_modules/pk holds index.js and package.json only. bun publish pk.tgz listed two members and went on to the auth step. npm 11 installs it and prints npm warn tar TAR_ENTRY_INVALID checksum failure four times. The extractor has no warning channel, so warn-and-continue would be a new mechanism for one corruption out of many that already fail.

Why the streaming path does not fail on the Retry itself: when the read callback runs dry, the gzip filter returns the output it already has, and the tar reader can hit the damaged block in that same call. So neither the callback state (archive_holds_reading) nor the error string (a pax WARN from earlier in the same member can still be set at a yield) tells the two cases apart. archive_file_count() does, one header later: upstream libarchive increments it for every header it starts, a discarded block included, and the patch keeps a yield plus its resume at one increment. The data blocks of the dropped member are scanned as headers in the meantime, as before this change; the result is the same failure.

False positive probe: tarballs made by GNU tar (--format=pax, pax with a global header, --format=gnu, default), npm pack, and bun pm pack, each with pax and long-name members, streamed in 7-, 512-, and 4096-byte chunks with BUN_INSTALL_STREAMING_DRAIN_THRESHOLD=1, all install. The pax global header variant also passed with 1-byte chunks. The committed 64-byte-chunk test keeps a small version of this.

Left as is: Bun.Archive's own files(), count, and glob loops stop at any status other than OK/WARN, FATAL included, and report what they have. That predates this change. readTarball in bun:internal-for-testing reads tarballs bun wrote itself. The temp directory a failed extraction leaves behind is the subject of #36542.

Suites run on the debug build: the three files above in full, plus test/cli/install/symlink-path-traversal.test.ts, GHSA-pfwx-36v6-832x.test.ts, bun-pm-diff.test.ts, bun-pack.test.ts. cargo clippy on bun_libarchive and bun_install is clean.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-publish.test.ts test/js/bun/archive.test.ts

[coderabbitai]

Warning

Review limit reached

Your included review limit has been reached.

You’re in a promotional period — use the checkbox below to run this review for free:

• Run review for free

_{On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file.}

How can I continue?

Run this review now using the option above, or comment @coderabbitai review --use-credits.

You can also wait for the limit to reset (next review available in 3 minutes), then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c68ebf4e-fca1-470f-bbf6-a98985eeeacd

📥 Commits

Reviewing files that changed from the base of the PR and between 34cbb9a and e694fd0.

📒 Files selected for processing (5)

• src/install/TarballStream.rs
• src/libarchive/lib.rs
• test/cli/install/bun-install-streaming-extract.test.ts
• test/cli/install/bun-publish.test.ts
• test/js/bun/archive.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: ready for review.

Reproduced on bun 1.4.0 with a bun pm pack tarball in which one byte of the package/bin.js header was flipped, installed as a file: dependency. The install exited 0 and bin.js was missing. The same tarball now fails with "Fail extracting tarball". The streaming extractor gets the same treatment through a local drip-feeding registry (see the tests in test/cli/install/bun-install-streaming-extract.test.ts).

Running a self-review of the diff next. CI results will show up below.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes package-extraction behavior (corrupted tarballs now fail instead of silently dropping members), replaces two existing regression tests, and the streaming-path fix depends on the archive_file_count() / read_header_in_progress interaction in the Bun-specific libarchive patch, a maintainer look would still be worthwhile.

What was reviewed:

• Verified in patches/libarchive/nonblocking-read.patch that file_count is incremented inside the !read_header_in_progress guard and that the damaged-block RETRY clears the flag, so the header-count check in TarballStream distinguishes yields from discarded blocks as described.
• Checked the three buffered header loops now treating Retry as fatal all use read_open_memory (blocking source), so the only Retry there is the discarded-block case.
• Confirmed the two deleted tests' underlying invariants (state reset across retry, input buffer alive across it) are still driven by the new streaming failure tests, and the extension-header test guards against false positives from pax 'g'/'x' and GNU 'L' headers.

Extended reasoning...

Overview

This PR changes how bun install, Bun.Archive.extract(), bun publish <tgz>, and bun pm diff handle a tarball whose member header has a bad checksum. libarchive discards such a block and returns ARCHIVE_RETRY; both extractors previously treated that as "call again", so the install succeeded with the damaged member silently missing. The fix makes the buffered header loops (src/libarchive/lib.rs) treat Retry as Fatal, and adds a header-count comparison in the streaming extractor (src/install/TarballStream.rs) since its read callback also uses Retry for out-of-input yields. Six new test cases cover both paths plus a false-positive guard; two existing tests that asserted the old skip behavior are replaced.

Security risks

This is security-adjacent: package extraction integrity. The change is strictly defensive — it turns a silent drop into a hard failure, matching bsdtar/GNU tar. No new attack surface. The main risk would be a false positive (rejecting a valid tarball), which the extension-header test and the PR's out-of-tree probe address. I traced the archive_file_count() semantics through the vendored patch and they match the PR's claim: yields count once via read_header_in_progress, discarded blocks count separately.

Level of scrutiny

High. Package installation is a critical path, the streaming fix leans on a subtle invariant of a Bun-local libarchive patch, and two existing regression tests are deleted. Per the repo's review guidelines, deleted tests need a stated reason (provided) and the replacement must keep asserting the property the original protected — the PR argues the new streaming failure tests do this by still driving libarchive past the discarded block while more body arrives, but a maintainer should confirm that judgment.

Other factors

The PR is unusually well-documented (mechanism, false-positive probe across GNU/npm/bun-packed tarballs at multiple chunk sizes, related suites re-run). No bugs surfaced from the multi-agent review. The ArchiveIterator::next() change fans out to multiple consumers (bun publish, bun pm diff, Bun.Archive glob/count loops per the PR notes), which is another reason a maintainer should sign off on the behavioral shift rather than auto-approving.

[robobun]

Two facts for the points the automated review left to a maintainer.

The replaced tests. The #37669 property (the input buffer stays alive while libarchive reads past a discarded block) is still exercised: the two streaming failure tests feed the body in 4 KiB chunks with a drain per chunk, so TarballStream still takes the Retry if archive_holds_reading => continue arm and libarchive reads on into the next chunks before the count check fails the extraction. CI runs this under ASAN. The #29430 property (header state reset after the retry) no longer has an observable effect: whatever libarchive returns after a discarded block, every caller now fails. The patch itself is unchanged.

The iterator. ArchiveIterator::next has two callers. Both already handle the Err arm that Fatal takes, and a discarded block now takes the same arm. bun publish <tgz> prints error: failed to read archive header: <libarchive message> and exits 1 (the publish test in this PR pins that line). bun pm diff prints error: <tree label>: failed to read archive header and exits 1 (pm_diff_command.rs:501). Bun.Archive's files(), count, and glob loops do not use the iterator and are not changed.