[libc-commits] [PATCH] D74091: [libc] Lay out framework for fuzzing libc functions.

[Paula Toth via Phabricator via libc-commits]

PaulkaToast marked an inline comment as done.
PaulkaToast added inline comments.


================
Comment at: libc/fuzzing/string/strcpy_fuzz.cpp:25-27
+  if (strcmp(dest, src) != 0) {
+    abort();
+  }
----------------
sivachandra wrote:
> sivachandra wrote:
> > abrachet wrote:
> > > PaulkaToast wrote:
> > > > abrachet wrote:
> > > > > Is this not `assert(strcmp(dest, src))` because you think `NDEBUG` might be defined for this file?
> > > > oss-fuzz compiles with optimization -o3 enabled. Does NDEBUG get defined with that level of optimization? If it does then assert will not crash the fuzzer as expected. 
> > > @sivachandra said to avoid `abort`, so that would mean avoid `assert` too, we could change this `abort` to `__builtin_trap`.
> > > 
> > > FWIW https://godbolt.org/z/khdC4E
> > I am not really against using `abort` but against including `stdlib.h`. So, we can create an abort wrapper in `utils/CPP` which allows us to avoid including `stdlib.h`.
> > 
> > But, if `exit` is disallowed in a fuzz target, is `abort` OK? FWIW, libcxx's fuzz tests seem to prefer `assert` as @abrachet suggested.
> > 
> > Also, asking for my own knowledge: Should one care about correctness in a fuzz test? Correctness is important of course, but is that the goal of a fuzz test?
> Just correcting myself: If we conclude we need `abort`/`assert`, then we should put their wrappers in a fuzzing specific util-library and not in `utils/CPP`.
LibFuzzer's [[ https://llvm.org/docs/LibFuzzer.html#id29 | documentation ]] makes use of ` __builtin_trap` so I'm replacing `abort` calls. 

As for correctness note, I believe that that there is a usefulness to having correctness also be a goal of the fuzz test. Here are some [[ https://afl-1.readthedocs.io/en/latest/fuzzing.html#going-beyond-crashes | reason ]] for this practice we might consider? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74091