[PATCH] D60334: Summary:Add close_fd_mask functionality to AFL driver.

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: compiler-rt/lib/fuzzer/afl/afl_driver.cpp:113
+// user asks to close stderr.
+__attribute__((weak, visibility("default"))) extern "C" void
+__sanitizer_set_report_fd(void*);
----------------
Do we need to set visibility here?


================
Comment at: compiler-rt/lib/fuzzer/afl/afl_driver.cpp:118
+// dup_and_close_stderr can use the correct one.
+FILE* output_file = stderr;
+
----------------
static


================
Comment at: compiler-rt/lib/fuzzer/afl/afl_driver.cpp:152
+// Most of these I/O functions were inspired by/copied from libFuzzer's code.
+void discard_output(int fd) {
+  FILE *temp = fopen("/dev/null", "w");
----------------
static - here and other private functions.


================
Comment at: compiler-rt/lib/fuzzer/afl/afl_driver.cpp:176
+    return;
+
+  __sanitizer_set_report_fd(reinterpret_cast<void *>(output_fd));
----------------
Nit:  Can we remove some of these blank lines?


================
Comment at: compiler-rt/lib/fuzzer/afl/afl_driver.cpp:181
+
+// TODO(metzman): Make the rest of this file obey LLVM's style and conventions.
+void Printf(const char *Fmt, ...) {
----------------
If you want to submit a follow-up patch, SGTM.  But lets leave this TODO out since it's only style-related.


================
Comment at: compiler-rt/lib/fuzzer/afl/afl_driver.cpp:199
+    dup_and_close_stderr();
+
+  if (fd_mask & 1)
----------------
Nit:  can we remove these blank lines?


================
Comment at: compiler-rt/test/fuzzer/AFLDriverCloseFdMaskTest.cpp:5
+
+// Contains dummy functions used to avoid dependency on AFL.
+#include <stdint.h>
----------------
Let's move this comment closer to the functions it refers to.


================
Comment at: compiler-rt/test/fuzzer/AFLDriverCloseFdMaskTest.cpp:16
+  if (Count--)
+    return 1;
+  return 0;
----------------
Do we really need this?  How about just making it `return 0` always?


================
Comment at: compiler-rt/test/fuzzer/AFLDriverCloseFdMaskTest.cpp:23
+extern "C" int LLVMFuzzerInitialize(int *argc, char ***argv) {
+  fprintf(stderr, "LLVMFuzzerInitialize called\n");
+  return 0;
----------------
Do we need this printf?


================
Comment at: compiler-rt/test/fuzzer/AFLDriverCloseFdMaskTest.cpp:29
+  puts("STDOUT MESSAGE");
+  fflush(0);
+  fprintf(stderr, "STDERR MESSAGE\n");
----------------
`fflush(stdout)`


================
Comment at: compiler-rt/test/fuzzer/AFLDriverCloseFdMaskTest.cpp:36
+  delete[] Array;
+  return Array[Size];
+}
----------------
Maybe just `return Data[Size]`?  Simpler and should still crash.


================
Comment at: compiler-rt/test/fuzzer/afl-driver-close-fd-mask.test:4
+
+RUN: echo "%no_fuzzer_cpp_compiler %S/AFLDriverCloseFdMaskTest.cpp %libfuzzer_src/afl/afl_driver.cpp -o %t-AFLDriverCloseFdMaskTest" > /tmp/cmd
+RUN: echo -n "abc" > %t.file1
----------------
Can we remove this line?


================
Comment at: compiler-rt/test/fuzzer/afl-driver-close-fd-mask.test:8
+
+RUN: echo "%libfuzzer_src/afl/afl_driver.cpp" > /tmp/src
+
----------------
Please remove


================
Comment at: compiler-rt/test/fuzzer/afl-driver-close-fd-mask.test:19
+CHECK1-NOT: STDOUT MESSAGE
+CHECK1: STDERR MESSAGE
+RUN: AFL_DRIVER_CLOSE_FD_MASK=1 %run %t-AFLDriverCloseFdMaskTest < %t.file1 2>&1 | FileCheck %s --check-prefix=CHECK1
----------------
Nit:  Some inconsistent formatting.  Can we keep the CHECKs right after the associated RUNs?  Rather than sometimes after and sometimes before.


================
Comment at: compiler-rt/test/fuzzer/afl-driver-close-fd-mask.test:36
+CHECK4: ERROR: AddressSanitizer
+RUN: AFL_DRIVER_CLOSE_FD_MASK=2 not %run %t-AFLDriverCloseFdMaskTest < %t.file2 2>&1  | FileCheck %s --check-prefix=CHECK4
+
----------------
Can we combine this into check2?


================
Comment at: compiler-rt/test/fuzzer/afl-driver-close-fd-mask.test:42
+CHECK5: ERROR: AddressSanitizer
+RUN: rm %t.stderr
+RUN: AFL_DRIVER_STDERR_DUPLICATE_FILENAME=%t.stderr AFL_DRIVER_CLOSE_FD_MASK=2 not %run %t-AFLDriverCloseFdMaskTest < %t.file2
----------------
I think this test will fail if the file doesn't exist.  Better make it `rm -f`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60334