Repository navigation
fix: clearer error when tar extraction hits a leftover compression layer - #1095
Saadanjum0 wants to merge 2 commits into
Conversation
A file like archive.tar.gz.gz that gets misnamed archive.tgz only has its outer gzip layer stripped before the tar parser runs, since the extension only names one layer of compression. The tar parser then tries to read what is still gzip-compressed bytes as tar headers, and the `tar` crate's raw low-level error (e.g. "numeric field did not have utf-8 text" or "failed to read entire block") gives no indication of what actually went wrong. Peek the first few bytes of the stream before handing it to `tar::Archive` and, if they match a known compression format's magic number (gzip, bzip2, xz, zstd or lz4), fail with a clear explanation instead of letting the tar parser produce a confusing error on data it was never meant to read. Fixes ouch-org#882
| /// Assumes that output_folder is empty | ||
| pub fn unpack_archive(reader: impl Read, output_folder: &Path, question_policy: QuestionPolicy) -> Result<u64> { | ||
| let mut reader = BufReader::new(reader); | ||
| if let Some(format) = detect_leftover_compression(reader.fill_buf()?) { |
There was a problem hiding this comment.
The prefix check rejects valid tar archives whose first member name begins with BZh. Tar headers start with the filename, so BZh-notes.txt (also BZh1-report.txt) matches the bzip2 prefix without being compressed data.
I reproduced this with a valid gzip-wrapped tar: listing succeeds on both revisions; extraction preserves the payload on base 6df14b0, but head c5f2cb1 fails with “The decompressed data is still bzip2-compressed”. This was tested on Windows with Rust1.93 and --no-default-features. A normal first filename extracts on both.
Could the check preserve valid tar headers before declaring leftover compression—for example by checking the tar header/checksum or applying the diagnostic after a tar parse failure? A regression archive whose first member is BZh-notes.txt would cover this.
…y fails
detect_leftover_compression() ran unconditionally on the raw start of the
stream before any tar parsing, checking it against known compression magic
bytes. A tar header's first bytes are its filename field, and bzip2's magic
('BZh', 0x42 0x5a 0x68) is plain ASCII, so a perfectly valid tar archive
whose first entry happens to be named something like 'BZh-notes.txt' would
false-positive: extraction failed with 'still bzip2-compressed' even though
the archive was never compressed at all.
Move the check so it only runs once tar parsing has genuinely failed
(archive.entries() or an individual entry() read), using the magic-byte
match purely as an explanation for that failure rather than as a pre-emptive
gate before parsing even starts. A valid tar is now never rejected based on
what its first entry happens to be named.
Added a regression test alongside the existing leftover-compression test:
a tar whose sole entry is 'BZh-notes.txt' must extract successfully. Fails
on the previous code with exactly this false positive, passes with the fix;
the original leftover-compression test still passes unchanged.
|
Good catch, thanks. Pushed 4b7d0f3 — moved the check so it only fires after tar parsing has actually failed, using the magic-byte match purely to explain that failure rather than as a gate before parsing starts. A valid tar is never rejected based on what its first entry happens to be named now. Added a regression test alongside the existing one: a tar whose sole entry is literally named |
|
Thanks, I rechecked this on Windows with no default features. Both |
|
The hint is added to every tar header error and not only to a failure on the first header and a corrupt tar whose first file name starts with BZh reports a false "still bzip2-compressed" error. |
3 participants
Problem
When a file is actually compressed twice (e.g. a real
archive.tar.gz.gz) but getsrenamed/misnamed to only reflect one layer of compression (
archive.tgz),ouchstrips only the single gzip layer the extension names, then hands what is still
gzip-compressed data to the
tarcrate as if it were a tar stream.The
tarcrate then fails with a low-level, confusing error that gives no hintabout what actually happened, e.g.:
or (depending on exactly which bytes it chokes on):
See #882.
Cause
crate::archive::tar::unpack_archivewraps the already-decompressed readerdirectly in
tar::Archive::new(reader)with no sanity check. If the bytes handedto it are still compressed (because the input had one more compression layer than
its extension/
--formatindicated), the tar parser just interprets arbitrarycompressed bytes as tar headers and surfaces whatever internal error that
produces.
Fix
Before constructing the
tar::Archive, peek the first few bytes of the stream(via
BufReader::fill_buf, which does not consume them) and check them againstthe magic numbers of the compression formats
ouchalready supports (gzip,bzip2, xz, zstd, lz4). If they match, fail immediately with a clear, actionable
error instead of letting the tar parser produce a confusing one:
This is purely a diagnostics change: legitimate tar streams are unaffected (a tar
header's first bytes are a filename field, which cannot coincidentally match any
of these binary magic numbers), and the peeked bytes are not consumed, so normal
parsing proceeds exactly as before when there's no leftover compression layer.
Testing
Added
ui_test_err_decompress_tar_with_leftover_gzip_layertotests/ui.rs,which builds a real
archive.tar.gz.gz(tar, gzip-compressed twice), renames itto
archive.tgzto reproduce the mismatch, and snapshot-asserts the new errormessage.
the raw
tarcrate error (failed to read entire blockin this environment,same error class as the "numeric field did not have utf-8 text" from the
issue).
shown above.
cargo test --profile fast(full suite): 6/6 test binaries pass, includingthis new test.
cargo fmt --all -- --check: clean.cargo clippy --all --all-targets --profile fast -- -D warnings: clean.Fixes #882