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

Multiple changes; python 2 support, dictionary, mutator refactor #26

Merged
merged 15 commits into from Jan 19, 2020
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Replaced infinite mutation loop with bounded loop.

Previously there was always a likelihood that we would terminate our
retries if the mutator said that it was unable to be used, because
we had a number of mutators that were unconditional. However, now that
the mutators are able to be filtered, it is possible to select a
set of mutators which may always claim they are inappropriate. In
such a case, we would loop forever.

This change bounds the retries on looking for a mutator to 20
attempts - an arbitrary number I picked from the air as seeming
reasonable.
  • Loading branch information
gerph committed Jan 7, 2020
commit ef2e1cf0e4ab5052efd7a92993b955ba57437241
@@ -459,7 +459,9 @@ def mutate(self, buf):
for i in range(nm):

# Select a mutator from those we can apply
while True:
# We'll try up to 20 times, but if we don't find a
# suitable mutator after that, we'll just give up.
for n in range(20):

This comment has been minimized.

Copy link
@yevgenypats

yevgenypats Jan 11, 2020

Contributor

why change to 20 instead of the nm? nm = self._rand_exp() was a magic number used both in go-fuzz and afl/libfuzzer and it worked well. I tried other numbers and it usually affected the speed of the fuzzer significantly

This comment has been minimized.

Copy link
@gerph

gerph Jan 11, 2020

Author Contributor

You're confusing the two loops, I think.

The outer loop (line 495 in the new changes, just above those change) selects the number of mutations to run. Before this change, it was actually the 'number of allocators we'll try to run', because if the mutator said 'sorry, I'm not appropriate' (by decrementing the iteration count) it wouldn't actually affect the number of mutations run.

In this new version, when the mutator says it's not appropriate (by returning None at line 504), we want to try the mutation sequence again. This is what the inner loop of 20 attempts does. 20 was just picked from the air as seeming like a good number.

This means that the number of mutators that we plan to add, as determined in by nm, will be the number we generate (unless we are exceptionally unlucky and repeatedly select inapprorpriate mutations within the inner loop - which can happen if you've filtered the mutations that are used to those that are more likely to complain).

This is a change in behaviour from the original - instead of just ignoring mutations that are unavailable, the code now tries to meet the number that were requested. I chose to follow what appeared to be the intent of the code (that the number of mutators we plan to apply will be the number that we aim for, and that we retry rather than drop those attempts when they fail).

This comment has been minimized.

Copy link
@gerph

gerph Jan 11, 2020

Author Contributor

In case it's not clear, the inner loop is bounded so that we don't get stuck continually retrying. I originally was lazy and used while True and then when testing the filtering, generated a filter that left only a mutator that worked on a non-empty input, but the string always started out as empty when no dictionary or corpus was supplied, so it ran forever.

x = self._rand(len(self.mutators))
mutator = self.mutators[x]

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.