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.

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

Introduce replaying fuzzer #2886

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

Conversation

riesentoaster
Copy link
Contributor

I have an unstable target regarding my observer values. There is very little I can do about this. One thing that helps is just repeatedly call the target until I'm somewhat sure which version is the correct one. This is what is happening here.

I know, more changes to fuzzer, but this makes sense neither in stages nor in the executor.

Ideally, one would be able to pass multiple handles (e.g. as a tuple_list), but I haven't quite figured out how to explain this to the compiler.

@domenukk
Copy link
Member

This sounds very related to the calibration stage (?)

@riesentoaster
Copy link
Contributor Author

Yes, but calibration stage only reports the diffs and doesn't prevent corpus explosion based on essentially duplicate entries.

Additionally, it only works for map observers, where ReplayingFuzzer works for any observer.

@tokatoka
Copy link
Member

Do you have any idea if this can be implemented into the existing fuzzer.rs

One thing we want to avoid is to having too much fuzzer.rs-variants with little difference across them. If you can do this, with Stages(i think this is possible with IfStage, Closure stage... etc), or without adding another Fuzzer then it is better?

@riesentoaster
Copy link
Contributor Author

Do you have any idea if this can be implemented into the existing fuzzer.rs

Yes, but it would introduce a small performance penalty because I'd need to check on each execute_input if a struct value is equal to 1. The compiler may be able to optimize it away, but I'm not sure.

One thing we want to avoid is to having too much fuzzer.rs-variants with little difference across them. If you can do this, with Stages(i think this is possible with IfStage, Closure stage... etc), or without adding another Fuzzer then it is better?

Fuzzer (or specifically ExecutesInput) is where this happens. Anything else would get very hacky and introduce a lot of code duplication and opportunity for misconfiguration.

@riesentoaster
Copy link
Contributor Author

I have added a bunch more logic to deal with inconsistent inputs. Since we're re-running the entire observer/executor/feedback stack, performance should be negligible, but it makes it much more flexible.

@@ -82,6 +84,8 @@ pub enum DiffExitKind {
Oom,
/// The run timed out
Timeout,
/// The run reports inconsistent results
Inconsistent,
Copy link
Member

Choose a reason for hiding this comment

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

This is very special to your case, maybe explain where/when this is actually used?
Why in Diff btw?

(unrelated sidenote: We should add Result::Skip) that never stores to corpus

@domenukk
Copy link
Member

A few questions:

  • Why can't that be done with a stage?
  • Can we instead add some general sort of hooks (or nested stages?) that run before storing new testcases (and that you can use to not have to build a new fuzzer) We could reuse that to improve the calibration stage
  • If not, can we at least share code with the normal/existing fuzzer?

@riesentoaster
Copy link
Contributor Author

riesentoaster commented Jan 24, 2025

  • Why can't that be done with a stage?

Because other stages (such as mutational stages) need it during their evaluation. It would also lead to code duplication since we'd have multiple implementations of the logic to prep observers, execute an input, check oberservers etc.

CalibrationStage isn't pretty in this regard as well. Plus, it does multiple things that are somewhat unrelated. (See e.g. #2857)

  • Can we instead add some general sort of hooks (or nested stages?) that run before storing new testcases (and that you can use to not have to build a new fuzzer) We could reuse that to improve the calibration stage

Maybe add this logic as a default implementation in the Stage trait? This would move it out of Fuzzer though. (Disclaimer: I haven't checked if this is actually possible)

Edit: this doesn't actually work I don't think, since I still want a central place to toggle it on or off.

  • If not, can we at least share code with the normal/existing fuzzer?

See above: yes, but it would include adding some data to the fuzzer struct and an single boolean check for each execution even for the existing mode. If that's something we're comfortable with, this would reduce like 80% of extra code.

@domenukk
Copy link
Member

Can we make this a compile-time boolean / type? then it'll get optimized out anyway(?)

I still believe the "correct" way to build this is with hooks that can run sub-stages, but I understand if that's too much refactoring

@riesentoaster
Copy link
Contributor Author

Can we make this a compile-time boolean / type? then it'll get optimized out anyway(?)

Possible, yes. I'm not sure the added complexity is worth the performance diff though. Let me give it a go.

I still believe the "correct" way to build this is with hooks that can run sub-stages, but I understand if that's too much refactoring

Not exactly sure what you mean.

@domenukk
Copy link
Member

For example: the calibration stage should ideally run every time we add a new entry to the corpus, before continuing adding the testcase to the actual corpus. As a hook. So it wouldn't be a normal stage anymore but become part of every stage that adds to the corpus

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.

3 participants