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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

On Wed, Sep 18, 2019 at 11:19:43PM +0000, Oleinik, Alexander wrote:
> +void set_fuzz_target_args(int argc, char **argv)
> +{
> +    if (fuzz_target) {
> +        fuzz_target->main_argc = argc;
> +        fuzz_target->main_argv = argv;
> +    }
> +}

Why calls this and why?

> +
> +void reboot(QTestState *s)
> +{
> +    qemu_system_reset(SHUTDOWN_CAUSE_GUEST_RESET);
> +}

Why does reboot() take an unused argument?

> +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.

> +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.

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;

  void fuzz_register_target(const FuzzTarget *target);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]