drop unused libarchive read formats and filters - #39484
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 49 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 74 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
@robobun adopt |
|
Adopted and verified: debug build of this branch links with only the tar/gnutar/gzip readers left, and the streaming extract, tarball integrity, pack, add and Bun.Archive suites pass (removing the new gzip registration makes the streaming test fail, so the ordering is covered). CI was green (https://buildkite.com/bun/bun/builds/100323); against this branch's own merge-base the stripped binaries shrink by 258 to 320 KB per platform, details in the PR description. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it patches vendored libarchive semantics and prunes the compiled source list for the bun install extraction path, a human look would still be worthwhile.
What was reviewed:
- Confirmed
TarballStreamis the only caller ofarchive_read_set_format/archive_read_append_filter, and it now registers gzip/tar before selecting them; other libarchive read users (Archive.rs,pack_command.rs,libarchive/lib.rs) already register tar/gnutar/gzip explicitly and don't use the patched entry points. - Traced the patched
archive_read_set_format.cslot-lookup loop —a->formatis set to&a->formats[0]before the name check, so removing the auto-register call cannot introduce a null deref; an unregistered format cleanly returnsARCHIVE_FATAL. - Checked the removed sources (ppmd8, blake2, filter/format
_all/_by_code, etc.) against the remaining files' references; the kept-but-unused entries (format_empty/format_raw/ppmd7/filter_program) match what the surviving objects still name.
Extended reasoning...
Overview
This PR shrinks the linked libarchive surface by (a) adding patches/libarchive/select-registered-only.patch, which strips the auto-registration calls out of archive_read_set_format and archive_read_append_filter so they only pick among formats/filters the caller already registered, (b) dropping ~20 read-format/filter source files plus ppmd8/blake2*_ref from the DirectBuild source list in scripts/build/deps/libarchive.ts, and (c) adding an explicit read_support_filter_gzip() call in TarballStream::open_archive before archive_read_append_filter(GZIP) to satisfy the new select-only semantics. The Rust change is one functional line plus a consolidated comment.
Security risks
Archive extraction during bun install is security-relevant, but this change removes attack surface (unused parsers for rar/7zip/cab/iso/xar/zip/etc. are no longer compiled in) rather than adding any. The patched C functions do not touch untrusted input — they operate on caller-supplied integer codes. No new parsing or filesystem behavior is introduced.
Level of scrutiny
Medium-high. This is a semantic patch to vendored C plus a hand-curated compile list, and it sits directly under the bun install streaming-extract hot path. Per the repo guidance, dependency/vendoring changes warrant a maintainer look. The failure mode of a mistake here is either a link error (loud, caught by CI) or the streaming extractor failing to open archives for tarballs above the 2 MiB streaming threshold (should be caught by the install tarball tests the author ran, but only if those tests exceed the threshold).
Other factors
I cross-checked the patch against the upstream archive_read_set_format.c/archive_read_append_filter.c at the pinned commit: the slot/bidder lookup loops are unchanged and safely handle the "not registered" case (they return ARCHIVE_FATAL with the new error string, no null deref). archive_read_append_filter_program_signature still references archive_read_support_filter_program, which is intentionally kept in SOURCES. The removed blake2/ppmd8 objects were only reachable from the rar5 reader, which is also removed. No new automated test is added — coverage relies on the existing streaming-extract and install-tarball suites, which is reasonable for a build-config change but is another reason a human should confirm CI is green across all targets before merge.
|
Updated 8:42 PM PT - Aug 17th, 2026
✅ @alii, your commit 53d522480ab6be095241fb70f16f344e01ce25f0 passed in 🧪 To try this PR locally: bunx bun-pr 39484That installs a local version of the PR into your bun-39484 --bun |
Problem
archive_read_support_format_tar/_gnutar) and the gzip read filter (TarballStream.rs,runtime/api/Archive.rs,cli/pack_command.rs,libarchive/lib.rs), but every libarchive reader and read filter still ends up in the binary.TarballStream::open_archive(src/install/TarballStream.rs), which callsarchive_read_set_formatandarchive_read_append_filterto skip bidding. Upstream, those two functions register the requested format/filter themselves, througharchive_read_support_format_by_code()and a switch over everyarchive_read_support_filter_*(), so one reference from bun keeps the 7zip, rar, zip, iso9660, xz, zstd, ... readers alive through--gc-sections.Fix
patches/libarchive/select-registered-only.patch:archive_read_set_formatandarchive_read_append_filterno longer register anything; they only select among the formats/filters the caller already registered, and fail with "Format is not registered" / "Filter is not registered" otherwise. The slot lookup loops are upstream's, unchanged.scripts/build/deps/libarchive.ts: drop the reader and read filter sources bun never registers, plusarchive_ppmd8and the blake2 reference implementations that only the zip and rar5 readers used.format_empty,format_raw,ppmd7andfilter_programstay becausearchive_match.c,archive_write_set_format_7zip.candarchive_read_append_filter.cstill reference them by name.TarballStream::open_archivenow registers gzip itself beforearchive_read_append_filter(it already registered tar beforearchive_read_set_format). The comment there explains the ordering constraint: tar has to be registered beforeread_set_options, andset_formathas to come after it, becausearchive_set_format_option()writes NULL toa->formatwhen it is done dispatching.nmon the binary shows only the tar/gnutar/gzip registration functions plus the three kept-by-reference stubs;test/cli/install/bun-install-streaming-extract.test.ts(the only path that uses the patched functions; with theread_support_filter_gzip()line removed, its streaming case fails with "Fail extracting tarball", so the existing suite covers the new ordering requirement),bun-install-tarball-integrity.test.ts,bun-pack.test.ts,bun-add.test.ts(includes an uncompressed.tar), andtest/js/bun/archive.test.tsall pass.Background
archive_read_support_*(), and on open libarchive "bids": each registered candidate inspects the first bytes and the best match wins.bun installextractor cannot provide before the first HTTP chunk arrives, soTarballStreamusesarchive_read_append_filter(GZIP)andarchive_read_set_format(TAR)to pin the chain up front instead. Those are the only two libarchive entry points bun uses that pick a reader by code, and they are what this patch changes.scripts/build/deps/libarchive.tslists the source files directly), so removing a file fromSOURCESremoves it from the link; the patch is what makes that possible without undefined references.