Re: [Qemu-devel] [PATCH v3 16/22] fuzz: add fuzzer skeleton

["Oleinik, Alexander" <alxndr@bu.edu>]

On Thu, 2019-09-19 at 13:48 +0100, Stefan Hajnoczi wrote:

> > +
> > +void reboot(QTestState *s)
> > +{
> > +    qemu_system_reset(SHUTDOWN_CAUSE_GUEST_RESET);
> > +}
> 
> Why does reboot() take an unused argument?
It was needed when I had a reset_state(s) pointer which was separate
from fuzz(). Since fuzz() is responsible for initializing and resetting
state now, I'll remove it.

> 
> > +static void usage(char *path)
> > +{
> > +    printf("Usage: %s --FUZZ_TARGET [LIBFUZZER ARGUMENTS]\n",
> > path);
> > +    printf("where --FUZZ_TARGET is one of:\n");
> 
> Is the "--" prefix a libfuzzer requirement?  I would have expected
> either FUZZ_TARGET by itself or --fuzz-target=FUZZ_TARGET (a properly
> formatted long option) so that collisions with other command-line
> options are not possible.
Yes libfuzzer will only pass arguments that start with "--". I can
replace it with --fuzz-target=FUZZ_TARGET. Alternatively, I can try to
build separate binaries for each target. It might waste disk space, but
we wouldn't need arguments (--trace could be replace with TRACE=1 in
ENV). With this design, I'm not sure what to do with code such as
i440fx_fuzz.c which re-purposes some functions for multiple different
fuzz targets.

> > +typedef struct FuzzTarget {
> > +    GString *name;
> > +    GString *description;
> > +    void(*pre_main)(void);
> > +    void(*pre_fuzz)(QTestState *);
> > +    void(*fuzz)(QTestState *, const unsigned char *, size_t);
> > +    int main_argc;
> > +    char **main_argv;
> > +} FuzzTarget;
> > +
> > +void set_fuzz_target_args(int argc, char **argv);
> > +void reboot(QTestState *);
> > +void fuzz_add_target(const char *name, const char *description,
> > FuzzTarget
> > +        *target);
> 
> This is a strange API.  I can't make sense of the struct,
> fuzz_add_target(), and set_fuzz_target_args() without reading the
> implementation:
> 
> 1. set_fuzz_target_args() implies that there is global state but then
>    FuzzTarget also has main_argc and main_argv fields.  I'm not sure
>    what the relationship is.
This is essentially there for the QOS support. For QOS targets, when
running fuzz_add_qos_target(), we don't yet know argc and argv, since
that requires walking the QOSGraph. When we have populated the
FuzzTargetList, QOSGraph and parsed the --FUZZ_TARGET, we set global
FuzzTarget and check against it while walking the QOSGraph. When we
find the matching path, we then know the argc/argv and can set them for
the global fuzz_target. Since qos_fuzz.c still needs major work due to
all of the duplicated code, I'll take another stab at this. Maybe we
can identify the argc/argv immediately when we add the fuzz target node
to the QOSGraph. This is another case for "each target has its own
binary", since that could avoid much of this complexity, since we
wouldn't need the fuzz_target_list.

> 2. fuzz_add_target() takes name and description as arguments but
> expects
>    the caller to populate the struct arg's pre_main(), pre_fuzz(),
>    fuzz() function pointers.  This is inconsistent and undocumented.
> 
> A cleaner API:
> 
>   /* Each fuzz target implements the following interface: */
>   typedef struct {
>       const char *name;        /* command-line option for this target
> */
>       const char *description; /* human-readable help text */
> 
>       /* TODO documentation */
>       void (*pre_main)(void);
> 
>       /* TODO documentation */
>       void (*pre_fuzz)(QTestState *);
> 
>       /* TODO documentation */
>       void (*fuzz)(QTestState *, const unsigned char *, size_t);
>   } FuzzTarget;

Sounds good. Should there also be argc and argv here? 

>   void fuzz_register_target(const FuzzTarget *target);