Merge pull request #288 from figsoda/recursive-compression

fix infinite compression if output file is inside the input folder

CHANGELOG.md

@@ -56,6 +56,7 @@ Categories Used:
 - Recover last modified time when unpacking zip archives [\#250](https://github.com/ouch-org/ouch/pull/250) ([vrmiguel](https://github.com/vrmiguel))
 - Remove single quotes from clap doc comments [\#251](https://github.com/ouch-org/ouch/pull/251) ([jcgruenhage](https://github.com/jcgruenhage))
 - Fix incorrect warnings for decompression [\#270](https://github.com/ouch-org/ouch/pull/270) ([figsoda](https://github.com/figsoda))
+- Fix infinite compression if output file is inside the input folder [\#288](https://github.com/ouch-org/ouch/pull/288) ([figsoda](https://github.com/figsoda))
 
 ### Improvements
 

Cargo.lock

@@ -557,6 +557,7 @@ dependencies = [
  "parse-display",
  "proptest",
  "rand",
+ "same-file",
  "snap",
  "tar",
  "tempfile",

Cargo.toml

@@ -23,6 +23,7 @@ libc = "0.2.135"
 linked-hash-map = "0.5.6"
 lzzzz = "1.0.3"
 once_cell = "1.15.0"
+same-file = "1.0.6"
 snap = "1.0.5"
 tar = "0.4.38"
 xz2 = "0.1.7"

src/archive/tar.rs

@@ -10,6 +10,7 @@ use std::{
 
 use fs_err as fs;
 use humansize::{format_size, DECIMAL};
+use same_file::Handle;
 
 use crate::{
     error::FinalError,
@@ -17,6 +18,7 @@ use crate::{
     list::FileInArchive,
     progress::OutputLine,
     utils::{self, FileVisibilityPolicy},
+    warning,
 };
 
 /// Unpacks the archive given by `archive` into the folder given by `into`.
@@ -87,6 +89,7 @@ pub fn list_archive(
 /// Compresses the archives given by `input_filenames` into the file given previously to `writer`.
 pub fn build_archive_from_paths<W, D>(
     input_filenames: &[PathBuf],
+    output_path: &Path,
     writer: W,
     file_visibility_policy: FileVisibilityPolicy,
     mut log_out: D,
@@ -96,6 +99,7 @@ where
     D: OutputLine,
 {
     let mut builder = tar::Builder::new(writer);
+    let output_handle = Handle::from_path(output_path);
 
     for filename in input_filenames {
         let previous_location = utils::cd_into_same_dir_as(filename)?;
@@ -107,6 +111,18 @@ where
             let entry = entry?;
             let path = entry.path();
 
+            // If the output_path is the same as the input file, warn the user and skip the input (in order to avoid compression recursion)
+            if let Ok(ref handle) = output_handle {
+                if matches!(Handle::from_path(path), Ok(x) if &x == handle) {
+                    warning!(
+                        @log_out,
+                        "The output file and the input file are the same: `{}`, skipping...",
+                        output_path.display()
+                    );
+                    continue;
+                }
+            }
+
             // This is printed for every file in `input_filenames` and has
             // little importance for most users, but would generate lots of
             // spoken text for users using screen readers, braille displays

src/archive/zip.rs

@@ -14,6 +14,7 @@ use std::{
 use filetime::{set_file_mtime, FileTime};
 use fs_err as fs;
 use humansize::{format_size, DECIMAL};
+use same_file::Handle;
 use zip::{self, read::ZipFile, DateTime, ZipArchive};
 
 use crate::{
@@ -25,6 +26,7 @@ use crate::{
         self, cd_into_same_dir_as, get_invalid_utf8_paths, pretty_format_list_of_paths, strip_cur_dir, to_utf,
         FileVisibilityPolicy,
     },
+    warning,
 };
 
 /// Unpacks the archive given by `archive` into the folder given by `output_folder`.
@@ -138,6 +140,7 @@ where
 /// Compresses the archives given by `input_filenames` into the file given previously to `writer`.
 pub fn build_archive_from_paths<W, D>(
     input_filenames: &[PathBuf],
+    output_path: &Path,
     writer: W,
     file_visibility_policy: FileVisibilityPolicy,
     mut log_out: D,
@@ -148,6 +151,7 @@ where
 {
     let mut writer = zip::ZipWriter::new(writer);
     let options = zip::write::FileOptions::default();
+    let output_handle = Handle::from_path(output_path);
 
     #[cfg(not(unix))]
     let executable = options.unix_permissions(0o755);
@@ -176,6 +180,18 @@ where
             let entry = entry?;
             let path = entry.path();
 
+            // If the output_path is the same as the input file, warn the user and skip the input (in order to avoid compression recursion)
+            if let Ok(ref handle) = output_handle {
+                if matches!(Handle::from_path(path), Ok(x) if &x == handle) {
+                    warning!(
+                        @log_out,
+                        "The output file and the input file are the same: `{}`, skipping...",
+                        output_path.display()
+                    );
+                    continue;
+                }
+            }
+
             // This is printed for every file in `input_filenames` and has
             // little importance for most users, but would generate lots of
             // spoken text for users using screen readers, braille displays

src/commands/compress.rs

@@ -30,7 +30,7 @@ pub fn compress_files(
     files: Vec<PathBuf>,
     extensions: Vec<Extension>,
     output_file: fs::File,
-    output_dir: &Path,
+    output_path: &Path,
     question_policy: QuestionPolicy,
     file_visibility_policy: FileVisibilityPolicy,
 ) -> crate::Result<bool> {
@@ -89,12 +89,24 @@ pub fn compress_files(
         }
         Tar => {
             if is_running_in_accessible_mode() {
-                archive::tar::build_archive_from_paths(&files, &mut writer, file_visibility_policy, io::stderr())?;
+                archive::tar::build_archive_from_paths(
+                    &files,
+                    output_path,
+                    &mut writer,
+                    file_visibility_policy,
+                    io::stderr(),
+                )?;
                 writer.flush()?;
             } else {
                 let mut progress = Progress::new(total_input_size, precise, true);
                 let mut writer = progress.wrap_write(writer);
-                archive::tar::build_archive_from_paths(&files, &mut writer, file_visibility_policy, &mut progress)?;
+                archive::tar::build_archive_from_paths(
+                    &files,
+                    output_path,
+                    &mut writer,
+                    file_visibility_policy,
+                    &mut progress,
+                )?;
                 writer.flush()?;
             }
         }
@@ -103,7 +115,7 @@ pub fn compress_files(
                 warn_user_about_loading_zip_in_memory();
 
                 // give user the option to continue compressing after warning is shown
-                if !user_wants_to_continue(output_dir, question_policy, QuestionAction::Compression)? {
+                if !user_wants_to_continue(output_path, question_policy, QuestionAction::Compression)? {
                     return Ok(false);
                 }
             }
@@ -111,12 +123,24 @@ pub fn compress_files(
             let mut vec_buffer = Cursor::new(vec![]);
 
             if is_running_in_accessible_mode() {
-                archive::zip::build_archive_from_paths(&files, &mut vec_buffer, file_visibility_policy, io::stderr())?;
+                archive::zip::build_archive_from_paths(
+                    &files,
+                    output_path,
+                    &mut vec_buffer,
+                    file_visibility_policy,
+                    io::stderr(),
+                )?;
                 vec_buffer.rewind()?;
                 io::copy(&mut vec_buffer, &mut writer)?;
             } else {
                 let mut progress = Progress::new(total_input_size, precise, true);
-                archive::zip::build_archive_from_paths(&files, &mut vec_buffer, file_visibility_policy, &mut progress)?;
+                archive::zip::build_archive_from_paths(
+                    &files,
+                    output_path,
+                    &mut vec_buffer,
+                    file_visibility_policy,
+                    &mut progress,
+                )?;
                 vec_buffer.rewind()?;
                 io::copy(&mut progress.wrap_read(vec_buffer), &mut writer)?;
             }

src/commands/mod.rs

@@ -91,13 +91,9 @@ pub fn run(
 ) -> crate::Result<()> {
     match args.cmd {
         Subcommand::Compress {
-            mut files,
+            files,
             output: output_path,
         } => {
-            // If the output_path file exists and is the same as some of the input files, warn the user and skip those inputs (in order to avoid compression recursion)
-            if output_path.exists() {
-                deduplicate_input_files(&mut files, &fs::canonicalize(&output_path)?);
-            }
             // After cleaning, if there are no input files left, exit
             if files.is_empty() {
                 return Err(FinalError::with_title("No files to compress").into());
@@ -361,21 +357,6 @@ fn check_mime_type(
     Ok(ControlFlow::Continue(()))
 }
 
-fn deduplicate_input_files(files: &mut Vec<PathBuf>, output_path: &Path) {
-    let mut idx = 0;
-    while idx < files.len() {
-        if files[idx] == output_path {
-            warning!(
-                "The output file and the input file are the same: `{}`, skipping...",
-                output_path.display()
-            );
-            files.remove(idx);
-        } else {
-            idx += 1;
-        }
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use std::path::Path;

src/macros.rs

@@ -50,6 +50,11 @@ macro_rules! info {
 /// Macro that prints \[WARNING\] messages, wraps [`eprintln`].
 #[macro_export]
 macro_rules! warning {
+    (@$log_out: expr, $($arg:tt)*) => {
+        if !$crate::accessible::is_running_in_accessible_mode() {
+            $log_out.output_line_warning(format_args!($($arg)*));
+        }
+    };
     ($($arg:tt)*) => {
         $crate::macros::_warning_helper();
         eprintln!($($arg)*);

src/progress.rs

@@ -6,7 +6,7 @@ use std::{
 
 use indicatif::{ProgressBar, ProgressBarIter, ProgressStyle};
 
-use crate::utils::colors::{RESET, YELLOW};
+use crate::utils::colors::{ORANGE, RESET, YELLOW};
 
 /// Draw a ProgressBar using a function that checks periodically for the progress
 pub struct Progress {
@@ -16,6 +16,7 @@ pub struct Progress {
 pub trait OutputLine {
     fn output_line(&mut self, args: Arguments);
     fn output_line_info(&mut self, args: Arguments);
+    fn output_line_warning(&mut self, args: Arguments);
 }
 
 impl OutputLine for Progress {
@@ -26,6 +27,10 @@ impl OutputLine for Progress {
     fn output_line_info(&mut self, args: Arguments) {
         self.bar.set_message(format!("{}[INFO]{} {args}", *YELLOW, *RESET));
     }
+
+    fn output_line_warning(&mut self, args: Arguments) {
+        self.bar.println(format!("{}[WARNING]{} {args}", *ORANGE, *RESET));
+    }
 }
 
 impl OutputLine for Stderr {
@@ -34,7 +39,13 @@ impl OutputLine for Stderr {
     }
 
     fn output_line_info(&mut self, args: Arguments) {
-        write!(self, "{}[INFO]{} {args}", *YELLOW, *RESET).unwrap();
+        write!(self, "{}[INFO]{} ", *YELLOW, *RESET).unwrap();
+        self.write_fmt(args).unwrap();
+        self.write_all(b"\n").unwrap();
+    }
+
+    fn output_line_warning(&mut self, args: Arguments) {
+        write!(self, "{}[WARNING]{}", *ORANGE, *RESET).unwrap();
         self.write_fmt(args).unwrap();
         self.write_all(b"\n").unwrap();
     }
@@ -48,6 +59,10 @@ impl<T: OutputLine + ?Sized> OutputLine for &mut T {
     fn output_line_info(&mut self, args: Arguments) {
         (*self).output_line_info(args);
     }
+
+    fn output_line_warning(&mut self, args: Arguments) {
+        (*self).output_line_warning(args);
+    }
 }
 
 impl Progress {