This is an archive of the discontinued LLVM Phabricator instance.

[libFuzzer] Handle unstable edges by using minimum hit counts
ClosedPublic

Authored by kevinwkt on Jul 18 2018, 5:39 PM.

Details

Summary

Created unstable_handle flag that takes 1 or 2, depending on the handling type.
Modified RunOne to accommodate the following heuristic:

Use the first CollectFeatures to count how many features there are.
If no new features, CollectFeatures like before.
If there is new feature, we run CB 2 more times,
    Check which edges are unstable per input and we store the least amount of hit counts for each edge.
    Apply these hit counts back to inline8bitcounters so that CollectFeatures can work as intended.

Modified UnstableCounters to 8int_t and created a bitset UnstableSet to tell which edges are unstable.

Patch by Kyungtak Woo (@kevinwkt).

Diff Detail

Event Timeline

kevinwkt created this revision.Jul 18 2018, 5:39 PM
Dor1s added inline comments.Jul 19 2018, 7:06 AM
lib/fuzzer/FuzzerCorpus.h
241

CheckFeature is a very generalized name. Can we call this like IsFeatureStable? or IsFeatureNew?

lib/fuzzer/FuzzerFlags.def
113

unstable_handle sounds weird to me, if you want to go with "handle", let's do handle_instability IMO. Feel free to ignore this comment unless others also raise this point.

lib/fuzzer/FuzzerLoop.cpp
496

Hm... shouldn't we check Options.UnstableHandle here instead of Options.PrintUnstableStats? If yes, you can just put this inside { } of the condition on line 491.

lib/fuzzer/FuzzerTracePC.cpp
86–97

This line and line 349 do not look good to me, why do we use 1? If it's an important constant, it should be in a constant variable, not a literal value used in different places.

lib/fuzzer/FuzzerTracePC.h
20

nit: use alphabetical order, incude bitset first, then set

150

Wait, this is bad, you're going to assign uint8_t values to int8_t. I think this is not what we've discussed regarding a bit mask. My suggestion was to keep using int16_t, but instead of kUnstableCounter = -1 make it to be something like kUnstableCounterMask = 1 << 8 to utilize higher bits of the UnstableCounters. What's wrong with that approach?

The bitset-based might also work, but I have two concerns:

  1. MSan may start reporting false positives again (it's been a case with std::vector)
  1. Whenever we decide to have another magic value to mark features with some other purpose, we won't have any room for that and have to either refactor the algorithm or introduce yet another bitset
metzman added inline comments.Jul 19 2018, 8:24 AM
lib/fuzzer/FuzzerCorpus.h
241

+1

243

Is this logic used elsewhere? I know AFL lets edges collide in the array, but from here it seems that LF does as well?

lib/fuzzer/FuzzerDriver.cpp
622

handle_unstable makes more sense I think

lib/fuzzer/FuzzerFlags.def
118

I can't understand the explanation for 2.
I think we should convey that if 2: we only consider new features for edges observed to be stable, meaning that they can never be non-deterministically reached.

lib/fuzzer/FuzzerLoop.cpp
452

This line is too long.
Please run clang-format-diff from now on.
It would probably help to configure your editor to highlight lines that are too long.

475

Does PoisonUnstable do anything? I'm not sure how it's actually handling unstable edges differently.

491

Please don't use multiline if bodies without curly braces.
Also I don't know why there isn't an || Options.PrintUnstableStats here.
We should only be collecting unstable stat info on interesting testcases right?

lib/fuzzer/FuzzerTracePC.cpp
87

Dumb question: why are we comparing UnstableIdx to MinUnstable, shouldn't the comparison be UnstableMode to MinUnstable? If not, then I find this very confusing.

93

I think the comment is wrong. It moves the counts to ModuleCounters right?

lib/fuzzer/FuzzerTracePC.h
77

HandleUnstableOptions if we are changing unstable_handle to handle_unstable

150

Did matt say to use a bitset? I don't like it since it is optimized for space (which we don't care much about) instead of speed (which we care a lot about).

An array of structs containing uint8_t and bool (so we can use true instead of 1) seems to be the best solution to me.

150

Also I don't know why this became int8_t. I think we decide it can be uint8_t but I think int8_t will lose info (lets say hit count of is UINT8_T_MAX across all runs.

150

Another idea: We don't really need to have UnstableCounters at all right? Couldn't we just use ModuleCounters instead? Don't worry about making this change now, since it is less flexible I think we can hold off on doing it until after the CL lands, if we find that we don't want to change this approach at all.

Dor1s added inline comments.Jul 19 2018, 8:29 AM
lib/fuzzer/FuzzerTracePC.h
150

Using an array of structs sounds good to me.

kevinwkt updated this revision to Diff 156309.Jul 19 2018, 10:24 AM
kevinwkt marked 19 inline comments as done.

PTAL
Ran Clang-Format-Diff

Changed CheckFeature to IsFeatureNew
Changed unstable_handle to handle_unstable
Put brackets around multiline ifs
Removed UnstableSet and created an array of structs

kevinwkt marked an inline comment as done.Jul 19 2018, 10:26 AM
kevinwkt added inline comments.
lib/fuzzer/FuzzerCorpus.h
243

This is copied from the AddFeature() method which is the method above this one.

lib/fuzzer/FuzzerLoop.cpp
475

PoisonUnstable is not implemented yet. I wanted the first implementation landed first. Should I also include it in this diff?

491

The new logic is the following:

if (Options.HandleUnstable || Options.PrintUnstableStats) {
  TPC.CollectFeatures([&](size_t Feature) {
    if (Corpus.IsFeatureNew(Feature, Size))
      NumNewFeaturesForUnstable++;
  });
  if (NumNewFeaturesForUnstable)
    CheckForUnstableCounters(Data, Size, Options.HandleUnstable);
}

This is due to that we are allowing print_unstable_stats to be run without handle_unstable.

496

I had it thought that you could either run with flags -handle_unstable and -print_unstable_stats independently and also together. If print_unstable_stats were to be run alone, then we would need to also run CheckForUnstableCounters.

lib/fuzzer/FuzzerTracePC.cpp
87

Bug fixed.

lib/fuzzer/FuzzerTracePC.h
150

We do need UnstableCounters when implementing the MinUnstable since we need somewhere to store the previous counter state to compare it with ModuleCounters (current state).

kevinwkt updated this revision to Diff 156321.Jul 19 2018, 11:00 AM

Removed PoisonUnstable from code.

Dor1s added inline comments.Jul 19 2018, 2:42 PM
lib/fuzzer/FuzzerTracePC.h
150

This looks a bit weird. Can you separate the struct and the array declarations please?

153

No need for this, a struct would have a default constructor generated by the compiler which initialized fields with the same values.

kevinwkt updated this revision to Diff 156369.Jul 19 2018, 3:01 PM
kevinwkt marked 8 inline comments as done.

PTAL
Separated struct from array declaration.
Removed unnecessary Struct constructor.

Dor1s added inline comments.Jul 19 2018, 3:29 PM
lib/fuzzer/FuzzerTracePC.h
77

If we go with enum, I think we should change functions UpdateUnstableCounters to accept an argument of HandleUnstableOptions type instead of int. Let's see first if Matt is fine with us using enum here.

Dor1s added a comment.Jul 20 2018, 8:59 AM

Kevin, can you think of a test that would verify that your handling approach works? We must have a test for this stuff to make sure it'll not get accidentally broken.

lib/fuzzer/FuzzerLoop.cpp
492

Did we consider incorporating this callback into the one used on line 501? I think this would be a bit cleaner and somewhat faster, rather then iterate through the features twice.

kevinwkt updated this revision to Diff 156533.Jul 20 2018, 10:21 AM
kevinwkt marked an inline comment as done.

Added test for -handle_unstable=1

kevinwkt added inline comments.Jul 20 2018, 10:22 AM
lib/fuzzer/FuzzerLoop.cpp
492

We use need to run the first one to check for new features in order to run CheckForUnstableCounters (line 498) accordingly before the "real" collectFeatures.

Dor1s accepted this revision.Jul 20 2018, 11:07 AM

Matt, can you please take a look?

This revision is now accepted and ready to land.Jul 20 2018, 11:07 AM
morehouse added inline comments.Jul 20 2018, 12:04 PM
lib/fuzzer/FuzzerCorpus.h
243

Can we reuse this inside AddFeature to reduce code duplication?

lib/fuzzer/FuzzerDriver.cpp
622

I know you intend to add other modes, but since there's only 1 right now, let's simplify this patch.

We can get rid of MinUnstable and just set Options.HandleUnstable = Flags.handle_unstable here. Elsewhere, we can just check if (Options.HandleUnstable) ...

lib/fuzzer/FuzzerInternal.h
71 ↗(On Diff #156533)

We shouldn't need to pass UnstableMode since we have access to Options.

lib/fuzzer/FuzzerLoop.cpp
453

Rather than passing UnstableMode around, let's check Options.

489

I think this can be a bool.

492

This iterates over all counters once. Then we iterate again with CheckForUnstableCounters if there are any new features. Then we iterate again in ApplyUnstableCounters. Then we iterate again with the below call to CollectFeatures.

If we get this down to 2 or 1 pass, the effectiveness of this feature may increase significantly.

test/fuzzer/handle_unstable_minunstable.test
6

Is the reason for not including t[0-4] that we could get lucky and hit the same one all three times?

kevinwkt updated this revision to Diff 156594.Jul 20 2018, 1:31 PM
kevinwkt marked 8 inline comments as done.

Got rid of enum and FuzzerTracePC::MinUnstable.

ptal

lib/fuzzer/FuzzerCorpus.h
243

We could, the only thing I could think of was the following:

  1. template function where IsFeatureNew would send an empty lambda, and AddFeature would send everything in between
  2. Call IsFeatureNew within AddFeature, but we would need to get Idx and OldSize in AddFeature anyways.

Do you have any suggestions?

lib/fuzzer/FuzzerLoop.cpp
492

Currently:

Iterate to check if new features
Iterate to save Counters to UnstableCounters

Run CB
Iterate to compare and save Counters to UnstableCounters (Update)

RunCB
Iterate to compare and save Counters to UnstableCounters(Update)

Copy UnstableCounters to Counters (Apply, only if handle_unstable=1)

I dont think we can unite things when there's CB in between.

  1. We could bind the "check if new features" with "iterate to save Counters to UnstableCounters" but I dont think this is beneficial given that we would be doing this even if no new features are found.
  2. ApplyUnstableCounters could be done at same time as Update, if we have an if-else condition. But this would make the code more complicated. I don't know if it would be worth it.

I might be missing something. What do you think?

morehouse added inline comments.Jul 20 2018, 2:51 PM
lib/fuzzer/FuzzerCorpus.h
243

No, looking closer I don't think there's a good way to reuse this... Ignore my previous suggestion.

244

Can combine previous two lines into

uint32_t OldSize = GetFeature(Idx % kFeatureSetSize);
245

We probably want to check Shrink here too.

Can simplify this to

return OldSize == 0 || (Shrink && OldSize > NewSize);
lib/fuzzer/FuzzerLoop.cpp
489

If it's a bool, we should rename it to something like NewFeaturesUnstable.

492

We can probably eliminate two iterations by remembering the new features that are stable and directly adding them to the corpus rather than modifying the counters.

kevinwkt updated this revision to Diff 156631.EditedJul 20 2018, 4:07 PM
kevinwkt marked 8 inline comments as done.

ptal
Changed IsFeatureNew like suggested.
Improvements to the amount of runs will be optimized if needed in the future.

morehouse added inline comments.Jul 20 2018, 4:16 PM
lib/fuzzer/FuzzerCorpus.h
244

Does this compile? I don't think Shrink is declared anywhere.

lib/fuzzer/FuzzerLoop.cpp
489

This comment was NOT addressed.

kevinwkt updated this revision to Diff 156638.Jul 20 2018, 4:36 PM
kevinwkt marked an inline comment as done.

ptal
Passes tests, fixed naming issue as previously asked.

morehouse accepted this revision.Jul 20 2018, 4:45 PM
Dor1s edited the summary of this revision. (Show Details)Jul 20 2018, 8:05 PM
Dor1s added subscribers: kcc, llvm-commits.

Matt, thanks a lot for the review!

Kostya, we'll land this on Monday morning and enable as a strategy on CF / OSS-Fuzz

Dor1s added a comment.Jul 23 2018, 6:58 AM

Kevin, a bunch of tests seems to be failing for me with your change, but everything works perfectly without it. Do the tests pass for you?

Dor1s added a comment.Jul 23 2018, 7:18 AM

Kevin, don't panic, ninja clean && ninja && ninja check-fuzzer made things work for me. Committing now.

Dor1s updated this revision to Diff 156781.Jul 23 2018, 7:20 AM

Rebase and getting ready to commit.

Herald added subscribers: Restricted Project, delcypher. · View Herald TranscriptJul 23 2018, 7:20 AM
This revision was automatically updated to reflect the committed changes.
Dor1s added a comment.Jul 23 2018, 8:53 PM

Kevin, looks like test is not fully deterministic :(

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/5509

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/5509/steps/ninja%20check%202/logs/FAIL%3A%20libFuzzer%3A%3A%20handle_unstable_minunstable.test

This is the first testbot failure I've seen so far, so am not reverting the change yet, but please take a look into why the test might be failing sometimes.

thopre added a subscriber: thopre.Jul 25 2018, 8:13 AM

Kevin, looks like test is not fully deterministic :(

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/5509

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/5509/steps/ninja%20check%202/logs/FAIL%3A%20libFuzzer%3A%3A%20handle_unstable_minunstable.test

This is the first testbot failure I've seen so far, so am not reverting the change yet, but please take a look into why the test might be failing sometimes.

It seems to have be failing for aarch64 since it was introduced:

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/5509 is failing and covers the range which added this test
http://lab.llvm.org:8011/waterfall?show=clang-cmake-aarch64-full shows that the test has been failing since that build onwards, except for build 5514 which was interrupted before stage2 tests were run.

Kevin, looks like test is not fully deterministic :(

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/5509

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/5509/steps/ninja%20check%202/logs/FAIL%3A%20libFuzzer%3A%3A%20handle_unstable_minunstable.test

This is the first testbot failure I've seen so far, so am not reverting the change yet, but please take a look into why the test might be failing sometimes.

It seems to have be failing for aarch64 since it was introduced:

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/5509 is failing and covers the range which added this test
http://lab.llvm.org:8011/waterfall?show=clang-cmake-aarch64-full shows that the test has been failing since that build onwards, except for build 5514 which was interrupted before stage2 tests were run.

@thopre, thanks for the heads up and sorry about the breakage. Kevin has prepared a fix, we'll land it soon.