From a29b860395518350b700f6aea59d3a2db4b2aa62 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Fri, 26 Jul 2024 14:42:03 -0700 Subject: [PATCH] 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 | 265 ++++++++++++++------------------ src/types.rs | 2 +- src/write.rs | 6 +- 3 files changed, 119 insertions(+), 154 deletions(-) diff --git a/fuzz/fuzz_targets/fuzz_write.rs b/fuzz/fuzz_targets/fuzz_write.rs index fa281a4ff..20f31e8a9 100755 --- a/fuzz/fuzz_targets/fuzz_write.rs +++ b/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>), DeepCopy(Box>), @@ -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::>()?; - 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::>()?; - f.write_str("writer\n") - } -} - fn deduplicate_paths(copy: &mut Cow, 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, original: &PathBuf) { } } -fn do_operation<'k, T>( - writer: &mut zip::ZipWriter, +fn do_operation<'k>( + writer: &mut zip::ZipWriter>>, operation: &FileOperation<'k>, abort: bool, flush_on_finish_file: bool, files_added: &mut usize, -) -> Result<(), Box> -where - T: Read + Write + Seek, -{ + stringifier: &mut impl Write, + panic_on_error: bool +) -> Result<(), Box> { 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| { - zip::ZipWriter::new_append(old_writer.finish().unwrap()).unwrap() + writeln!(stringifier, "let mut writer = ZipWriter::new_append(writer.finish()?)?;")?; + replace_with_or_abort(writer, |old_writer: zip::ZipWriter>>| { + (|| -> ZipResult>>> { + 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)); + 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| { - zip::ZipWriter::new_append(old_writer.finish_into_readable().unwrap().into_inner()) - .unwrap() + writeln!(stringifier, "let mut writer = ZipWriter::new_append(writer.finish()?)?;")?; + replace_with_or_abort(writer, |old_writer| { + (|| -> ZipResult>>> { + 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| { - 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 last_op.reopen != ReopenOption::ViaFinishIntoReadable { - final_reopen = true; +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, _)) = self.operations.last() { + if last_op.reopen != ReopenOption::ViaFinishIntoReadable { + final_reopen = true; + } + } + #[allow(unknown_lints)] + #[allow(boxed_slice_into_iter)] + for (operation, abort) in self.operations.iter() { + let _ = do_operation( + &mut writer, + &operation, + *abort, + self.flush_on_finish_file, + &mut files_added, + stringifier, + panic_on_error + ); } + if final_reopen { + writeln!(stringifier, "let _ = writer.finish_into_readable()?;") + .map_err(|_| ZipError::InvalidArchive(""))?; + let _ = writer.finish_into_readable()?; + } + Ok(()) } - #[allow(unknown_lints)] - #[allow(boxed_slice_into_iter)] - for (operation, abort) in test_case.operations.into_iter() { - let _ = do_operation( - &mut writer, - &operation, - *abort, - test_case.flush_on_finish_file, - &mut files_added, - ); +} + +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(()) } - if final_reopen { - let _ = writer.finish_into_readable().unwrap(); +} + +#[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() }); diff --git a/src/types.rs b/src/types.rs index c1b7adcec..31cb31f39 100644 --- a/src/types.rs +++ b/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 { diff --git a/src/write.rs b/src/write.rs index 2ecfea8f1..5d977cd9d 100644 --- a/src/write.rs +++ b/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, @@ -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>, central_extra_data: Arc>,