[Fuzzer] Afl driver changing iterations handling

[devnexen]

Summary

Handling differently the iterations with type limit and eventually error message.

[morehouse]

lib/fuzzer/afl/afl_driver.cpp

309 ↗

(On Diff #150310)

Maybe next_char would be a more descriptive name here.

314 ↗

(On Diff #150310)

Would it make more sense to just exit if iterations is invalid?

315 ↗

(On Diff #150310)

Since the if-block returns, else is unnecessary here.

316 ↗

(On Diff #150310)

Please use static_cast here.

[devnexen]

lib/fuzzer/afl/afl_driver.cpp

316 ↗

(On Diff #150310)

I usually prefer indeed but was not sure if you prefered C's cast style. Ok will address all points.

[kcc]

is this testable (somewhere in test/fuzzer/afl-driver*)?

[devnexen]

To avoid breaking tests, not exiting directly but keeping the default to 1000.

[kcc]

test/fuzzer/afl-driver.test

31 ↗

(On Diff #150414)

if you use "not" in the test command line you can have a test like that is expected to break

[devnexen]

ping

[morehouse]

test/fuzzer/afl-driver.test

25 ↗

(On Diff #150422)

Why have you changed these two lines?

[devnexen]

test/fuzzer/afl-driver.test

25 ↗

(On Diff #150422)

Most likely because of the exit call. Without this, the previous test passes.

[morehouse]

test/fuzzer/afl-driver.test

25 ↗

(On Diff #150422)

If I understand the code correctly, this should cause N to default to 1000. So I don't think exit should be called.

[devnexen]

test/fuzzer/afl-driver.test

25 ↗

(On Diff #150422)

If I change the signature of set_iterations to return a value and bring back the else if (argc == 2 && ...) as it was the test passes somehow.

[morehouse]

test/fuzzer/afl-driver.test

25 ↗

(On Diff #150422)

Ah, I see. The driver doesn't support specifying N with the deprecated call style. Let's move the deprecation warning to before the call to set_iterations, and change the above test to check for the deprecation warning + the iterations invalid warning.

[devnexen]

test/fuzzer/afl-driver.test

25 ↗

(On Diff #150422)

I changed it as asked and realised it indeed exited, i.e the second warning (i.e iterations invalid) displays the path of the file as argument making the conversion to number failing for sure.

[metzman]

Reposting my comments here from https://reviews.llvm.org/D49141

I think this patch should be reverted.
It breaks afl_driver's command line interface by causing invocations such as ./fuzzer inputfile to fail.
I don't think this breakage was intentional since ./fuzzer inputfile1 inputfile2 still works and I don't think there is a reason to break this.
I've created a revert here: https://reviews.llvm.org/D49141

lib/fuzzer/afl/afl_driver.cpp

335 ↗

(On Diff #150422)

I don't think (N = atoi(argv[1])) > 0) should have been removed.
It allowed a single file to be passed to afl_driver.

test/fuzzer/afl-driver.test

25 ↗

(On Diff #150422)

I think changing this test was incorrect. The test verified that the command line interface worked a certain way.
Thus the test WAI if this patch causes it to fail.

compiler-rt/trunk/lib/fuzzer/afl/afl_driver.cpp

 #include <signal.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/resource.h>
 #include <sys/time.h>
 #include <unistd.h>
+#include <limits.h>
 
 #include <fstream>
 #include <iostream>
 #include <vector>
 
 // Platform detection. Copied from FuzzerInternal.h
 #ifdef __linux__
 #define LIBFUZZER_LINUX 1
for (int i = 1; i < argc; i++) {
     assert(in);
     LLVMFuzzerTestOneInput(reinterpret_cast<const uint8_t *>(bytes.data()),
                            bytes.size());
     std::cout << "Execution successful" << std::endl;
   }
   return 0;
 }
 
+static void set_iterations(int *N, const char *arg) {
+  char *next_char;
+  long NL = strtol(arg, &next_char, 10);
+  if (NL < 1 || NL > INT_MAX || *next_char != '\0') {
+    fprintf(stderr, "WARNING: iterations invalid `%s`\n",
+        arg);
+    ::exit(-1);
+  }
+
+  *N = static_cast<int>(NL);
+}
+
 int main(int argc, char **argv) {
   fprintf(stderr,
       "======================= INFO =========================\n"
       "This binary is built for AFL-fuzz.\n"
       "To run the target function on individual input(s) execute this:\n"
       "  %s < INPUT_FILE\n"
       "or\n"
       "  %s INPUT_FILE1 [INPUT_FILE2 ... ]\n"
int main(int argc, char **argv) {
   maybe_duplicate_stderr();
   maybe_initialize_extra_stats();
 
   if (!getenv("AFL_DRIVER_DONT_DEFER"))
     __afl_manual_init();
 
   int N = 1000;
   if (argc == 2 && argv[1][0] == '-')
-      N = atoi(argv[1] + 1);
-  else if(argc == 2 && (N = atoi(argv[1])) > 0)
-      fprintf(stderr, "WARNING: using the deprecated call style `%s %d`\n",
-              argv[0], N);
-  else if (argc > 1)
+    set_iterations(&N, argv[1] + 1);
+  else if(argc == 2) {
+    set_iterations(&N, argv[1]);
+  } else if (argc > 1)
     return ExecuteFilesOnyByOne(argc, argv);
 
   assert(N > 0);
 
   // Call LLVMFuzzerTestOneInput here so that coverage caused by initialization
   // on the first execution of LLVMFuzzerTestOneInput is ignored.
   uint8_t dummy_input[1] = {0};
   LLVMFuzzerTestOneInput(dummy_input, 1);

compiler-rt/trunk/test/fuzzer/afl-driver.test

 
 
 RUN: %run %t-AFLDriverTest < %t.file3 666 2>&1 | FileCheck %s --check-prefix=CHECK3
 CHECK3: WARNING: using the deprecated call style
 CHECK3: __afl_persistent_loop calle, Count = 666
 CHECK3: LLVMFuzzerTestOneInput called; Size = 3
 
 
-RUN: %run %t-AFLDriverTest %t.file3 2>&1 | FileCheck %s --check-prefix=CHECK4
-CHECK4: LLVMFuzzerTestOneInput called; Size = 3
+RUN: not %run %t-AFLDriverTest %t.file3 2>&1 | FileCheck %s --check-prefix=CHECK4
+CHECK4: WARNING: using the deprecated call style `{{.*}} 1000`
+CHECK4: WARNING: iterations invalid `{{.*}}`
 
 RUN: %run %t-AFLDriverTest %t.file3 %t.file4  2>&1 | FileCheck %s --check-prefix=CHECK5
 CHECK5: LLVMFuzzerTestOneInput called; Size = 3
 CHECK5: LLVMFuzzerTestOneInput called; Size = 4
+
+RUN: not %run %t-AFLDriverTest < %t.file3 --1 2>&1 | FileCheck %s --check-prefix=CHECK6
+CHECK6: WARNING: iterations invalid `-1`
+
+RUN: not %run %t-AFLDriverTest < %t.file3 -Invalid 2>&1 | FileCheck %s --check-prefix=CHECK7
+CHECK7: WARNING: iterations invalid `Invalid`