Re: [PATCH 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Jun 24, 2024 at 11:55 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 21.06.2024 21:14, Tamas K Lengyel wrote:
> > @@ -58,6 +58,9 @@ afl-harness: afl-harness.o $(OBJS) cpuid.o wrappers.o
> >  afl-harness-cov: afl-harness-cov.o $(patsubst %.o,%-cov.o,$(OBJS)) cpuid.o 
> > wrappers.o
> >       $(CC) $(CFLAGS) $(GCOV_FLAGS) $(addprefix 
> > -Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@
> >
> > +libfuzzer-harness: $(OBJS) cpuid.o
> > +     $(CC) $(CFLAGS) $(LIB_FUZZING_ENGINE) -fsanitize=fuzzer $^ -o $@
>
> What is LIB_FUZZING_ENGINE? I don't think we have any use of that in the
> tree anywhere.

It's used by oss-fuzz, otherwise it's not doing anything.

>
> I'm further surprised you get away here without wrappers.o.

Wrappers.o was actually breaking the build for oss-fuzz at the linking
stage. It works just fine without it.

>
> Finally, despite its base name the lack of an extension suggest to me
> this isn't actually a library. Can you help me bring both aspects together?

Libfuzzer is the the name of the fuzzing engine, like afl:
https://llvm.org/docs/LibFuzzer.html

>
> > @@ -67,7 +70,7 @@ distclean: clean
> >
> >  .PHONY: clean
> >  clean:
> > -     rm -f *.a *.o $(DEPS_RM) afl-harness afl-harness-cov *.gcda *.gcno 
> > *.gcov
> > +     rm -f *.a *.o $(DEPS_RM) afl-harness afl-harness-cov *.gcda *.gcno 
> > *.gcov libfuzzer-harness
>
> I'm inclined to suggest it's time to split this line (e.g. keeping all the
> wildcard patterns together and moving the rest to a new rm invocation).

Sure.

>
> > --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> > +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> > @@ -906,14 +906,12 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, 
> > size_t size)
> >
> >      if ( size <= DATA_OFFSET )
> >      {
> > -        printf("Input too small\n");
> > -        return 1;
> > +        return -1;
> >      }
> >
> >      if ( size > FUZZ_CORPUS_SIZE )
> >      {
> > -        printf("Input too large\n");
> > -        return 1;
> > +        return -1;
> >      }
> >
> >      memcpy(&input, data_p, size);
>
> This part of the change clearly needs explaining in the description.
> It's not even clear to me in how far this is related to the purpose
> of the patch here (iow it may want to be a separate change, depending
> on why the change is needed).

The printf simply produces a ton of unnecessary output while the
fuzzer is running, slowing it down. It's also not useful at all, even
for debugging. Switching the return -1 is necessary because beside
0/-1 values are reserved by libfuzzer for "future use". No issue about
putting this info into the commit message.

Tamas