[PATCH] Add a Fuzzer library

[Kostya Serebryany]

> > It'd probably be good to have the build infrastructure go in with the source - having non-building/non-testing source in tree isn't terribly useful.

> 


I'll send the patch for cmake support later today (I already have it working, but would like to disentangle the two commits)


================
Comment at: lib/Fuzzer/FuzzerFlags.h:1
@@ +1,2 @@
+//===- FuzzerFlags.cpp - Run-time flags -----------------------------------===//
+//
----------------
rnk wrote:
> LLVM typically uses a .def suffix for such repeated include files. Also, you can change the header comment to match.
done.

================
Comment at: lib/Fuzzer/FuzzerFlags.h:16
@@ +15,3 @@
+FUZZER_FLAG(int, iterations, -1,
+            "Number of iterations of the fuzzer (-1 for infinite runs).")
+FUZZER_FLAG(int, max_len, 64, "Maximal length of the test input.")
----------------
ygribov wrote:
> I wonder if people will mostly be interested to run fuzzer until coverage keeps increasing.
How do you know that coverage keeps increasing? 
And generally, no. Even if the coverage is steady fuzzing is still interesting (it finds bugs :))

================
Comment at: lib/Fuzzer/FuzzerIO.cpp:16
@@ +15,3 @@
+
+std::vector<std::string> ListFilesInDir(const std::string &Dir) {
+  std::vector<std::string> V;
----------------
dblaikie wrote:
> Dir could be StringRef here, maybe?
This lib should not have any external dependencies. 

================
Comment at: lib/Fuzzer/FuzzerIO.cpp:19
@@ +18,3 @@
+  DIR *D = opendir(Dir.c_str());
+  if (!D) return V;
+  while (auto E = readdir(D)) {
----------------
ygribov wrote:
> Perhaps allow files as well?
Maybe in next steps, or maybe not. 
Allowing files trades (small) flexibility to (small) complexity. 

================
Comment at: lib/Fuzzer/FuzzerIO.cpp:34
@@ +33,3 @@
+
+void WriteToFile(const Unit &U, std::string Path) {
+  std::ofstream OF(Path);
----------------
dblaikie wrote:
> Path should be passed by const ref here?
done

================
Comment at: lib/Fuzzer/FuzzerInternal.h:38
@@ +37,3 @@
+  struct FuzzingOptions {
+    int Verbosity = 1;
+    int MaxLen = 0;
----------------
dblaikie wrote:
> I suspect MSVC 2012 (2013? whatever our min support is) doesn't support non-static data member initializers.
Grrrr. I'd prefer not to bother about old MSVC at this point...
The whole thing (even sanitizer coverage) does not work on windows yet. 
And the fuzzer is a strictly tool for developers, they can use the newest MSVC. 

================
Comment at: lib/Fuzzer/FuzzerLoop.cpp:113
@@ +112,3 @@
+  size_t NewUnits = 0;
+  for (size_t Iter = 0; Iter < Options.MutateDepth; Iter++) {
+    Mutate(U, Options.MaxLen);
----------------
dblaikie wrote:
> idiomatic != rather than <? And prefix increment rather than post increment?
> 
> (& maybe use a name like 'i' which I think is idiomatic LLVM for index variables - since this is an index, not an iterator)
> 
> (similar comments on lots of other loops)
changed back to 'i'. the rest is bikesheding (?)

================
Comment at: lib/Fuzzer/FuzzerLoop.cpp:118
@@ +117,3 @@
+    if (NewCoverage) {
+      Corpus.push_back(*U);
+      NewUnits++;
----------------
ygribov wrote:
> Just curious, have you considered making this a heap to explore the most covering units first?
Sure, I wanted to add a suite minimizer here. (later)

================
Comment at: lib/Fuzzer/FuzzerMain.cpp:60
@@ +59,3 @@
+    std::cerr << "  " << D.Name;
+    for (size_t i = 0; i < MaxFlagLen - strlen(D.Name); i++)
+      std::cerr << " ";
----------------
dblaikie wrote:
> Compute the bound once rather than on every iteration?
done

================
Comment at: lib/Fuzzer/FuzzerMain.cpp:70
@@ +69,3 @@
+  if (Param[0] == '-' && strstr(Param + 1, Name) == Param + 1 &&
+      Param[Len + 1] == '=')
+      return &Param[Len + 2];
----------------
ygribov wrote:
> You know the len so perhaps just do memcmp for both checks?
will it make the code more readable? 

================
Comment at: lib/Fuzzer/FuzzerMain.cpp:85
@@ +84,3 @@
+        std::cerr << "Flag: " << Name << " " << Val << "\n";
+      return true;;
+    }
----------------
ygribov wrote:
> Extra ;
done

================
Comment at: lib/Fuzzer/FuzzerMain.cpp:118
@@ +117,3 @@
+  if (!inputs.empty())
+    Options.OutputCorpus = inputs[0];
+  Fuzzer F(Options);
----------------
ygribov wrote:
> Perhaps add an option for this?
Err. Maybe. need to see how this will be used. 
If we can get away w/o extra options -- we should. 


================
Comment at: lib/Fuzzer/FuzzerMutate.cpp:36
@@ +35,3 @@
+  assert(MaxLen > 0);
+  assert(U->size() <= MaxLen);
+  switch (rand() % 3) {
----------------
ygribov wrote:
> These asserts are not very user-friendly, perhaps check for them in main() instead? Actually I wonder if MaxLen should be MaxSizeIncreasePercentage...
Well, asserts are not for users, they are for developers. 

 MaxSizeIncreasePercentage is something to consider, yes. 

================
Comment at: lib/Fuzzer/FuzzerMutate.cpp:38
@@ +37,3 @@
+  switch (rand() % 3) {
+    case 0:
+      if (U->size()) U->erase(U->begin() + rand() % U->size());
----------------
dblaikie wrote:
> Is this clang-format/LLVM's formatting for case? I would've expected the 'case' to be at the same level as the 'switch'.
reformatted. 

================
Comment at: lib/Fuzzer/FuzzerUtil.cpp:42
@@ +41,3 @@
+  }
+  return std::to_string(h1) + std::to_string(h2);
+}
----------------
dblaikie wrote:
> Could be marginally more efficient to use an ostringstream here to avoid two allocations? (Well, I guess they'll be short strings, etc... - so maybe not a problem)
This is a very cold part of code.
Much better to keep things simple. 
Also, I want to depend on STL less, not more. 
I actually may need to get rid of all of STL later.

Reason: this fuzzer is in-process, so it will be testing other code that also uses STL in the same process, 
i.e. it will receive coverage feedback from STL functions. We don't want to mix coverage data from the library under test and the fuzzer itself. 

================
Comment at: lib/Fuzzer/FuzzerUtil.cpp:56
@@ +55,3 @@
+  memset(&sigact, 0, sizeof(sigact));
+  sigact.sa_sigaction = AlarmHandler;
+  Res = sigaction(SIGALRM, &sigact, 0);
----------------
ygribov wrote:
> Don't you need SA_SIGINFO and perhaps SA_ONSTACK here?
What for? For now all I need it to die on timeout. 
I may need these later, sure.

http://reviews.llvm.org/D7184

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/