Filter Duplicate Input Execution

[riesentoaster]

See #2759.

Some mutators report MutationResult::Mutated, even if nothing actually changes about the input. HashMutator is a wrapper around other mutators that hashes inputs pre- and post-mutation to ensure MutationResult::Mutated is only reported if something actually changed.

This may be worth using on slow targets, where the hashing is quicker than the unnecessary additional executions of the target for previously tried inputs.

[domenukk]

HashFilterMutator?

Or even HashMutationFilter

[domenukk]

As I stated in the discussion thread, I think a method for rejecting inputs that were already tried would be more useful (but I don't know your use case, so..)
Maybe using a Bloomfilter on the executor, or similar..

[riesentoaster]

As I stated in the discussion thread, I think a method for rejecting inputs that were already tried would be more useful (but I don't know your use case, so..)

I'm targeting the TCP/IP stack of an OS, so each execution takes in the order of magnitude of 1s, although most of that is spent in wait states (hence previous work like overcommit). Even still, the added runtime of this would be nothing compared to the execution, so this felt like an easy win.

Maybe using a Bloomfilter on the executor, or similar..

Something like this would definitely further improve the situation. Do you suggest creating a wrapping executor that returns either ExitKind::Ok or a new ExitKind::Skipped if the input was previously evaluated? This seems like a bodged-on solution as well though, since observers/feedbacks still run — we probably don't even want to call the executor in such cases.

Tracing this back it seems most appropriate in the stage? But that seems not that generic. So maybe in Fuzzer (resp. it's Evaluator impl)?

I'm also not sure if there's an opportunity here to combine this somehow with CentralizedLauncher?

[domenukk]

I think it could simply wrap an executor, yeah. And have an extra observation that's "skipped" -if it's true the testcase isn't interesting. Should be easy enough to do.

\We can still merge this PR as well, but the feedback should be renamed IMHO.

[riesentoaster]

How about something like this?

[riesentoaster]

I'll do some performance comparisons later today. Initial runs suggest that adding even a 10µs sleep to the harness reduces the performance penalty to <5%.

I might also see how many duplicate inputs actually appear. But for now I feel like for slow targets this very well might be worth using.

[riesentoaster]

Alright, some performance tests. Running against the libfuzzer_libpng example fuzzer:

1. Without the bloom filter, I'm getting a throughput of ~100k/s.
2. With the bloom filter, I get ~85k/s.
3. The rate of duplicate vs. new inputs increases over time, after 1min and 2min it was 0.6%, after 3min it was 1%, after 4min 2%, after 5min 4.4%, after ~7min it reaches 10%, after ~13mins 40%. At this point I assume most inputs are going to be duplicates.

All these numbers obviously depend on the exact fuzzers:

1. When the corpus count reaches a plateau, duplicate inputs become increasingly likely
2. When the number of possible mutations is small, duplicate inputs are more likely
3. If the execution time of the target is larger, the added runtime may be less than the runtime saved from not executing an input twice.
4. The bloom filter requires quite a bit of memory, so if that is your limitation, not using it and instead spawning additional instances may be worth it

Overall, I feel like this may be worth having in the library.

Btw: There is no easy way of adding metadata to the state such that it is printed by monitors, right? Otherwise, calculating the number/rate of duplicates may be an interesting addition.

[domenukk]

There is an easy way, using UserStats see... however other things do UserStats. For example the stability in the calibration stage.

[domenukk]

That's just testing for I assume?

[domenukk]

The feature flag should be called by the name of the feature, not by implementation detail. Maybe something like. "reexecution_filter" or similar?

[domenukk]

bloom_input_filter would be a middle ground(?)

[riesentoaster]

wait i think you can just do this with IfElseStage
you can put your bloom filter into the closure to IfElseStage and execute the main stage of the fuzzer depending on the evaluated result of the filter

Btw: This will not work, since the stage might mutate the input and execute it multiple times while the filtering is only possible at the start of the stage. So while the input at the beginning of the mutational stage might not have been seen before, mutations might still transform it into a version we've already executed before. Also: The input at the beginning of the stage comes from the corpus, right? So it has been executed before by definition. So implementing this as a wrapper stage seems not possible. Implementing this within stages would require changes to every mutational stage.

I'm in favour of doing it in the fuzzer tbh. Implementations in the executor still require observers/feedbacks to run, implementations in the stage don't really work either.

[tokatoka]

Btw: This will not work

yes. after talking to domenuk i realized what we want to is to filter against every generated input. so it's impossible to do with stages

[riesentoaster]

So do I fix the things @domenukk mentioned in the beginning and we merge this approach? Or how do we continue?

[domenukk]

The idea is to add the option to return ExitKind::Skipped to Executors and give ExecutorHooks the option to return these.
Then we can use the bloom filter inside executor hooks. We'll probably also want to find a way to make calibration still possible, like, have some way to tell the hook to let this input through.

[riesentoaster]

Correct me if I'm wrong, but wouldn't this still run the observers and feedback every time?

[domenukk]

Not if we change the executors to not execute in this case

[riesentoaster]

But those are not run from within the executors but instead in the fuzzer, no?

Or do you want to change this as well?

[tokatoka]

@riesentoaster
i see what you point as the problem..
putting this in the executor will not cancel the observer run or feedback runs indeed

[tokatoka]

I think the problems is that we are using several types of APIs to call the harness target.
We can call it from fuzzer.evaluate_input() or we can call it from executor.run_target (See how messed things are in stages/*)

I think adding your change to one of them before unifying the use of them is not good

[tokatoka]

Another solution is that since your stuff will work mostly with MutationalStage or PowerMutationalStage then we can just add filter to those files only

then domenukk's

We'll probably also want to find a way to make calibration still possible, like, have some way to tell the hook to let this input through.

this problem is solved

[domenukk]

Also GenStage and TuneableMutationalStage at least. Sounds like a good solution but we need to be careful not to forget things

[riesentoaster]

Another solution is that since your stuff will work mostly with MutationalStage or PowerMutationalStage then we can just add filter to those files only

Also GenStage and TuneableMutationalStage at least. Sounds like a good solution but we need to be careful not to forget things

This sounds like a lot of code duplication.

I think the problems is that we are using several types of APIs to call the harness target. We can call it from fuzzer.evaluate_input() or we can call it from executor.run_target (See how messed things are in stages/*)

I like the ability to just call the executor without any observers or anything around it, since it may be helpful to run just the target.

I personally think the functionality in this PR should be implemented wherever the observers and executor are called during the fuzzing loop. If that is in the fuzzer, so be it. If we want it elsewhere, move the logic that calls observers/executors there, too. It's a single function. Anything else is just hacky.

Btw: Why does executor.run_target need a reference to the fuzzer? That seems like a cyclic dependency, since it would otherwise mostly be called through the fuzzer. I think it's exclusively used to run observers in InProcessExecutors, but those have observer tuples all over the place anyways, so I don't think this should be necessary.

[domenukk]

It's the right amount of code duplication: each stage should probably decide for itself if it needs to filter inputs or not.

[domenukk]

Unrelated, if you can remove some trait bounds you're more than welcome to open a PR :)

[domenukk]

We could have a run_with_filter method on the executor trait, maybe that'd reduce the shared code?

[riesentoaster]

We could have a run_with_filter method on the executor trait, maybe that'd reduce the shared code?

And have a default implementation that calls run_target? Put the functionality of EvaluatorObservers there and call the function from StdFuzzer? That'd work I think.

[riesentoaster]

I've thought about this some more. To me, filtering the evaluation of an input belongs in the Evaluator trait. That's stage-independent and encompasses anything that is part of the evaluation. It is just the right place for this, anything else I've thought of is just jumping through hoops to implement something worse.

1. If you absolutely do not want to touch StdFuzzer, the best alternative I see is implementing a second Fuzzer that essentially wraps StdFuzzer, forwarding all functions except what is necessary for the filtering. That's just a lot of boilerplate code.
2. Alternatively, I could try to put as much common logic as possible in a new structure that is used by both StdFuzzer and the new FilteringFuzzer or whatever we end up calling it. This would lead to a rewrite of the internal architecture, but no changes to the logic or outer interface, and the new StdFuzzer would have no filtering logic in it.
3. Finally, I've again come to like the approach of this PR. It should not introduce any slowdowns for StdFuzzer because the check for the unfiltered option is static and can be optimised away by the compiler. And it introduces as little additional and changed code as possible.

Would you be willing to entertain any of these ideas? My current favourite is 3 > 1 > 2.

[riesentoaster]

@domenukk @tokatoka Opinions?

[tokatoka]

now we know executor hook won't work, and implementing into stages causes duplicate codes,
then i think the current code is good

[tokatoka]

can you add doc here to explain that here filtering is taking place

[domenukk]

I'm still pro stages: most stages don't need filtering. At least Calibration, Tracing, Concolic execution stages all will break when we filter the inputs. Many other stages are not executing the target.

[domenukk]

It's only relevant for three stages or so, right? Let's just add a helper function that the stages can call if they choose.

[riesentoaster]

I'm still pro stages: most stages don't need filtering. At least Calibration, Tracing, Concolic execution stages all will break when we filter the inputs. Many other stages are not executing the target.
It's only relevant for three stages or so, right? Let's just add a helper function that the stages can call if they choose.

Good points. That would also mean storing the filter data in each stage. Is that the best approach? Or do we want to store that globally for each fuzzing instance (so probably either in the state or the fuzzer)?

Also: I think we should still make the unfiltered version of each stage available, the overhead of filtering may not always be worth it. And since we need to store the data somewhere, but only in certain cases, it would involve significant code changes to each stage, more than a simple helper function.

[tokatoka]

can't we add evaluate_input_with_filter to the Evaluator trait?
then change the calls to evaluate_input to evaluate_input_with_filter in mutational.rs, power.rs ...
for the other calls such as conconlic we keep the current state

[domenukk]

I would name it evaluate_filtered but otherwise sounds good