From 84aa6e8f11805378b6f0fabc820a8b3ad9519e34 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Sun, 5 May 2024 18:55:48 -0700 Subject: [PATCH 1/6] refactor: Make `ZipWriter::finish()` consume the `ZipWriter` --- src/write.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/write.rs b/src/write.rs index 0760365b6..4a26d9a49 100644 --- a/src/write.rs +++ b/src/write.rs @@ -1188,7 +1188,7 @@ impl ZipWriter { /// /// This will return the writer, but one should normally not append any data to the end of the file. /// Note that the zipfile will also be finished on drop. - pub fn finish(&mut self) -> ZipResult { + pub fn finish(mut self) -> ZipResult { let _central_start = self.finalize()?; let inner = mem::replace(&mut self.inner, Closed); Ok(inner.unwrap()) @@ -1440,14 +1440,13 @@ impl GenericZipWriter { feature = "deflate-zlib-ng", ))] { - return Ok(Box::new(move |bare| { + Ok(Box::new(move |bare| { GenericZipWriter::Deflater(DeflateEncoder::new( bare, Compression::new(level), )) - })); + })) } - unreachable!() } #[cfg(feature = "deflate64")] CompressionMethod::Deflate64 => Err(ZipError::UnsupportedArchive( From b59515bbd733d7f4c1f2a3120e18b8aa83e83ae5 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Sun, 5 May 2024 19:30:18 -0700 Subject: [PATCH 2/6] chore: Remove a drop that can no longer be explicit --- tests/zip_crypto.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/zip_crypto.rs b/tests/zip_crypto.rs index 9e91e1bd3..4c4cc8b29 100644 --- a/tests/zip_crypto.rs +++ b/tests/zip_crypto.rs @@ -49,7 +49,6 @@ fn encrypting_file() { .unwrap(); archive.write_all(b"test").unwrap(); archive.finish().unwrap(); - drop(archive); let mut archive = zip::ZipArchive::new(Cursor::new(&mut buf)).unwrap(); let mut file = archive.by_index_decrypt(0, b"password").unwrap(); let mut buf = Vec::new(); From e9d48b7333bd2eb11ac50c2ad4bb0175bb788845 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Sun, 5 May 2024 19:39:13 -0700 Subject: [PATCH 3/6] style: Remove unnecessary "mut"s in merge_archive benchmarks --- benches/merge_archive.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/benches/merge_archive.rs b/benches/merge_archive.rs index 709cd8a54..ff07e1a6c 100644 --- a/benches/merge_archive.rs +++ b/benches/merge_archive.rs @@ -59,7 +59,7 @@ fn merge_archive_stored(bench: &mut Bencher) { bench.iter(|| { let buf = Cursor::new(Vec::new()); let zip = ZipWriter::new(buf); - let mut zip = perform_merge(src.clone(), zip).unwrap(); + let zip = perform_merge(src.clone(), zip).unwrap(); let buf = zip.finish().unwrap().into_inner(); assert_eq!(buf.len(), len); }); @@ -75,7 +75,7 @@ fn merge_archive_compressed(bench: &mut Bencher) { bench.iter(|| { let buf = Cursor::new(Vec::new()); let zip = ZipWriter::new(buf); - let mut zip = perform_merge(src.clone(), zip).unwrap(); + let zip = perform_merge(src.clone(), zip).unwrap(); let buf = zip.finish().unwrap().into_inner(); assert_eq!(buf.len(), len); }); @@ -90,7 +90,7 @@ fn merge_archive_raw_copy_file_stored(bench: &mut Bencher) { bench.iter(|| { let buf = Cursor::new(Vec::new()); let zip = ZipWriter::new(buf); - let mut zip = perform_raw_copy_file(src.clone(), zip).unwrap(); + let zip = perform_raw_copy_file(src.clone(), zip).unwrap(); let buf = zip.finish().unwrap().into_inner(); assert_eq!(buf.len(), len); }); @@ -106,7 +106,7 @@ fn merge_archive_raw_copy_file_compressed(bench: &mut Bencher) { bench.iter(|| { let buf = Cursor::new(Vec::new()); let zip = ZipWriter::new(buf); - let mut zip = perform_raw_copy_file(src.clone(), zip).unwrap(); + let zip = perform_raw_copy_file(src.clone(), zip).unwrap(); let buf = zip.finish().unwrap().into_inner(); assert_eq!(buf.len(), len); }); From f2b55a1f596d6fa1104697c11820f61e62a8b07e Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Sun, 5 May 2024 20:09:23 -0700 Subject: [PATCH 4/6] chore: Update fuzz_write to use replace_with --- fuzz/Cargo.toml | 1 + fuzz/fuzz_targets/fuzz_write.rs | 42 ++++++++++++++++----------------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 2f94e945f..dc113f96b 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -11,6 +11,7 @@ cargo-fuzz = true [dependencies] libfuzzer-sys = "0.4" arbitrary = { version = "1.3.0", features = ["derive"] } +replace_with = "0.1.7" [dependencies.zip] path = ".." diff --git a/fuzz/fuzz_targets/fuzz_write.rs b/fuzz/fuzz_targets/fuzz_write.rs index 3f03edb56..046eb81c2 100644 --- a/fuzz/fuzz_targets/fuzz_write.rs +++ b/fuzz/fuzz_targets/fuzz_write.rs @@ -2,7 +2,7 @@ use arbitrary::Arbitrary; use libfuzzer_sys::fuzz_target; -use std::cell::RefCell; +use replace_with::replace_with_or_abort; use std::io::{Cursor, Read, Seek, Write}; use std::path::PathBuf; @@ -37,7 +37,7 @@ pub struct FuzzTestCase { } fn do_operation( - writer: &mut RefCell>, + writer: &mut zip::ZipWriter, operation: &FileOperation, abort: bool, flush_on_finish_file: bool, @@ -45,9 +45,7 @@ fn do_operation( where T: Read + Write + Seek, { - writer - .borrow_mut() - .set_flush_on_finish_file(flush_on_finish_file); + writer.set_flush_on_finish_file(flush_on_finish_file); let path = &operation.path; match &operation.basic { BasicFileOperation::WriteNormalFile { @@ -60,44 +58,44 @@ where if uncompressed_size >= u32::MAX as usize { options = options.large_file(true); } - writer.borrow_mut().start_file_from_path(path, options)?; + writer.start_file_from_path(path, options)?; for chunk in contents { - writer.borrow_mut().write_all(chunk.as_slice())?; + writer.write_all(chunk.as_slice())?; } } BasicFileOperation::WriteDirectory(options) => { - writer.borrow_mut().add_directory_from_path(path, options.to_owned())?; + writer.add_directory_from_path(path, options.to_owned())?; } BasicFileOperation::WriteSymlinkWithTarget { target, options } => { - writer - .borrow_mut() - .add_symlink_from_path(&path, target, options.to_owned())?; + writer.add_symlink_from_path(&path, target, options.to_owned())?; } BasicFileOperation::ShallowCopy(base) => { do_operation(writer, &base, false, flush_on_finish_file)?; - writer.borrow_mut().shallow_copy_file_from_path(&base.path, &path)?; + writer.shallow_copy_file_from_path(&base.path, &path)?; } BasicFileOperation::DeepCopy(base) => { do_operation(writer, &base, false, flush_on_finish_file)?; - writer.borrow_mut().deep_copy_file_from_path(&base.path, &path)?; + writer.deep_copy_file_from_path(&base.path, &path)?; } } if abort { - writer.borrow_mut().abort_file().unwrap(); + writer.abort_file().unwrap(); } if operation.reopen { - let old_comment = writer.borrow().get_raw_comment().to_owned(); - let new_writer = - zip::ZipWriter::new_append(writer.borrow_mut().finish().unwrap()).unwrap(); - assert_eq!(&old_comment, new_writer.get_raw_comment()); - *writer = new_writer.into(); + let old_comment = writer.get_raw_comment().to_owned(); + replace_with_or_abort(writer, |old_writer: zip::ZipWriter| { + let new_writer = + zip::ZipWriter::new_append(old_writer.finish().unwrap()).unwrap(); + assert_eq!(&old_comment, new_writer.get_raw_comment()); + new_writer + }); } Ok(()) } fuzz_target!(|test_case: FuzzTestCase| { - let mut writer = RefCell::new(zip::ZipWriter::new(Cursor::new(Vec::new()))); - writer.borrow_mut().set_raw_comment(test_case.comment); + let mut writer = zip::ZipWriter::new(Cursor::new(Vec::new())); + writer.set_raw_comment(test_case.comment); for (operation, abort) in test_case.operations { let _ = do_operation( &mut writer, @@ -106,5 +104,5 @@ fuzz_target!(|test_case: FuzzTestCase| { test_case.flush_on_finish_file, ); } - let _ = zip::ZipArchive::new(writer.borrow_mut().finish().unwrap()); + let _ = zip::ZipArchive::new(writer.finish().unwrap()); }); From 0011370fdcd10de8e9424992fb3fd39452c40dab Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Sun, 5 May 2024 20:17:12 -0700 Subject: [PATCH 5/6] chore: Use panic! rather than abort to ensure the fuzz harness can process the failure --- fuzz/fuzz_targets/fuzz_write.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/fuzz/fuzz_targets/fuzz_write.rs b/fuzz/fuzz_targets/fuzz_write.rs index 046eb81c2..44493f2d3 100644 --- a/fuzz/fuzz_targets/fuzz_write.rs +++ b/fuzz/fuzz_targets/fuzz_write.rs @@ -2,7 +2,7 @@ use arbitrary::Arbitrary; use libfuzzer_sys::fuzz_target; -use replace_with::replace_with_or_abort; +use replace_with::replace_with; use std::io::{Cursor, Read, Seek, Write}; use std::path::PathBuf; @@ -83,7 +83,9 @@ where } if operation.reopen { let old_comment = writer.get_raw_comment().to_owned(); - replace_with_or_abort(writer, |old_writer: zip::ZipWriter| { + replace_with(writer, || { + panic!("Failed to reopen writer"); + }, |old_writer: zip::ZipWriter| { let new_writer = zip::ZipWriter::new_append(old_writer.finish().unwrap()).unwrap(); assert_eq!(&old_comment, new_writer.get_raw_comment()); From 30ef662aa28661754115dbd2e755fb9166465d33 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Sun, 5 May 2024 20:19:09 -0700 Subject: [PATCH 6/6] Revert "chore: Use panic! rather than abort to ensure the fuzz harness can process the failure" This reverts commit 0011370fdcd10de8e9424992fb3fd39452c40dab. --- fuzz/fuzz_targets/fuzz_write.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/fuzz/fuzz_targets/fuzz_write.rs b/fuzz/fuzz_targets/fuzz_write.rs index 44493f2d3..046eb81c2 100644 --- a/fuzz/fuzz_targets/fuzz_write.rs +++ b/fuzz/fuzz_targets/fuzz_write.rs @@ -2,7 +2,7 @@ use arbitrary::Arbitrary; use libfuzzer_sys::fuzz_target; -use replace_with::replace_with; +use replace_with::replace_with_or_abort; use std::io::{Cursor, Read, Seek, Write}; use std::path::PathBuf; @@ -83,9 +83,7 @@ where } if operation.reopen { let old_comment = writer.get_raw_comment().to_owned(); - replace_with(writer, || { - panic!("Failed to reopen writer"); - }, |old_writer: zip::ZipWriter| { + replace_with_or_abort(writer, |old_writer: zip::ZipWriter| { let new_writer = zip::ZipWriter::new_append(old_writer.finish().unwrap()).unwrap(); assert_eq!(&old_comment, new_writer.get_raw_comment());