Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: lint fuzz projects in CI #283

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat: lint fuzz projects in CI #283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26bf2f0
ci: checks format for fuzz_read and fuzz_write
Bogay Jan 20, 2025
4cebeba
style: simplify checks for empty extra data fields
Bogay Jan 24, 2025
4dd996d
ci: add clippy checks for fuzz_read and fuzz_write
Bogay Jan 24, 2025
df9deaf
style: fix some clippy warnings
Bogay Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -126,6 +136,7 @@ jobs:
profile: minimal
toolchain: nightly
override: true
components: clippy
- name: Install afl
uses: actions-rs/cargo@v1
with:
Expand All @@ -136,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:
Expand Down Expand Up @@ -195,6 +211,7 @@ jobs:
profile: minimal
toolchain: nightly
override: true
components: clippy
- name: Install afl
uses: actions-rs/cargo@v1
with:
Expand All @@ -205,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:
Expand Down Expand Up @@ -253,6 +275,7 @@ jobs:
profile: minimal
toolchain: nightly
override: true
components: clippy
- name: Install afl
uses: actions-rs/cargo@v1
with:
Expand All @@ -263,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:
Expand Down Expand Up @@ -322,6 +350,7 @@ jobs:
profile: minimal
toolchain: nightly
override: true
components: clippy
- name: Install afl
uses: actions-rs/cargo@v1
with:
Expand All @@ -332,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:
Expand Down
172 changes: 124 additions & 48 deletions fuzz_write/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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<PathBuf> {
match &self.basic {
BasicFileOperation::SetArchiveComment(_) => None,
BasicFileOperation::WriteDirectory(_) => Some(self.path.join("/")),
BasicFileOperation::WriteDirectory(_) => Some(self.path.join("")),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether this is intened. I guess in this line we want to return a path with trailing /, so I replaced the params with "".

see: https://rust-lang.github.io/rust-clippy/master/index.html#join_absolute_paths

Copy link
Member

@Pr0methean Pr0methean Jan 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably isn't the correct behavior either. What's the correct way to append a / to a path? That's how we detect that it is, in fact, intended to be a directory and not a normal file. The documentation indicates, if I'm reading it correctly, that self.path.join("") doesn't do that and is instead a no-op. Maybe we need to append the / at some point while it's still a string-like type?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation indicates, if I'm reading it correctly, that self.path.join("") doesn't do that and is instead a no-op.

I think self.path.join("") appends a / at the end of path. You can verify that with following code:

use std::path::PathBuf;

fn main() {
    let original_path = PathBuf::from("/home/user");
    let joined_path = original_path.join("");

    println!("Original path: {:?}", original_path);
    println!("Joined path: {:?}", joined_path);
    assert_eq!(joined_path, PathBuf::from("/home/user/"));
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=37109e0971e2e92e6ef255d46d3f8eeb

BasicFileOperation::MergeWithOtherFile { operations, .. } => operations
.iter()
.flat_map(|(op, abort)| if !abort { op.get_path() } else { None })
Expand Down Expand Up @@ -84,44 +84,66 @@ fn deduplicate_paths(copy: &mut PathBuf, original: &PathBuf) {
}
}

fn do_operation<'k>(
fn do_operation(
writer: &mut zip::ZipWriter<Cursor<Vec<u8>>>,
operation: FileOperation<'k>,
operation: FileOperation<'_>,
abort: bool,
flush_on_finish_file: bool,
files_added: &mut usize,
stringifier: &mut impl Write,
panic_on_error: bool
panic_on_error: bool,
) -> Result<(), Box<dyn std::error::Error>> {
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::<usize>();
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() {
writeln!(stringifier, "writer.write_all(&{:?})?;", chunk)?;
writer.write_all(&chunk)?;
writer.write_all(chunk)?;
}
*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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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<Cursor<Vec<u8>>>| {
(|| -> ZipResult<zip::ZipWriter<Cursor<Vec<u8>>>> {
zip::ZipWriter::new_append(old_writer.finish()?)
})().unwrap_or_else(|_| {
})()
.unwrap_or_else(|_| {
if panic_on_error {
panic!("Failed to create new ZipWriter")
}
Expand All @@ -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<Cursor<Vec<u8>>>> {
zip::ZipWriter::new_append(old_writer.finish()?)
})().unwrap_or_else(|_| {
})()
.unwrap_or_else(|_| {
if panic_on_error {
panic!("Failed to create new ZipWriter")
}
Expand All @@ -229,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))?;
Expand All @@ -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 {
Expand All @@ -263,14 +332,21 @@ 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!(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(())
Expand Down
Loading
Loading