From 26bf2f0cfe5b1ba77e93b085908c409751440822 Mon Sep 17 00:00:00 2001 From: bogay Date: Mon, 20 Jan 2025 19:43:17 +0800 Subject: [PATCH 1/4] ci: checks format for fuzz_read and fuzz_write --- .github/workflows/ci.yaml | 10 +++ fuzz_write/src/main.rs | 162 ++++++++++++++++++++++++++++---------- 2 files changed, 129 insertions(+), 43 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index fe7735702..2c8b062f9 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -69,6 +69,16 @@ jobs: with: command: fmt args: --all -- --check + - name: fmt fuzz_read + uses: actions-rs/cargo@v1 + with: + command: fmt + args: --manifest-path ${{ github.workspace }}/fuzz_read/Cargo.toml -- --check + - name: fmt fuzz_write + uses: actions-rs/cargo@v1 + with: + command: fmt + args: --manifest-path ${{ github.workspace }}/fuzz_write/Cargo.toml -- --check check_minimal_versions: if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name != github.event.pull_request.base.repo.full_name diff --git a/fuzz_write/src/main.rs b/fuzz_write/src/main.rs index 5e6fe6141..c3fd09901 100644 --- a/fuzz_write/src/main.rs +++ b/fuzz_write/src/main.rs @@ -1,10 +1,10 @@ use afl::fuzz; use arbitrary::{Arbitrary, Unstructured}; -use core::fmt::{Debug}; +use core::fmt::Debug; use replace_with::replace_with_or_abort; use std::fmt::{Arguments, Formatter, Write}; -use std::io::{Cursor, Seek, SeekFrom}; use std::io::Write as IoWrite; +use std::io::{Cursor, Seek, SeekFrom}; use std::path::PathBuf; use tikv_jemallocator::Jemalloc; use zip::result::{ZipError, ZipResult}; @@ -91,22 +91,36 @@ fn do_operation<'k>( flush_on_finish_file: bool, files_added: &mut usize, stringifier: &mut impl Write, - panic_on_error: bool + panic_on_error: bool, ) -> Result<(), Box> { writer.set_flush_on_finish_file(flush_on_finish_file); - let FileOperation { basic, mut path, reopen} = operation; + let FileOperation { + basic, + mut path, + reopen, + } = operation; match basic { BasicFileOperation::WriteNormalFile { - contents, mut options, .. + contents, + mut options, + .. } => { let uncompressed_size = contents.iter().map(|chunk| chunk.len()).sum::(); 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)?; + writeln!( + stringifier, + "writer.start_file_from_path({:?}, Default::default())?;", + path + )?; } else { - writeln!(stringifier, "writer.start_file_from_path({:?}, {:?})?;", path, options)?; + writeln!( + stringifier, + "writer.start_file_from_path({:?}, {:?})?;", + path, options + )?; } writer.start_file_from_path(&*path, options)?; for chunk in contents.iter() { @@ -116,12 +130,20 @@ fn do_operation<'k>( *files_added += 1; } BasicFileOperation::WriteDirectory(options) => { - writeln!(stringifier, "writer.add_directory_from_path(&{:?}, {:?})?;", path, 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)?; + writeln!( + stringifier, + "writer.add_symlink_from_path(&{:?}, {:?}, {:?});", + path, target, options + )?; writer.add_symlink_from_path(&*path, target, options.to_owned())?; *files_added += 1; } @@ -130,8 +152,20 @@ 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)?; - writeln!(stringifier, "writer.shallow_copy_file_from_path({:?}, {:?});", base_path, path)?; + 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; } @@ -140,38 +174,65 @@ 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)?; - writeln!(stringifier, "writer.deep_copy_file_from_path({:?}, {:?});", base_path, path)?; + 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, initial_junk } => { + BasicFileOperation::MergeWithOtherFile { + operations, + initial_junk, + } => { if initial_junk.is_empty() { - writeln!(stringifier, "let sub_writer = {{\n\ - let mut writer = ZipWriter::new(Cursor::new(Vec::new()));")?; + writeln!( + stringifier, + "let sub_writer = {{\n\ + let mut writer = ZipWriter::new(Cursor::new(Vec::new()));" + )?; } else { - writeln!(stringifier, - "let sub_writer = {{\n\ + writeln!( + stringifier, + "let sub_writer = {{\n\ let mut initial_junk = Cursor::new(vec!{:?});\n\ initial_junk.seek(SeekFrom::End(0))?; - let mut writer = ZipWriter::new(initial_junk);", initial_junk)?; + let mut writer = ZipWriter::new(initial_junk);", + initial_junk + )?; } let mut initial_junk = Cursor::new(initial_junk.into_vec()); initial_junk.seek(SeekFrom::End(0))?; let mut other_writer = zip::ZipWriter::new(initial_junk); let mut inner_files_added = 0; - operations.into_vec().into_iter().for_each(|(operation, abort)| { - let _ = do_operation( - &mut other_writer, - operation, - abort, - false, - &mut inner_files_added, - stringifier, - panic_on_error - ); - }); - writeln!(stringifier, "writer\n}};\nwriter.merge_archive(sub_writer.finish_into_readable()?)?;")?; + operations + .into_vec() + .into_iter() + .for_each(|(operation, abort)| { + let _ = do_operation( + &mut other_writer, + operation, + 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; } @@ -191,15 +252,19 @@ fn do_operation<'k>( match reopen { ReopenOption::DoNotReopen => { writeln!(stringifier, "writer")?; - return Ok(()) - }, + return Ok(()); + } ReopenOption::ViaFinish => { let old_comment = writer.get_raw_comment().to_owned(); - writeln!(stringifier, "let mut writer = ZipWriter::new_append(writer.finish()?)?;")?; + 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(|_| { + })() + .unwrap_or_else(|_| { if panic_on_error { panic!("Failed to create new ZipWriter") } @@ -212,11 +277,15 @@ fn do_operation<'k>( } ReopenOption::ViaFinishIntoReadable => { let old_comment = writer.get_raw_comment().to_owned(); - writeln!(stringifier, "let mut writer = ZipWriter::new_append(writer.finish()?)?;")?; + 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(|_| { + })() + .unwrap_or_else(|_| { if panic_on_error { panic!("Failed to create new ZipWriter") } @@ -229,7 +298,7 @@ fn do_operation<'k>( Ok(()) } -impl <'k> FuzzTestCase<'k> { +impl<'k> FuzzTestCase<'k> { fn execute(self, stringifier: &mut impl Write, panic_on_error: bool) -> ZipResult<()> { let mut initial_junk = Cursor::new(self.initial_junk.into_vec()); initial_junk.seek(SeekFrom::End(0))?; @@ -251,7 +320,7 @@ impl <'k> FuzzTestCase<'k> { self.flush_on_finish_file, &mut files_added, stringifier, - panic_on_error + panic_on_error, ); } if final_reopen { @@ -263,14 +332,21 @@ impl <'k> FuzzTestCase<'k> { } } -impl <'k> Debug for FuzzTestCase<'k> { +impl<'k> Debug for FuzzTestCase<'k> { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { if self.initial_junk.is_empty() { - writeln!(f, "let mut writer = ZipWriter::new(Cursor::new(Vec::new()));")?; + writeln!( + f, + "let mut writer = ZipWriter::new(Cursor::new(Vec::new()));" + )?; } else { - writeln!(f, "let mut initial_junk = Cursor::new(vec!{:?});\n\ + writeln!( + f, + "let mut initial_junk = Cursor::new(vec!{:?});\n\ initial_junk.seek(SeekFrom::End(0))?;\n\ - let mut writer = ZipWriter::new(initial_junk);", &self.initial_junk)?; + let mut writer = ZipWriter::new(initial_junk);", + &self.initial_junk + )?; } let _ = self.clone().execute(f, false); Ok(()) From 4cebebaa327088eb72902bcdfe7b9fe4774cb3ff Mon Sep 17 00:00:00 2001 From: bogay Date: Fri, 24 Jan 2025 11:39:32 +0800 Subject: [PATCH 2/4] style: simplify checks for empty extra data fields which makes current nightly clippy fail. --- src/write.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/write.rs b/src/write.rs index 96b72a2f3..1d516f5a0 100644 --- a/src/write.rs +++ b/src/write.rs @@ -536,10 +536,10 @@ impl FileOptions<'_, ExtendedFileOptions> { /// Removes the extra data fields. #[must_use] pub fn clear_extra_data(mut self) -> Self { - if self.extended_options.extra_data.len() > 0 { + if !self.extended_options.extra_data.is_empty() { self.extended_options.extra_data = Arc::new(vec![]); } - if self.extended_options.central_extra_data.len() > 0 { + if !self.extended_options.central_extra_data.is_empty() { self.extended_options.central_extra_data = Arc::new(vec![]); } self From 4dd996d065b393f4222185719583d94eeb955ec0 Mon Sep 17 00:00:00 2001 From: bogay Date: Fri, 24 Jan 2025 11:53:58 +0800 Subject: [PATCH 3/4] ci: add clippy checks for fuzz_read and fuzz_write it seems that they depends on cargo-afl, so we need to use `cargo afl clippy` instead of `cargo clippy`. --- .github/workflows/ci.yaml | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 2c8b062f9..fb2996bf1 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -136,6 +136,7 @@ jobs: profile: minimal toolchain: nightly override: true + components: clippy - name: Install afl uses: actions-rs/cargo@v1 with: @@ -146,6 +147,11 @@ jobs: with: command: afl args: system-config + - name: clippy + uses: actions-rs/cargo@v1 + with: + command: afl + args: clippy --all-features --manifest-path ${{ github.workspace }}/fuzz_read/Cargo.toml -- -D warnings - name: compile fuzz uses: actions-rs/cargo@v1 with: @@ -205,6 +211,7 @@ jobs: profile: minimal toolchain: nightly override: true + components: clippy - name: Install afl uses: actions-rs/cargo@v1 with: @@ -215,6 +222,11 @@ jobs: with: command: afl args: system-config + - name: clippy + uses: actions-rs/cargo@v1 + with: + command: afl + args: clippy --no-default-features --manifest-path ${{ github.workspace }}/fuzz_read/Cargo.toml -- -D warnings - name: compile fuzz uses: actions-rs/cargo@v1 with: @@ -263,6 +275,7 @@ jobs: profile: minimal toolchain: nightly override: true + components: clippy - name: Install afl uses: actions-rs/cargo@v1 with: @@ -273,6 +286,11 @@ jobs: with: command: afl args: system-config + - name: clippy + uses: actions-rs/cargo@v1 + with: + command: afl + args: clippy --all-features --manifest-path ${{ github.workspace }}/fuzz_write/Cargo.toml -- -D warnings - name: compile fuzz uses: actions-rs/cargo@v1 with: @@ -332,6 +350,7 @@ jobs: profile: minimal toolchain: nightly override: true + components: clippy - name: Install afl uses: actions-rs/cargo@v1 with: @@ -342,6 +361,11 @@ jobs: with: command: afl args: system-config + - name: clippy + uses: actions-rs/cargo@v1 + with: + command: afl + args: clippy --no-default-features --manifest-path ${{ github.workspace }}/fuzz_write/Cargo.toml -- -D warnings - name: compile fuzz uses: actions-rs/cargo@v1 with: From df9deafb82640632a767579c525e494a2708b367 Mon Sep 17 00:00:00 2001 From: bogay Date: Fri, 24 Jan 2025 15:30:07 +0800 Subject: [PATCH 4/4] style: fix some clippy warnings In this commit I replaced `self.path.join("/")` with `self.path.join("")` because I think the expected result should be the original path with trailing "/". see: https://rust-lang.github.io/rust-clippy/master/index.html#join_absolute_paths --- fuzz_write/src/main.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/fuzz_write/src/main.rs b/fuzz_write/src/main.rs index c3fd09901..388e0d29d 100644 --- a/fuzz_write/src/main.rs +++ b/fuzz_write/src/main.rs @@ -49,11 +49,11 @@ pub struct FileOperation<'k> { // 'abort' flag is separate, to prevent trying to copy an aborted file } -impl<'k> FileOperation<'k> { +impl FileOperation<'_> { fn get_path(&self) -> Option { match &self.basic { BasicFileOperation::SetArchiveComment(_) => None, - BasicFileOperation::WriteDirectory(_) => Some(self.path.join("/")), + BasicFileOperation::WriteDirectory(_) => Some(self.path.join("")), BasicFileOperation::MergeWithOtherFile { operations, .. } => operations .iter() .flat_map(|(op, abort)| if !abort { op.get_path() } else { None }) @@ -84,9 +84,9 @@ fn deduplicate_paths(copy: &mut PathBuf, original: &PathBuf) { } } -fn do_operation<'k>( +fn do_operation( writer: &mut zip::ZipWriter>>, - operation: FileOperation<'k>, + operation: FileOperation<'_>, abort: bool, flush_on_finish_file: bool, files_added: &mut usize, @@ -125,7 +125,7 @@ fn do_operation<'k>( writer.start_file_from_path(&*path, options)?; for chunk in contents.iter() { writeln!(stringifier, "writer.write_all(&{:?})?;", chunk)?; - writer.write_all(&chunk)?; + writer.write_all(chunk)?; } *files_added += 1; } @@ -298,7 +298,7 @@ fn do_operation<'k>( Ok(()) } -impl<'k> FuzzTestCase<'k> { +impl FuzzTestCase<'_> { fn execute(self, stringifier: &mut impl Write, panic_on_error: bool) -> ZipResult<()> { let mut initial_junk = Cursor::new(self.initial_junk.into_vec()); initial_junk.seek(SeekFrom::End(0))?; @@ -332,7 +332,7 @@ impl<'k> FuzzTestCase<'k> { } } -impl<'k> Debug for FuzzTestCase<'k> { +impl Debug for FuzzTestCase<'_> { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { if self.initial_junk.is_empty() { writeln!(