pack: store tarball entry names byte for byte - #38715
Conversation
add_archive_entry built the member pathname by formatting the filename through bstr's Display, which replaces every byte sequence that is not valid UTF-8 with U+FFFD, while the length handed to ZStr::from_buf was computed from the original filename. On Linux, where such filenames are legal, two files whose names differed only in such a byte were written to the tarball under the same substituted name, so one of them was lost on extract; debug builds hit the from_buf NUL assertion instead. Append the raw bytes and take the NUL-terminated buffer as written. The gzip compression level a few lines up went through the same pattern and gets the same treatment.
|
Status: reproduced on bun 1.4.0-canary.1 (two files CI: every lane that has run is green (builds 96741 and 97120, 177 jobs each; the remaining entries in the annotations are retried flakes in unrelated tests). The only lane still outstanding is darwin 14 aarch64, which is queued behind a long backlog on that agent pool; in build 96741 its one failure was |
|
Updated 12:51 AM PT - Aug 15th, 2026
✅ @robobun, your commit d6441241b86dc6c6ec4ef88c740303a8aed75fe3 passed in 🧪 To try this PR locally: bunx bun-pr 38715That installs a local version of the PR into your bun-38715 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe pack command now builds NUL-terminated values from raw bytes. Linux tests parse tarballs and verify that invalid UTF-8 filename bytes remain unchanged. ChangesArchive byte preservation
Possibly related PRs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change preserves non-UTF-8 filenames byte for byte in packed tarballs, preventing filename collisions and debug-build failures. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — replaces lossy BStr Display formatting with direct byte appends so tarball entry names carry the on-disk bytes.
Checked that print_buf is empty on entry to both changed sites (fresh at 2466, cleared at 2507/3209), that ZStr::from_slice_with_nul matches the pushed trailing NUL, and that filename.as_bytes() excludes the NUL. Grepped the file for other write!-via-BStr → ZStr::from_buf sites — none remain; the other from_buf calls are byte-slice arithmetic, not formatted writes. The Windows set_pathname_utf8 path is unchanged for valid UTF-8 (BStr Display was already identity there). The new Linux-only test reads raw ustar/pax bytes to sidestep readTarball's own lossy decode, and asserts both files land under distinct byte-exact names.
Extended reasoning...
Overview
Two-site change in src/runtime/cli/pack_command.rs: the tarball entry pathname and the gzip compression-level string are now built by extend_from_slice + push(0) + ZStr::from_slice_with_nul instead of write!(buf, "{}\x00", BStr(..)) + ZStr::from_buf(buf, computed_len). The old path was lossy (BStr's Display substitutes U+FFFD for invalid UTF-8, three bytes per one) and the length passed to from_buf was computed from the original bytes, so debug builds asserted and release builds stored the substituted name — colliding distinct non-UTF-8 filenames into one member. A Linux-only test in bun-pack.test.ts gunzips the archive and reads the ustar name field and pax path record byte-for-byte via latin1 decoding.
Security risks
None. This narrows behavior toward byte fidelity for a name that was already being written to the archive; it does not change what files are selected for packing, and libarchive already handles the pax hdrcharset=BINARY emission for non-UTF-8 names.
Level of scrutiny
Low-to-medium. The Rust change is ~10 lines net, mechanical, and strictly less transformation than before. I verified the shared-buffer invariant (print_buf is empty on entry at both sites — declared at 2466, cleared at 2507 and 3209), confirmed from_slice_with_nul's contract in bun_core/util.rs, and checked that the remaining ZStr::from_buf calls in this file don't share the lossy-format pattern. For valid UTF-8 (the overwhelming common case, and the only case on Windows where set_pathname_utf8 is used), BStr Display was already byte-identical, so this is a no-op there.
Other factors
The test is well-constructed: it can't use readTarball (which would map both correct and buggy output onto U+FFFD), so it hand-parses 512-byte ustar blocks and pax records via latin1 to preserve one-char-per-byte, and asserts a Set per member so the ustar and pax spellings must agree. The skipIf(!isLinux) gate is justified in a comment (APFS rejects such names; Windows names are UTF-16). The PR description shows before/after tar tvzf output demonstrating the fix, and reports the rest of the 77-test file still passes.
There was a problem hiding this comment.
LGTM — swaps lossy write!(BStr(...)) formatting for direct byte appends into print_buf, so libarchive receives the directory entry's exact bytes.
What was reviewed:
- Confirmed
print_bufis empty at both changed sites (freshVecbefore the compression-level use;.clear()at 2507 and 3209 bracket everyadd_archive_entrycall), sofrom_slice_with_nulsees only the just-written bytes. - Checked the rest of
pack_command.rsfor the samewrite!(BStr) + from_buf(computed_len)pattern — the two changed sites were the only ones; remainingBStr::newuses are display/error output where U+FFFD substitution is intended. - On Windows the path still goes through
set_pathname_utf8; Windows filenames are UTF-16-derived sofilename.as_bytes()is already valid UTF-8 there — no behavior change. - Test reads the ustar header and pax
pathrecord byte-for-byte via latin1 (avoidingreadTarball's string decode that would collapse both correct and buggy output onto U+FFFD), and is Linux-gated with a stated reason.
Extended reasoning...
Overview
Two small edits in src/runtime/cli/pack_command.rs (the gzip compression-level filter option and the per-entry pathname in add_archive_entry) replace write!(print_buf, "{}...\x00", BStr(...)) + ZStr::from_buf(buf, computed_len) with extend_from_slice + push(0) + ZStr::from_slice_with_nul. A Linux-only test in test/cli/install/bun-pack.test.ts gunzips the produced tarball and asserts the raw bytes of both the ustar name field and the pax path record match the on-disk filename bytes.
The old code had two coupled defects: BStr's Display replaces each invalid-UTF-8 byte with U+FFFD (3 bytes), so (a) the length passed to from_buf no longer pointed at the NUL (debug assertion), and (b) in release builds distinct filenames collapsed to the same tar member. The new code writes the bytes verbatim and derives the NUL position from the buffer itself, eliminating both.
Security risks
None identified. The change narrows behavior toward byte-for-byte fidelity of filenames already on disk; it does not introduce new parsing of untrusted input, and libarchive's pax writer already handles non-UTF-8 names via hdrcharset=BINARY. Interior NULs are impossible in POSIX filenames, so from_slice_with_nul's trailing-NUL contract holds.
Level of scrutiny
Low-to-medium. This is a ~10-line mechanical fix in a CLI packing path, using an existing in-tree helper (ZStr::from_slice_with_nul, whose doc comment describes exactly this call shape). The invariant it relies on — print_buf is empty on entry — is locally verifiable: print_buf is created empty, cleared at line 2507 after the compression-level use, and cleared at line 3209 after each entry write; no other writes occur between clears. I grepped for sibling sites combining lossy BStr formatting with a separately-computed length passed to from_buf and found none — the remaining BStr::new calls are all in Output::err_generic/log formatting where lossy display is correct, and remaining from_buf calls copy bytes directly into a fixed buffer with the NUL written at the same computed offset.
Other factors
The test is careful: it can't use readTarball (which would decode both the correct and the buggy bytes to the same JS string), so it hand-parses the 512-byte ustar blocks and pax extended-header records with latin1 decoding, sorts by content for determinism, and asserts exact Set equality on names. The skipIf(!isLinux) is justified in a comment (APFS rejects such names; Windows names are UTF-16). No CODEOWNERS cover these files. CI shows a red build, but given the scope (two byte-append sites + one Linux-only test) and that the full bun-pack.test.ts suite is reported passing, any failure there is worth checking against main before attributing it to this change.
2 participants
Problem
bun pm pack(andbun publish, which packs the same way) writes a file whose name is not valid UTF-8 into the tarball under a different name: each offending byte becomes U+FFFD. Two files such asx_\xe9andx_\xff(legal names on Linux) both end up aspackage/x_\xef\xbf\xbd, so extracting or installing the tarball keeps only one of them. The command exits 0 and reportsTotal files: 3.assertion 'left == right' failed: ZStr::from_buf: missing NUL at buf[len].add_archive_entryinsrc/runtime/cli/pack_command.rsbuilt the member name withwrite!(print_buf, "{}{}\x00", BStr(..), BStr(filename)).BStr'sDisplayis lossy (it substitutes U+FFFD, 3 bytes, for each invalid byte), but the length passed toZStr::from_bufwas computed from the original filename. In release builds libarchive reads the C string up to its real NUL and stores the substituted name; in debug builds the NUL check at the computed length fails.--gzip-level) a few lines earlier inpack()was built the same way. Its release behavior was already correct (libarchive rejects any non-numeric value), but a non-UTF-8 value tripped the same debug assertion.Fix
package/, the filename bytes, and the NUL toprint_bufdirectly and take the buffer as written withZStr::from_slice_with_nul, so the bytes libarchive receives are the directory entry's bytes. Same change for the compression level.hdrcharset=BINARYwith the rawpath. GNU tar extracts both files under their original names.test/cli/install/bun-pack.test.ts, "filenames that are not valid UTF-8 are stored byte for byte" (Linux only; APFS rejects such names and Windows names are UTF-16). It gunzips the tarball and reads the names out of the ustar header and the paxpathrecord itself, becausereadTarballfrombun:internal-for-testingdecodes names to JS strings, which maps both the correct and the substituted bytes onto U+FFFD.x_\xe9is stored asx_\xef\xbf\xbd), passes with this change.bun-pack.test.ts(77 tests, including the--gzip-levelones) passes with the change.Background
ZStris bun's borrowed NUL-terminated byte string.ZStr::from_buf(buf, len)trusts the caller thatbuf[len] == 0(debug-asserted only);ZStr::from_slice_with_nul(buf)takes a slice whose last byte is the NUL, so the length can only come from what was actually written.bstr::BStris a[u8]wrapper whoseDisplayimpl prints bytes as text, replacing invalid UTF-8 with U+FFFD. That is what thepacked ... x_\u{fffd}progress lines want, and what the tarball must not get.bun pm packwrites (archive_write_set_format_pax_restricted). For a name libarchive cannot represent as UTF-8 it adds an extended header withhdrcharset=BINARY, telling extractors thepathrecord holds raw bytes.Reproduction
Before (
tar tvzf, raw header bytes):After:
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-pack.test.ts