test(fuzz): Make cargo fuzz fmt fuzz_write output more reliably equivalent to the code path it follows (#224)

fuzz/fuzz_targets/fuzz_write.rs

@@ -1,14 +1,18 @@
 #![no_main]
 
 use arbitrary::Arbitrary;
-use core::fmt::{Debug, Formatter};
+use core::fmt::{Debug};
 use libfuzzer_sys::fuzz_target;
 use replace_with::replace_with_or_abort;
 use std::borrow::Cow;
-use std::io::{Cursor, Read, Seek, Write};
+use std::fmt::{Arguments, Formatter, Write};
+use std::io::Cursor;
+use std::io::Write as IoWrite;
 use std::path::PathBuf;
 use tikv_jemallocator::Jemalloc;
+use zip::result::{ZipError, ZipResult};
 use zip::unstable::path_to_string;
+use zip::write::FullFileOptions;
 
 #[global_allocator]
 static GLOBAL: Jemalloc = Jemalloc;
@@ -17,12 +21,12 @@ static GLOBAL: Jemalloc = Jemalloc;
 pub enum BasicFileOperation<'k> {
     WriteNormalFile {
         contents: Box<[Box<[u8]>]>,
-        options: zip::write::FullFileOptions<'k>,
+        options: FullFileOptions<'k>,
     },
-    WriteDirectory(zip::write::FullFileOptions<'k>),
+    WriteDirectory(FullFileOptions<'k>),
     WriteSymlinkWithTarget {
         target: PathBuf,
-        options: zip::write::FullFileOptions<'k>,
+        options: FullFileOptions<'k>,
     },
     ShallowCopy(Box<FileOperation<'k>>),
     DeepCopy(Box<FileOperation<'k>>),
@@ -61,123 +65,12 @@ impl<'k> FileOperation<'k> {
     }
 }
 
-impl<'k> Debug for FileOperation<'k> {
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        match &self.basic {
-            BasicFileOperation::WriteNormalFile { contents, options } => {
-                f.write_fmt(format_args!(
-                    "let options = {:?};\n\
-                writer.start_file_from_path({:?}, options)?;\n",
-                    options, self.path
-                ))?;
-                for content_slice in contents {
-                    f.write_fmt(format_args!("writer.write_all(&({:?}))?;\n", content_slice))?;
-                }
-            }
-            BasicFileOperation::WriteDirectory(options) => {
-                f.write_fmt(format_args!(
-                    "let options = {:?};\n\
-                writer.add_directory_from_path({:?}, options)?;\n",
-                    options, self.path
-                ))?;
-            }
-            BasicFileOperation::WriteSymlinkWithTarget { target, options } => {
-                f.write_fmt(format_args!(
-                    "let options = {:?};\n\
-                writer.add_symlink_from_path({:?}, {:?}, options)?;\n",
-                    options,
-                    self.path,
-                    target.to_owned()
-                ))?;
-            }
-            BasicFileOperation::ShallowCopy(base) => {
-                let Some(base_path) = base.get_path() else {
-                    return Ok(());
-                };
-                f.write_fmt(format_args!(
-                    "{:?}writer.shallow_copy_file_from_path({:?}, {:?})?;\n",
-                    base, base_path, self.path
-                ))?;
-            }
-            BasicFileOperation::DeepCopy(base) => {
-                let Some(base_path) = base.get_path() else {
-                    return Ok(());
-                };
-                f.write_fmt(format_args!(
-                    "{:?}writer.deep_copy_file_from_path({:?}, {:?})?;\n",
-                    base, base_path, self.path
-                ))?;
-            }
-            BasicFileOperation::MergeWithOtherFile { operations } => {
-                f.write_str(
-                    "let sub_writer = {\n\
-                    let mut writer = ZipWriter::new(Cursor::new(Vec::new()));\n\
-                    writer.set_flush_on_finish_file(false);\n",
-                )?;
-                operations
-                    .iter()
-                    .map(|op| {
-                        f.write_fmt(format_args!("{:?}", op.0))?;
-                        if op.1 {
-                            f.write_str("writer.abort_file()?;\n")
-                        } else {
-                            Ok(())
-                        }
-                    })
-                    .collect::<Result<(), _>>()?;
-                f.write_str(
-                    "writer\n\
-                };\n\
-                writer.merge_archive(sub_writer.finish_into_readable()?)?;\n",
-                )?;
-            }
-            BasicFileOperation::SetArchiveComment(comment) => {
-                f.write_fmt(format_args!(
-                    "writer.set_raw_comment({:?}.into());\n",
-                    comment
-                ))?;
-            }
-        }
-        match &self.reopen {
-            ReopenOption::DoNotReopen => Ok(()),
-            ReopenOption::ViaFinish => {
-                f.write_str("writer = ZipWriter::new_append(writer.finish()?)?;\n")
-            }
-            ReopenOption::ViaFinishIntoReadable => f.write_str(
-                "writer = ZipWriter::new_append(writer.finish_into_readable()?.into_inner())?;\n",
-            ),
-        }
-    }
-}
-
 #[derive(Arbitrary, Clone)]
 pub struct FuzzTestCase<'k> {
     operations: Box<[(FileOperation<'k>, bool)]>,
     flush_on_finish_file: bool,
 }
 
-impl<'k> Debug for FuzzTestCase<'k> {
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        f.write_fmt(format_args!(
-            "let mut writer = ZipWriter::new(Cursor::new(Vec::new()));\n\
-            writer.set_flush_on_finish_file({:?});\n",
-            self.flush_on_finish_file
-        ))?;
-        self.operations
-            .iter()
-            .map(|op| {
-                f.write_fmt(format_args!("{:?}", op.0))?;
-                if op.1 {
-                    f.write_str("writer.abort_file()?;\n")
-                } else {
-                    Ok(())
-                }
-            })
-            .collect::<Result<(), _>>()?;
-        f.write_str("writer\n")
-    }
-}
-
 fn deduplicate_paths(copy: &mut Cow<PathBuf>, original: &PathBuf) {
     if path_to_string(&**copy) == path_to_string(original) {
         let new_path = match original.file_name() {
@@ -192,16 +85,15 @@ fn deduplicate_paths(copy: &mut Cow<PathBuf>, original: &PathBuf) {
     }
 }
 
-fn do_operation<'k, T>(
+fn do_operation<'k>(
-    writer: &mut zip::ZipWriter<T>,
+    writer: &mut zip::ZipWriter<Cursor<Vec<u8>>>,
     operation: &FileOperation<'k>,
     abort: bool,
     flush_on_finish_file: bool,
     files_added: &mut usize,
-) -> Result<(), Box<dyn std::error::Error>>
+    stringifier: &mut impl Write,
-where
+    panic_on_error: bool
-    T: Read + Write + Seek,
+) -> Result<(), Box<dyn std::error::Error>> {
-{
     writer.set_flush_on_finish_file(flush_on_finish_file);
     let mut path = Cow::Borrowed(&operation.path);
     match &operation.basic {
@@ -213,17 +105,25 @@ where
             if uncompressed_size >= u32::MAX as usize {
                 options = options.large_file(true);
             }
+            if options == FullFileOptions::default() {
+                writeln!(stringifier, "writer.start_file_from_path({:?}, Default::default())?;", path)?;
+            } else {
+                writeln!(stringifier, "writer.start_file_from_path({:?}, {:?})?;", path, options)?;
+            }
             writer.start_file_from_path(&*path, options)?;
             for chunk in contents.iter() {
+                writeln!(stringifier, "writer.write_all(&{:?})?;", chunk)?;
                 writer.write_all(&chunk)?;
             }
             *files_added += 1;
         }
         BasicFileOperation::WriteDirectory(options) => {
+            writeln!(stringifier, "writer.add_directory_from_path(&{:?}, {:?})?;", path, options)?;
             writer.add_directory_from_path(&*path, options.to_owned())?;
             *files_added += 1;
         }
         BasicFileOperation::WriteSymlinkWithTarget { target, options } => {
+            writeln!(stringifier, "writer.add_symlink_from_path(&{:?}, {:?}, {:?});", path, target, options)?;
             writer.add_symlink_from_path(&*path, target, options.to_owned())?;
             *files_added += 1;
         }
@@ -232,7 +132,8 @@ where
                 return Ok(());
             };
             deduplicate_paths(&mut path, &base_path);
-            do_operation(writer, &base, false, flush_on_finish_file, files_added)?;
+            do_operation(writer, &base, false, flush_on_finish_file, files_added, stringifier, panic_on_error)?;
+            writeln!(stringifier, "writer.shallow_copy_file_from_path({:?}, {:?});", base_path, path)?;
             writer.shallow_copy_file_from_path(&*base_path, &*path)?;
             *files_added += 1;
         }
@@ -241,13 +142,16 @@ where
                 return Ok(());
             };
             deduplicate_paths(&mut path, &base_path);
-            do_operation(writer, &base, false, flush_on_finish_file, files_added)?;
+            do_operation(writer, &base, false, flush_on_finish_file, files_added, stringifier, panic_on_error)?;
+            writeln!(stringifier, "writer.deep_copy_file_from_path({:?}, {:?});", base_path, path)?;
             writer.deep_copy_file_from_path(&*base_path, &*path)?;
             *files_added += 1;
         }
         BasicFileOperation::MergeWithOtherFile { operations } => {
             let mut other_writer = zip::ZipWriter::new(Cursor::new(Vec::new()));
             let mut inner_files_added = 0;
+            writeln!(stringifier,
+                "let sub_writer = {{\nlet mut writer = ZipWriter::new(Cursor::new(Vec::new()));")?;
             operations.iter().for_each(|(operation, abort)| {
                 let _ = do_operation(
                     &mut other_writer,
@@ -255,16 +159,21 @@ where
                     *abort,
                     false,
                     &mut inner_files_added,
+                    stringifier,
+                    panic_on_error
                 );
             });
+            writeln!(stringifier, "writer\n}};\nwriter.merge_archive(sub_writer.finish_into_readable()?)?;")?;
             writer.merge_archive(other_writer.finish_into_readable()?)?;
             *files_added += inner_files_added;
         }
         BasicFileOperation::SetArchiveComment(comment) => {
+            writeln!(stringifier, "writer.set_raw_comment({:?})?;", comment)?;
             writer.set_raw_comment(comment.clone());
         }
     }
     if abort && *files_added != 0 {
+        writeln!(stringifier, "writer.abort_file()?;")?;
         writer.abort_file()?;
         *files_added -= 1;
     }
@@ -272,19 +181,39 @@ where
     // comment, then there will be junk after the new comment that we can't get rid of. Thus, we
     // can only check that the expected is a prefix of the actual
     match operation.reopen {
-        ReopenOption::DoNotReopen => return Ok(()),
+        ReopenOption::DoNotReopen => {
+            writeln!(stringifier, "writer")?;
+            return Ok(())
+        },
         ReopenOption::ViaFinish => {
             let old_comment = writer.get_raw_comment().to_owned();
-            replace_with_or_abort(writer, |old_writer: zip::ZipWriter<T>| {
+            writeln!(stringifier, "let mut writer = ZipWriter::new_append(writer.finish()?)?;")?;
-                zip::ZipWriter::new_append(old_writer.finish().unwrap()).unwrap()
+            replace_with_or_abort(writer, |old_writer: zip::ZipWriter<Cursor<Vec<u8>>>| {
+                (|| -> ZipResult<zip::ZipWriter<Cursor<Vec<u8>>>> {
+                    zip::ZipWriter::new_append(old_writer.finish()?)
+                })().unwrap_or_else(|_| {
+                    if panic_on_error {
+                        panic!("Failed to create new ZipWriter")
+                    }
+                    zip::ZipWriter::new(Cursor::new(Vec::new()))
+                })
             });
+            if panic_on_error {
                 assert!(writer.get_raw_comment().starts_with(&old_comment));
             }
+        }
         ReopenOption::ViaFinishIntoReadable => {
             let old_comment = writer.get_raw_comment().to_owned();
-            replace_with_or_abort(writer, |old_writer: zip::ZipWriter<T>| {
+            writeln!(stringifier, "let mut writer = ZipWriter::new_append(writer.finish()?)?;")?;
-                zip::ZipWriter::new_append(old_writer.finish_into_readable().unwrap().into_inner())
+            replace_with_or_abort(writer, |old_writer| {
-                    .unwrap()
+                (|| -> ZipResult<zip::ZipWriter<Cursor<Vec<u8>>>> {
+                    zip::ZipWriter::new_append(old_writer.finish()?)
+                })().unwrap_or_else(|_| {
+                    if panic_on_error {
+                        panic!("Failed to create new ZipWriter")
+                    }
+                    zip::ZipWriter::new(Cursor::new(Vec::new()))
+                })
             });
             assert!(writer.get_raw_comment().starts_with(&old_comment));
         }
@@ -292,27 +221,63 @@ where
     Ok(())
 }
 
-fuzz_target!(|test_case: FuzzTestCase| {
+impl <'k> FuzzTestCase<'k> {
+    fn execute(&self, stringifier: &mut impl Write, panic_on_error: bool) -> ZipResult<()> {
         let mut files_added = 0;
         let mut writer = zip::ZipWriter::new(Cursor::new(Vec::new()));
         let mut final_reopen = false;
-    if let Some((last_op, _)) = test_case.operations.last() {
+        if let Some((last_op, _)) = self.operations.last() {
             if last_op.reopen != ReopenOption::ViaFinishIntoReadable {
                 final_reopen = true;
             }
         }
         #[allow(unknown_lints)]
         #[allow(boxed_slice_into_iter)]
-    for (operation, abort) in test_case.operations.into_iter() {
+        for (operation, abort) in self.operations.iter() {
             let _ = do_operation(
                 &mut writer,
                 &operation,
                 *abort,
-            test_case.flush_on_finish_file,
+                self.flush_on_finish_file,
                 &mut files_added,
+                stringifier,
+                panic_on_error
             );
         }
         if final_reopen {
-        let _ = writer.finish_into_readable().unwrap();
+            writeln!(stringifier, "let _ = writer.finish_into_readable()?;")
+                .map_err(|_| ZipError::InvalidArchive(""))?;
+            let _ = writer.finish_into_readable()?;
         }
+        Ok(())
+    }
+}
+
+impl <'k> Debug for FuzzTestCase<'k> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        writeln!(f, "let mut writer = ZipWriter::new(Cursor::new(Vec::new()));")?;
+        let _ = self.execute(f, false);
+        Ok(())
+    }
+}
+
+#[derive(Default, Eq, PartialEq)]
+struct NoopWrite {}
+
+impl Write for NoopWrite {
+    fn write_str(&mut self, _: &str) -> std::fmt::Result {
+        Ok(())
+    }
+
+    fn write_char(&mut self, _: char) -> std::fmt::Result {
+        Ok(())
+    }
+
+    fn write_fmt(&mut self, _: Arguments<'_>) -> std::fmt::Result {
+        Ok(())
+    }
+}
+
+fuzz_target!(|test_case: FuzzTestCase| {
+    test_case.execute(&mut NoopWrite::default(), true).unwrap()
 });

src/types.rs

@@ -1050,7 +1050,7 @@ pub enum AesVendorVersion {
 }
 
 /// AES variant used.
-#[derive(Copy, Clone, Debug)]
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
 #[cfg_attr(fuzzing, derive(arbitrary::Arbitrary))]
 #[repr(u8)]
 pub enum AesMode {

src/write.rs

@@ -225,7 +225,7 @@ mod sealed {
     }
 }
 
-#[derive(Copy, Clone, Debug)]
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
 pub(crate) enum EncryptWith<'k> {
     #[cfg(feature = "aes-crypto")]
     Aes {
@@ -254,7 +254,7 @@ impl<'a> arbitrary::Arbitrary<'a> for EncryptWith<'a> {
 }
 
 /// Metadata for a file to be written
-#[derive(Clone, Debug, Copy)]
+#[derive(Clone, Debug, Copy, Eq, PartialEq)]
 pub struct FileOptions<'k, T: FileOptionExtension> {
     pub(crate) compression_method: CompressionMethod,
     pub(crate) compression_level: Option<i64>,
@@ -272,7 +272,7 @@ pub type SimpleFileOptions = FileOptions<'static, ()>;
 /// Adds Extra Data and Central Extra Data. It does not implement copy.
 pub type FullFileOptions<'k> = FileOptions<'k, ExtendedFileOptions>;
 /// The Extension for Extra Data and Central Extra Data
-#[derive(Clone, Default)]
+#[derive(Clone, Default, Eq, PartialEq)]
 pub struct ExtendedFileOptions {
     extra_data: Arc<Vec<u8>>,
     central_extra_data: Arc<Vec<u8>>,