provide option for deterministic dictionary mutation in non-deterministic mode (-d or -S)

[laurentsimon]

This adds support or running AFL in non-deterministic mode and a dictionary.

[lszekeres]

would it be simpler to leave this condition, and goto dict_stage here, and at

dict_stage:
  if (!use_dictionary) goto havoc_stage;

?

[laurentsimon]

I don't have a strong preference.

…

On Tue, Apr 28, 2020 at 7:39 PM Laszlo Szekeres ***@***.***> wrote: ***@***.**** approved this pull request. ------------------------------ In afl-fuzz.c <#91 (comment)>: > @@ -5117,7 +5118,7 @@ static u8 fuzz_one(char** argv) { this entry ourselves (was_fuzzed), or if it has gone through deterministic testing in earlier, resumed runs (passed_det). */ - if (skip_deterministic || queue_cur->was_fuzzed || queue_cur->passed_det) + if ((skip_deterministic && !use_dictionary) || queue_cur->was_fuzzed || queue_cur->passed_det) goto havoc_stage; would it be simpler to leave this condition, and goto dict_stage here, and at dict_stage: if (!use_dictionary) goto havoc_stage; ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#91 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APMEKC7NZXZUHVAKXEIK553RO6HODANCNFSM4MTKHC2A> .

[laurentsimon]

maybe one reason not to do it is that using too many goto makes the code harder to read... but in this case... maybe OK? On Tue, Apr 28, 2020 at 8:01 PM Laurent Simon <laurentsimon@google.com> wrote:

…

I don't have a strong preference. On Tue, Apr 28, 2020 at 7:39 PM Laszlo Szekeres ***@***.***> wrote: > ***@***.**** approved this pull request. > ------------------------------ > > In afl-fuzz.c > <#91 (comment)>: > > > @@ -5117,7 +5118,7 @@ static u8 fuzz_one(char** argv) { > this entry ourselves (was_fuzzed), or if it has gone through deterministic > testing in earlier, resumed runs (passed_det). */ > > - if (skip_deterministic || queue_cur->was_fuzzed || queue_cur->passed_det) > + if ((skip_deterministic && !use_dictionary) || queue_cur->was_fuzzed || queue_cur->passed_det) > goto havoc_stage; > > would it be simpler to leave this condition, and goto dict_stage here, > and at > > dict_stage: > if (!use_dictionary) goto havoc_stage; > > ? > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#91 (review)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/APMEKC7NZXZUHVAKXEIK553RO6HODANCNFSM4MTKHC2A> > . >

[vanhauser-thc]

what is the reasoning behind the idea that a slave should also do deterministic fuzzing?

IMHO that is very bad.

deterministic fuzzing means that the generated mutations are always the same. hence they are only done once per queue entry and then never again.

with this options every slave would do the deterministic fuzzing too resulting in as many duplicates of the same fuzzing inputs as there are slaves.

if the reasoning behind is that deterministic fuzzing is running behind with many discovered queue entries as only the master is doing it, I would rather propose a different solution - only the afl-fuzz which discovers a new queue entry performs deterministic fuzzing, learned queue entries (so from other afl-fuzz instances) will not. But this means that this command line option must be given to every single instance though.

also the results at fuzzbench have shown that deterministic fuzzing discovers less paths than random fuzzing, especially with a good and large corpus.

[laurentsimon]

Thanks for your feedback. We're not proposing to do deterministic fuzzing for slaves. We're proposing that the dictionary be used in non-deterministic mode if the -x option is passed by the user. The reasoning is that many users use -x -d options for example, without realizing the -x will be ignored silently by afl if -d is used. Today the only way to use dictionary and non-determinic fuzzing is to spawn 2 instances (-M -x and -S). Not everyone wants to do this. Does it make sense? Please feel free to suggest a better alternative.

…

On Wed, Apr 29, 2020 at 2:17 AM van Hauser ***@***.***> wrote: what is the reasoning behind the idea that a slave should also do deterministic fuzzing? IMHO that is very bad. deterministic fuzzing means that the generated mutations are always the same. hence they are only done once per queue entry and then never again. with this options every slave would do the deterministic fuzzing too resulting in as many duplicates of the same fuzzing inputs as there are slaves. if the reasoning behind is that deterministic fuzzing is running behind with many discovered queue entries as only the master is doing it, I would rather propose a different solution - only the afl-fuzz which discovers a new queue entry performs deterministic fuzzing, learned queue entries (so from other afl-fuzz instances) will not. But this means that this command line option must be given to every single instance though. also the results at fuzzbench have shown that deterministic fuzzing discovers less paths than random fuzzing, especially with a good and large corpus. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#91 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APMEKC3NBZYBOUBN4RMQC43RO7WBHANCNFSM4MTKHC2A> .

[vanhauser-thc]

@laurentsimon ah sorry, the original title was misleading so I thought it would activate deterministic fuzzing in slaves :)

Now I get your point - you want to have dictionaries if -x is used, which are silently ignored if -S or -d otherwise.

You still have the issue that the dictionary part is done every time the queue entry is selected and not only once and that is an issue that should be fixed (setting and checking passed_det).

And I think you should show a warning though. either warn it is ignored (if left as is), or warn that if other instances are running that there will be duplicates.

[vanhauser-thc]

Oh and I totally forgot:

dictionaries are not ignored with -S/-d. in havoc:

    switch (rand_below(
          afl, 15 + ((extras_cnt + a_extras_cnt) ? 2 : 0))) {

so if there are dictionaries, stages 15 + 16 are activated and they randomly put dictionary entries in the mutation

[laurentsimon]

Thanks. These are valid points. mark_as_det_done() is called at https://github.com/google/AFL/blob/master/afl-fuzz.c#L6084, so we should be OK with pass_det problem. I'll double check in case. You're right the dictionary is used in non-deterministic mode but only probabilistically (which is the point of non-deterministic mode :)). This has some effect in practice: I tested with the following program: if (buf[0] == 1 && buf[1] == 2 && buf[2] == 3 && buf[3] == 4 && buf[4] == 5 && buf[5] == 6) abort(); It took AFL between 10s and 2mn to find the bug, compared to 2s if we apply the patches. So there is some value in changing dictionary behavior with non-deterministic mode. That said, the patches would bias the mutations towards doing more dictionary insertions vs non-deterministic ones. So I think if we apply some patches, maybe it should be another option, ex -X instead of -x. Clearly the best option right now is to use two afl instances: -M -x and -S. Let's not land these patches for now and keep the discussion going. Let us know what you think. What do others think?

…

On Thu, Apr 30, 2020 at 2:50 AM van Hauser ***@***.***> wrote: Oh and I totally forgot: dictionaries are not ignored -S/-d. in havoc: switch (rand_below( afl, 15 + ((extras_cnt + a_extras_cnt) ? 2 : 0))) { so if there are dictionaries, stages 15 + 16 are activated and they randomly put dictionary entries in the mutation — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#91 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APMEKCZ7UKNATW2Q2VTIAWDRPFCVVANCNFSM4MTKHC2A> .

[vanhauser-thc]

well you could put an entry to fuzzbench with this patch and then we can see if it makes it perform better or not overall. "overall" is however just an indicator. if something is really good at corner cases it could already be good to have.

[laurentsimon]

I'm leaning towards a similar solution. Thanks again for your input!

…

On Thu, Apr 30, 2020 at 12:15 PM van Hauser ***@***.***> wrote: well you could put an entry to fuzzbench with this patch and then we can see if it makes it perform better or not overall. "overall" is however just an indicator. if something is really good at corner cases it could already be good to have. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#91 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APMEKC3M4AIVHHTQDWWLAPTRPHE5PANCNFSM4MTKHC2A> .

[Dor1s]

@lszekeres @laurentsimon what's the final decision here? Do we merge this or close?

[laurentsimon]

it's not been tested so we cannot merge it. Maybe we could create a branch for it so we could test it on fuzzbench...? Not sure how many benchmarks have a dictionary, though.

…

On Mon, Aug 3, 2020 at 5:12 PM Max Moroz ***@***.***> wrote: ***@***.**** commented on this pull request. @lszekeres <https://github.com/lszekeres> @laurentsimon <https://github.com/laurentsimon> what's the final decision here? Do we merge this or close? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#91 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APMEKC7PPUZ47C2LCSJMPADR65G5VANCNFSM4MTKHC2A> .

[lszekeres]

it's not been tested so we cannot merge it. Maybe we could create a branch for it so we could test it on fuzzbench...?

This says 4, but grepping dict on fuzzbench/benchmark says 9 :)

Consider running a fuzzbench experiment using the branch of this PR (ie https://github.com/laurentsimon/AFL) and compare to upstream afl.

[laurentsimon]

@jonathanmetzman shall we run an experiment with the modified AFL, as suggested by @lszekeres ?