[PATCH] D66107: [libFuzzer] Improve -merge= process to account for REDUCED corpus units.

[Jonathan Metzman via Phabricator via llvm-commits]

metzman added a comment.

Thanks for looking into making this change. It should be very useful for CF.
I'll try to take a look again tomorrow morning with fresh eyes.



================
Comment at: lib/fuzzer/FuzzerDefs.h:23
 #include <vector>
-#include <set>
-#include <memory>
+#include <utility>
+
----------------
Nit: before `<vector>`.


================
Comment at: lib/fuzzer/FuzzerDriver.cpp:499
+  for (auto &Pair : ReplacedFiles) {
+    F->WriteToOutputCorpus(FileToVector(Pair.second, Options.MaxLen));
+    RemoveFile(Pair.first);
----------------
I'm a bit confused why WriteToOutputCorpus isn't going to truncate tescases (which i'm pretty sure we don't want).
I know your CL didn't introduce this, but do you know if this is the case?


================
Comment at: lib/fuzzer/FuzzerFork.cpp:224
     }
+    // TODO(Dor1s): go through FilesToReplace here.
     Features.insert(NewFeatures.begin(), NewFeatures.end());
----------------
How about doing the FilesToReplace loop inside of CrashResistantMerge instead so it doesn't need to be repeated and so that `CrashResistantMerge` doesn't need to be passed more arguments.


================
Comment at: lib/fuzzer/FuzzerMerge.cpp:194
+
+    auto Signature = SignaturesMap.find(Files[i].Signature);
+    if (Signature != SignaturesMap.end()) {
----------------
I'm not sure this approach with signatures works the same way reduction works within a fuzzing run (ie: the way I think we probably want). I'll have to think about this more by tomorrow, since I haven't 100% thought this through and the code dealing with this during fuzzing is tricky.

As I understand it, this implementation it only replaces initial corpus elements with new ones that are smaller and are exactly the same in coverage.
However, [[ https://cs.chromium.org/chromium/src/third_party/libFuzzer/src/FuzzerCorpus.h?g=0&l=220 | during fuzzing ]] corpus elements are replaced if a new corpus element is found that is smaller and covers the features for which the initial element is the smallest testcase, i.e. testcases can be replaced by smaller testcases that aren't exactly equivalent.

Example:
Unit A is the smallest element/unit covering feature X. A only covers feature X and no other feature.
Then we find Unit B which is smaller than Unit A. B covers features X and is the only (or smallest) unit that covers Y.
During fuzzing we will delete A and add B since B is now the smallest unit covering X.
Here, if A were an initial testcase (primary/output corpus) and B a new one (secondary/input corpus), I think we don't delete A.

Is my example correct?

If so then there may be a problem if there is an initial Unit C that is already covering Y, I don't think B will get added to the merged corpus, since it isn't a pure reduction of any testcase.


Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66107/new/

https://reviews.llvm.org/D66107