From eaee24f414ac8e571bf29095b2cc420e9f2025c9 Mon Sep 17 00:00:00 2001 From: Chris Hennick Date: Fri, 19 Jul 2024 16:01:16 -0700 Subject: [PATCH 1/7] ci(fuzz): Apply read_zipfile_from_stream to fuzz_write output when possible --- fuzz/fuzz_targets/fuzz_write.rs | 23 ++++++++++++++++++++++- src/write.rs | 8 ++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/fuzz/fuzz_targets/fuzz_write.rs b/fuzz/fuzz_targets/fuzz_write.rs index fa281a4ff..9b0ac30be 100755 --- a/fuzz/fuzz_targets/fuzz_write.rs +++ b/fuzz/fuzz_targets/fuzz_write.rs @@ -8,6 +8,7 @@ use std::borrow::Cow; use std::io::{Cursor, Read, Seek, Write}; use std::path::PathBuf; use tikv_jemallocator::Jemalloc; +use zip::read::read_zipfile_from_stream; use zip::unstable::path_to_string; #[global_allocator] @@ -59,6 +60,19 @@ impl<'k> FileOperation<'k> { _ => Some(Cow::Borrowed(&self.path)), } } + + fn is_streamable(&self) -> bool { + match &self.basic { + BasicFileOperation::WriteNormalFile {options, ..} => !options.has_encryption(), + BasicFileOperation::WriteDirectory(options) => !options.has_encryption(), + BasicFileOperation::WriteSymlinkWithTarget {options, ..} => !options.has_encryption(), + BasicFileOperation::ShallowCopy(base) => base.is_streamable(), + BasicFileOperation::DeepCopy(base) => base.is_streamable(), + BasicFileOperation::MergeWithOtherFile {operations, ..} => + operations.iter().all(|(op, _)| op.is_streamable()), + _ => true + } + } } impl<'k> Debug for FileOperation<'k> { @@ -295,6 +309,7 @@ where fuzz_target!(|test_case: FuzzTestCase| { let mut files_added = 0; let mut writer = zip::ZipWriter::new(Cursor::new(Vec::new())); + let mut streamable = true; let mut final_reopen = false; if let Some((last_op, _)) = test_case.operations.last() { if last_op.reopen != ReopenOption::ViaFinishIntoReadable { @@ -304,6 +319,9 @@ fuzz_target!(|test_case: FuzzTestCase| { #[allow(unknown_lints)] #[allow(boxed_slice_into_iter)] for (operation, abort) in test_case.operations.into_iter() { + if streamable { + streamable = operation.is_streamable(); + } let _ = do_operation( &mut writer, &operation, @@ -312,7 +330,10 @@ fuzz_target!(|test_case: FuzzTestCase| { &mut files_added, ); } - if final_reopen { + if streamable { + let mut stream = writer.finish().unwrap(); + while read_zipfile_from_stream(&mut stream).unwrap().is_some() {} + } else if final_reopen { let _ = writer.finish_into_readable().unwrap(); } }); diff --git a/src/write.rs b/src/write.rs index ae179fb3b..f516126c4 100644 --- a/src/write.rs +++ b/src/write.rs @@ -267,6 +267,14 @@ pub struct FileOptions<'k, T: FileOptionExtension> { #[cfg(feature = "deflate-zopfli")] pub(super) zopfli_buffer_size: Option, } + +impl<'k, T: FileOptionExtension> FileOptions<'k, T> { + /// True if this instance will encrypt the file contents or symlink target. + pub fn has_encryption(&self) -> bool { + self.encrypt_with.is_some() + } +} + /// Simple File Options. Can be copied and good for simple writing zip files pub type SimpleFileOptions = FileOptions<'static, ()>; /// Adds Extra Data and Central Extra Data. It does not implement copy. From ce38fac6188166acd478fbdd2e7506610cb51977 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Sat, 27 Jul 2024 12:09:08 -0700 Subject: [PATCH 2/7] style: Fix indentation after merge --- fuzz/fuzz_targets/fuzz_write.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fuzz/fuzz_targets/fuzz_write.rs b/fuzz/fuzz_targets/fuzz_write.rs index 1e3c8a029..34dce7d13 100755 --- a/fuzz/fuzz_targets/fuzz_write.rs +++ b/fuzz/fuzz_targets/fuzz_write.rs @@ -240,8 +240,8 @@ impl <'k> FuzzTestCase<'k> { let mut files_added = 0; let mut writer = zip::ZipWriter::new(Cursor::new(Vec::new())); let mut streamable = true; - let mut final_reopen = false; - if let Some((last_op, _)) = self.operations.last() { + let mut final_reopen = false; + if let Some((last_op, _)) = self.operations.last() { if last_op.reopen != ReopenOption::ViaFinishIntoReadable { final_reopen = true; } From 2a8e41b97a1eae38fc493047d1f96b41b033d67c Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Sat, 27 Jul 2024 12:13:19 -0700 Subject: [PATCH 3/7] ci(fuzz): Fix missing part of dump --- fuzz/fuzz_targets/fuzz_write.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/fuzz/fuzz_targets/fuzz_write.rs b/fuzz/fuzz_targets/fuzz_write.rs index 34dce7d13..a27196149 100755 --- a/fuzz/fuzz_targets/fuzz_write.rs +++ b/fuzz/fuzz_targets/fuzz_write.rs @@ -263,9 +263,13 @@ impl <'k> FuzzTestCase<'k> { ); } if streamable { - let mut stream = writer.finish().unwrap(); - while read_zipfile_from_stream(&mut stream).unwrap().is_some() {} - } else if final_reopen {writeln!(stringifier, "let _ = writer.finish_into_readable()?;") + writeln!(stringifier, "let mut stream = writer.finish().unwrap();\n\ + while read_zipfile_from_stream(&mut stream).unwrap().is_some() {{}}") + .map_err(|_| ZipError::InvalidArchive(""))?; + let mut stream = writer.finish().unwrap(); + while read_zipfile_from_stream(&mut stream).unwrap().is_some() {} + } else if final_reopen { + writeln!(stringifier, "let _ = writer.finish_into_readable()?;") .map_err(|_| ZipError::InvalidArchive(""))?; let _ = writer.finish_into_readable()?; } From c3aad8b7a0fa1f67f805dcd5507a91739ba3db45 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Sat, 27 Jul 2024 12:36:44 -0700 Subject: [PATCH 4/7] test(fuzz): No longer need panic_on_error option, because we print fallible operations before running them --- fuzz/fuzz_targets/fuzz_write.rs | 58 ++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/fuzz/fuzz_targets/fuzz_write.rs b/fuzz/fuzz_targets/fuzz_write.rs index a27196149..d0e6216ba 100755 --- a/fuzz/fuzz_targets/fuzz_write.rs +++ b/fuzz/fuzz_targets/fuzz_write.rs @@ -105,9 +105,9 @@ fn do_operation<'k>( abort: bool, flush_on_finish_file: bool, files_added: &mut usize, - stringifier: &mut impl Write, - panic_on_error: bool + stringifier: &mut impl Write ) -> Result<(), Box> { + writeln!(stringifier, "writer.set_flush_on_finish_file({});", flush_on_finish_file)?; writer.set_flush_on_finish_file(flush_on_finish_file); let mut path = Cow::Borrowed(&operation.path); match &operation.basic { @@ -146,7 +146,7 @@ fn do_operation<'k>( return Ok(()); }; deduplicate_paths(&mut path, &base_path); - do_operation(writer, &base, false, flush_on_finish_file, files_added, stringifier, panic_on_error)?; + do_operation(writer, &base, false, flush_on_finish_file, files_added, stringifier)?; writeln!(stringifier, "writer.shallow_copy_file_from_path({:?}, {:?});", base_path, path)?; writer.shallow_copy_file_from_path(&*base_path, &*path)?; *files_added += 1; @@ -156,7 +156,7 @@ fn do_operation<'k>( return Ok(()); }; deduplicate_paths(&mut path, &base_path); - do_operation(writer, &base, false, flush_on_finish_file, files_added, stringifier, panic_on_error)?; + do_operation(writer, &base, false, flush_on_finish_file, files_added, stringifier)?; writeln!(stringifier, "writer.deep_copy_file_from_path({:?}, {:?});", base_path, path)?; writer.deep_copy_file_from_path(&*base_path, &*path)?; *files_added += 1; @@ -173,8 +173,7 @@ fn do_operation<'k>( *abort, false, &mut inner_files_added, - stringifier, - panic_on_error + stringifier ); }); writeln!(stringifier, "writer\n}};\nwriter.merge_archive(sub_writer.finish_into_readable()?)?;")?; @@ -191,6 +190,7 @@ fn do_operation<'k>( writer.abort_file()?; *files_added -= 1; } + let mut abort = false; // If a comment is set, we finish the archive, reopen it for append and then set a shorter // 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 @@ -200,20 +200,20 @@ fn do_operation<'k>( return Ok(()) }, ReopenOption::ViaFinish => { + writeln!(stringifier, "let old_comment = writer.get_raw_comment().to_owned();")?; let old_comment = writer.get_raw_comment().to_owned(); 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") - } + abort = true; zip::ZipWriter::new(Cursor::new(Vec::new())) }) }); - if panic_on_error { - assert!(writer.get_raw_comment().starts_with(&old_comment)); + writeln!(stringifier, "assert!(writer.get_raw_comment().starts_with(&old_comment));")?; + if !writer.get_raw_comment().starts_with(&old_comment) { + return Err("Comment mismatch".into()); } } ReopenOption::ViaFinishIntoReadable => { @@ -223,20 +223,25 @@ fn do_operation<'k>( (|| -> ZipResult>>> { zip::ZipWriter::new_append(old_writer.finish()?) })().unwrap_or_else(|_| { - if panic_on_error { - panic!("Failed to create new ZipWriter") - } + abort = true; zip::ZipWriter::new(Cursor::new(Vec::new())) }) }); - assert!(writer.get_raw_comment().starts_with(&old_comment)); + writeln!(stringifier, "assert!(writer.get_raw_comment().starts_with(&old_comment));")?; + if !writer.get_raw_comment().starts_with(&old_comment) { + return Err("Comment mismatch".into()); + } } } - Ok(()) + if abort { + Err("Failed to reopen writer".into()) + } else { + Ok(()) + } } impl <'k> FuzzTestCase<'k> { - fn execute(&self, stringifier: &mut impl Write, panic_on_error: bool) -> ZipResult<()> { + fn execute(&self, stringifier: &mut impl Write) -> ZipResult<()> { let mut files_added = 0; let mut writer = zip::ZipWriter::new(Cursor::new(Vec::new())); let mut streamable = true; @@ -257,17 +262,16 @@ impl <'k> FuzzTestCase<'k> { &operation, *abort, self.flush_on_finish_file, - &mut files_added, - stringifier, - panic_on_error - ); + &mut files_added, + stringifier + ); } if streamable { - writeln!(stringifier, "let mut stream = writer.finish().unwrap();\n\ - while read_zipfile_from_stream(&mut stream).unwrap().is_some() {{}}") + writeln!(stringifier, "let mut stream = writer.finish()?;\n\ + while read_zipfile_from_stream(&mut stream)?.is_some() {{}}") .map_err(|_| ZipError::InvalidArchive(""))?; - let mut stream = writer.finish().unwrap(); - while read_zipfile_from_stream(&mut stream).unwrap().is_some() {} + let mut stream = writer.finish()?; + while read_zipfile_from_stream(&mut stream)?.is_some() {} } else if final_reopen { writeln!(stringifier, "let _ = writer.finish_into_readable()?;") .map_err(|_| ZipError::InvalidArchive(""))?; @@ -280,7 +284,7 @@ impl <'k> FuzzTestCase<'k> { 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); + let _ = self.execute(f); Ok(()) } } @@ -303,5 +307,5 @@ impl Write for NoopWrite { } fuzz_target!(|test_case: FuzzTestCase| { - test_case.execute(&mut NoopWrite::default(), true).unwrap() + test_case.execute(&mut NoopWrite::default()).unwrap() }); From c43c4b4e4e18e70d82192bc7cb72961b25004b52 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Sat, 27 Jul 2024 12:49:59 -0700 Subject: [PATCH 5/7] test(fuzz): Fix missing error messages --- fuzz/fuzz_targets/fuzz_write.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fuzz/fuzz_targets/fuzz_write.rs b/fuzz/fuzz_targets/fuzz_write.rs index d0e6216ba..c70251c8b 100755 --- a/fuzz/fuzz_targets/fuzz_write.rs +++ b/fuzz/fuzz_targets/fuzz_write.rs @@ -269,12 +269,12 @@ impl <'k> FuzzTestCase<'k> { if streamable { writeln!(stringifier, "let mut stream = writer.finish()?;\n\ while read_zipfile_from_stream(&mut stream)?.is_some() {{}}") - .map_err(|_| ZipError::InvalidArchive(""))?; + .map_err(|_| ZipError::InvalidArchive("Failed to read from stream"))?; let mut stream = writer.finish()?; while read_zipfile_from_stream(&mut stream)?.is_some() {} } else if final_reopen { writeln!(stringifier, "let _ = writer.finish_into_readable()?;") - .map_err(|_| ZipError::InvalidArchive(""))?; + .map_err(|_| ZipError::InvalidArchive("Failed to finish_into_readable"))?; let _ = writer.finish_into_readable()?; } Ok(()) From 1beb72e57ed7e3b0a1b46900fb1648d17f9347b2 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Sat, 27 Jul 2024 12:52:41 -0700 Subject: [PATCH 6/7] test(fuzz): Bug fix? Rewind archive cursor before reading from it --- fuzz/fuzz_targets/fuzz_write.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fuzz/fuzz_targets/fuzz_write.rs b/fuzz/fuzz_targets/fuzz_write.rs index c70251c8b..bee88b6bd 100755 --- a/fuzz/fuzz_targets/fuzz_write.rs +++ b/fuzz/fuzz_targets/fuzz_write.rs @@ -6,7 +6,7 @@ use libfuzzer_sys::fuzz_target; use replace_with::replace_with_or_abort; use std::borrow::Cow; use std::fmt::{Arguments, Formatter, Write}; -use std::io::Cursor; +use std::io::{Cursor, Seek}; use std::io::Write as IoWrite; use std::path::PathBuf; use tikv_jemallocator::Jemalloc; @@ -268,9 +268,11 @@ impl <'k> FuzzTestCase<'k> { } if streamable { writeln!(stringifier, "let mut stream = writer.finish()?;\n\ + stream.rewind()?;\n\ while read_zipfile_from_stream(&mut stream)?.is_some() {{}}") .map_err(|_| ZipError::InvalidArchive("Failed to read from stream"))?; let mut stream = writer.finish()?; + stream.rewind()?; while read_zipfile_from_stream(&mut stream)?.is_some() {} } else if final_reopen { writeln!(stringifier, "let _ = writer.finish_into_readable()?;") From 1bdfc25802b6590925cb6424d68b0356b2e400fa Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Sun, 28 Jul 2024 13:51:49 -0700 Subject: [PATCH 7/7] chore: Fix merge --- fuzz/fuzz_targets/fuzz_write.rs | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/fuzz/fuzz_targets/fuzz_write.rs b/fuzz/fuzz_targets/fuzz_write.rs index a8909e9c8..c8863e1f5 100755 --- a/fuzz/fuzz_targets/fuzz_write.rs +++ b/fuzz/fuzz_targets/fuzz_write.rs @@ -146,7 +146,7 @@ fn do_operation<'k>( return Ok(()); }; deduplicate_paths(&mut path, &base_path); - do_operation(writer, &base, false, flush_on_finish_file, files_added, stringifier)?; + do_operation(writer, *base, false, flush_on_finish_file, files_added, stringifier)?; writeln!(stringifier, "writer.shallow_copy_file_from_path({:?}, {:?});", base_path, path)?; writer.shallow_copy_file_from_path(&*base_path, &*path)?; *files_added += 1; @@ -156,7 +156,7 @@ fn do_operation<'k>( return Ok(()); }; deduplicate_paths(&mut path, &base_path); - do_operation(writer, &base, false, flush_on_finish_file, files_added, stringifier)?; + do_operation(writer, *base, false, flush_on_finish_file, files_added, stringifier)?; writeln!(stringifier, "writer.deep_copy_file_from_path({:?}, {:?});", base_path, path)?; writer.deep_copy_file_from_path(&*base_path, path)?; *files_added += 1; @@ -251,7 +251,9 @@ fn do_operation<'k>( } impl <'k> FuzzTestCase<'k> { - fn execute(self, stringifier: &mut impl Write, panic_on_error: bool) -> ZipResult<()> { + fn execute(self, stringifier: &mut impl Write) -> ZipResult<()> { + let junk_len = self.initial_junk.len(); + let mut streamable = true; let mut initial_junk = Cursor::new(self.initial_junk.into_vec()); initial_junk.seek(SeekFrom::End(0))?; let mut writer = zip::ZipWriter::new(initial_junk); @@ -264,25 +266,26 @@ impl <'k> FuzzTestCase<'k> { } #[allow(unknown_lints)] #[allow(boxed_slice_into_iter)] - for (operation, abort) in self.operations.iter() { + for (operation, abort) in self.operations.into_vec().into_iter() { if streamable { - streamable = operation.is_streamable(); + streamable = operation.is_streamable(); + } + let _ = do_operation( + &mut writer, + operation, + abort, + self.flush_on_finish_file, + &mut files_added, + stringifier + ); } - let _ = do_operation( - &mut writer, - &operation, - *abort, - self.flush_on_finish_file, - &mut files_added, - stringifier - ); if streamable { writeln!(stringifier, "let mut stream = writer.finish()?;\n\ stream.rewind()?;\n\ while read_zipfile_from_stream(&mut stream)?.is_some() {{}}") .map_err(|_| ZipError::InvalidArchive("Failed to read from stream"))?; let mut stream = writer.finish()?; - stream.seek(SeekFrom::Start(initial_junk.len()))?; + stream.seek(SeekFrom::Start(junk_len as u64))?; while read_zipfile_from_stream(&mut stream)?.is_some() {} } else if final_reopen { writeln!(stringifier, "let _ = writer.finish_into_readable()?;") @@ -302,7 +305,7 @@ impl <'k> Debug for FuzzTestCase<'k> { initial_junk.seek(SeekFrom::End(0))?;\n\ let mut writer = ZipWriter::new(initial_junk);", &self.initial_junk)?; } - let _ = self.clone().execute(f, false); + let _ = self.clone().execute(f); Ok(()) } }