fix: clearer error when tar extraction hits a leftover compression layer

src/archive/tar.rs

@@ -4,7 +4,7 @@
 use std::os::unix::fs::MetadataExt;
 use std::{
     collections::HashMap,
-    io::{self, prelude::*},
+    io::{self, BufReader, prelude::*},
     ops::Not,
     path::{Path, PathBuf},
 };
@@ -25,16 +25,73 @@ use crate::{
     warning,
 };
 
+/// Magic bytes of common compression formats, checked against the start of a tar stream to
+/// give a clearer error when the data is still compressed (one extra layer of compression
+/// that the filename did not account for, e.g. an actual `.tar.gz.gz` misnamed `.tgz`).
+const LEFTOVER_COMPRESSION_MAGIC: &[(&[u8], &str)] = &[
+    (&[0x1f, 0x8b], "gzip"),
+    (&[0xfd, 0x37, 0x7a, 0x58, 0x5a, 0x00], "xz"),
+    (&[0x28, 0xb5, 0x2f, 0xfd], "zstd"),
+    (&[0x04, 0x22, 0x4d, 0x18], "lz4"),
+];
+
+/// If `peeked` starts with the magic bytes of a known compression format, return that
+/// format's name so the caller can explain why tar parsing is failing.
+fn detect_leftover_compression(peeked: &[u8]) -> Option<&'static str> {
+    // bzip2 starts with "BZh", a level digit and the block magic. The "BZh" alone is plain ASCII
+    // that a tar header's filename field can start with, so the longer sequence is required.
+    let is_bzip2 = peeked.starts_with(b"BZh")
+        && matches!(peeked.get(3), Some(b'1'..=b'9'))
+        && peeked.get(4..10) == Some(&[0x31, 0x41, 0x59, 0x26, 0x53, 0x59][..]);
+    if is_bzip2 {
+        return Some("bzip2");
+    }
+
+    LEFTOVER_COMPRESSION_MAGIC
+        .iter()
+        .find(|(magic, _)| peeked.starts_with(magic))
+        .map(|(_, name)| *name)
+}
+
+/// Wraps a tar-parsing `io::Error` with a clearer explanation when the stream starts with the
+/// magic bytes of a known compression format. Only called once tar parsing has actually failed,
+/// so a valid tar archive is never misdiagnosed just because its first entry's filename happens
+/// to start with bytes that collide with a magic sequence (e.g. a tar whose first file is named
+/// `BZh-notes.txt` starts with the same three bytes as the bzip2 magic).
+fn with_leftover_compression_hint(err: io::Error, leftover_compression_format: Option<&'static str>) -> crate::Error {
+    match leftover_compression_format {
+        Some(format) => FinalError::with_title("Could not extract archive")
+            .detail(format!(
+                "The decompressed data is still {format}-compressed instead of being a tar archive"
+            ))
+            .detail("This usually means the file was compressed one more time than its extension suggests")
+            .hint("Rename the file to include the missing compression extension, or decompress the extra layer manually before extracting it")
+            .into(),
+        None => err.into(),
+    }
+}
+
 /// Unpacks the archive given by `archive` into the folder given by `into`.
 /// 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);
+    // Peeked, but not acted on, before parsing: only used as a hint if parsing fails below, so a
+    // valid tar is never rejected just because its first bytes happen to collide with this.
+    let leftover_compression_format = detect_leftover_compression(reader.fill_buf()?);
+
     let mut archive = tar::Archive::new(reader);
 
     let mut files_unpacked = 0;
     let mut read_only_dirs_and_modes = Vec::new();
 
-    for entry in archive.entries()? {
-        let mut entry = entry?;
+    let entries = archive
+        .entries()
+        .map_err(|err| with_leftover_compression_hint(err, leftover_compression_format))?;
+
+    for (index, entry) in entries.enumerate() {
+        // Leftover compression breaks the very first header, so a later failure is a different problem.
+        let mut entry = entry
+            .map_err(|err| with_leftover_compression_hint(err, leftover_compression_format.filter(|_| index == 0)))?;
 
         // Set when the user renamed a file so the log can show the real path.
         let mut written = None;
@@ -264,3 +321,24 @@ where
 
     Ok(builder.into_inner()?)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::detect_leftover_compression;
+
+    #[test]
+    fn detects_real_bzip2_stream() {
+        assert_eq!(detect_leftover_compression(b"BZh91AY&SY\x00\x00"), Some("bzip2"));
+    }
+
+    #[test]
+    fn ignores_filenames_that_start_like_bzip2() {
+        assert_eq!(detect_leftover_compression(b"BZh-notes.txt\0\0\0\0"), None);
+        assert_eq!(detect_leftover_compression(b"BZh1-report.txt\0\0"), None);
+    }
+
+    #[test]
+    fn detects_gzip_stream() {
+        assert_eq!(detect_leftover_compression(&[0x1f, 0x8b, 0x08, 0x00]), Some("gzip"));
+    }
+}

tests/snapshots/ui__ui_test_err_decompress_tar_with_leftover_gzip_layer.snap

@@ -0,0 +1,9 @@
+---
+source: tests/ui.rs
+expression: "run_ouch(\"ouch decompress archive.tgz\", dir)"
+---
+[ERROR] Could not extract archive
+ - The decompressed data is still gzip-compressed instead of being a tar archive
+ - This usually means the file was compressed one more time than its extension suggests
+
+hint: Rename the file to include the missing compression extension, or decompress the extra layer manually before extracting it

tests/ui.rs

@@ -248,3 +248,54 @@ fn ui_test_err_decompress_output_file_exists() {
     // Answer "s" (skip) to the conflict prompt
     ui!(run_ouch_with_stdin("ouch decompress out.gz", dir, Some("s\n")));
 }
+
+#[test]
+fn ui_test_err_decompress_tar_with_leftover_gzip_layer() {
+    // Regression test for https://github.com/ouch-org/ouch/issues/882:
+    // a file like `archive.tar.gz.gz` that got misnamed `archive.tgz` only has its
+    // outer gzip layer stripped by the `.tgz` extension, leaving the tar parser to
+    // choke on what is still gzip-compressed data. That used to surface as a raw,
+    // confusing `tar` crate error (e.g. "numeric field did not have utf-8 text").
+    let (_dropper, dir) = testdir().unwrap();
+
+    create_files_in(dir, &["input.txt"]);
+
+    // Build `archive.tar.gz.gz`: a tar archive, gzip-compressed twice.
+    run_ouch("ouch compress input.txt archive.tar.gz", dir);
+    run_ouch("ouch compress archive.tar.gz archive.tar.gz.gz", dir);
+    std::fs::remove_file(dir.join("archive.tar.gz")).unwrap();
+
+    // Misname it as `.tgz`, so only one gzip layer gets stripped before the tar parser runs.
+    std::fs::rename(dir.join("archive.tar.gz.gz"), dir.join("archive.tgz")).unwrap();
+
+    ui!(run_ouch("ouch decompress archive.tgz", dir));
+}
+
+#[test]
+fn test_decompress_tar_with_entry_name_colliding_with_compression_magic() {
+    // A valid tar archive whose first entry's filename happens to start with the same bytes
+    // as a compression format's magic number (bzip2's is "BZh") must still extract correctly:
+    // the leftover-compression check that ui_test_err_decompress_tar_with_leftover_gzip_layer
+    // exercises above must only fire once tar parsing has genuinely failed, not just because
+    // the raw start of a valid tar stream happens to collide with a magic sequence.
+    let (_dropper, dir) = testdir().unwrap();
+
+    create_files_in(dir, &["BZh-notes.txt"]);
+    std::fs::write(
+        dir.join("BZh-notes.txt"),
+        b"not actually bzip2, just a filename that starts like it",
+    )
+    .unwrap();
+
+    run_ouch("ouch compress BZh-notes.txt archive.tar", dir);
+
+    std::fs::create_dir(dir.join("output")).unwrap();
+    let output = run_ouch("ouch decompress archive.tar --dir output", dir);
+    assert!(
+        !output.to_lowercase().contains("error"),
+        "extraction should succeed, got: {output}"
+    );
+
+    let extracted = std::fs::read_to_string(dir.join("output").join("BZh-notes.txt")).unwrap();
+    assert_eq!(extracted, "not actually bzip2, just a filename that starts like it");
+}