Skip to content

Make input loading fallible in SyncFromDiskStage #3195

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.

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

R9295
Copy link
Collaborator

@R9295 R9295 commented May 5, 2025

Description

This PR introduces the InvalidInput variant in Error. When syncing in SyncFromDiskStage, if such an error is encountered, it is logged and the file is skipped.

Reasoning:
In the case of a grammar fuzzer loading inputs from other fuzzers, the inputs may be structurally invalid. This should not make it an error in the syncing process

Checklist

  • [ x] I have run ./scripts/precommit.sh and addressed all comments

@R9295 R9295 requested review from domenukk and tokatoka May 5, 2025 11:16
@R9295 R9295 force-pushed the sync-fallible branch from 5249843 to f983d80 Compare May 5, 2025 13:18
@R9295 R9295 requested a review from domenukk May 8, 2025 15:14
@@ -341,6 +341,8 @@ pub enum Error {
InvalidCorpus(String, ErrorBacktrace),
/// Error specific to a runtime like QEMU or Frida
Runtime(String, ErrorBacktrace),
/// The `Input` was invalid.
InvalidInput(ErrorBacktrace),
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense to include a string here? Like, the path of the invalid input, and maybe a root cause if there is any?

Copy link
Member

Choose a reason for hiding this comment

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

Especially given the callback is user provided, adding a string will make things way easier to debug IMHO. The callback can differentiate between different invalid input causes

@R9295 R9295 requested a review from domenukk May 11, 2025 11:05
let input = match (self.load_callback)(fuzzer, state, &path) {
Ok(input) => input,
Err(Error::InvalidInput(reason, _)) => {
log::debug!(
Copy link
Member

Choose a reason for hiding this comment

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

Think this should be warning!

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe info if you think it's expected

@@ -1064,7 +1081,7 @@ pub fn get_thread_id() -> u64 {
#[allow(clippy::cast_sign_loss)]
/// Return thread ID without using TLS
pub fn get_thread_id() -> u64 {
use libc::{SYS_gettid, syscall};
use libc::{syscall, SYS_gettid};
Copy link
Member

Choose a reason for hiding this comment

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

is this a cargo fmt update of some sorts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes

Copy link
Member

Choose a reason for hiding this comment

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

Seems like your fmt is not the same as the CI one

@domenukk
Copy link
Member

Change debug to info and figure out how to make CI happy, then this is good to go <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants