Re: [PATCH v4 15/20] fuzz: add fuzzer skeleton

[Alexander Bulekov]

On 11/7/19 7:55 AM, Stefan Hajnoczi wrote:
> On Wed, Oct 30, 2019 at 02:50:00PM +0000, Oleinik, Alexander wrote:
> > diff --git a/tests/fuzz/fuzz.c b/tests/fuzz/fuzz.c
> > new file mode 100644
> > index 0000000000..0e38f81c48
> > --- /dev/null
> > +++ b/tests/fuzz/fuzz.c
> > @@ -0,0 +1,177 @@
> > +/*
> > + * fuzzing driver
> > + *
> > + * Copyright Red Hat Inc., 2019
> > + *
> > + * Authors:
> > + *  Alexander Bulekov   <address@hidden>
>
> Bulekov instead of Oleinik?
Yes I changed my last name and the approval from the court finally came 
through last week :)
I'll make sure its consistent across v5.

> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
>
> stdio.h and stdlib.h are already included by qemu/osdep.h.
>
> > +/* Executed for each fuzzing-input */
> > +int LLVMFuzzerTestOneInput(const unsigned char *Data, size_t Size)
> > +{
> > +    if (fuzz_target->fuzz) {
>
> Will this ever be NULL?
I'll remove the check

> > +        fuzz_target->fuzz(fuzz_qts, Data, Size);
> > +    }
> > +    return 0;
> > +}
> > +
> > +/* Executed once, prior to fuzzing */
> > +int LLVMFuzzerInitialize(int *argc, char ***argv, char ***envp)
> > +{
> > +
> > +    char *target_name;
> > +
> > +    /* Initialize qgraph and modules */
> > +    qos_graph_init();
> > +    module_call_init(MODULE_INIT_FUZZ_TARGET);
> > +    module_call_init(MODULE_INIT_QOM);
> > +    module_call_init(MODULE_INIT_LIBQOS);
> > +
> > +    if (*argc <= 1) {
> > +        usage(**argv);
> > +    }
> > +
> > +    /* Identify the fuzz target */
> > +    target_name = (*argv)[1];
> > +    if (!strstr(target_name, "--fuzz-target=")) {
> > +        usage(**argv);
> > +    }
> > +
> > +    target_name += strlen("--fuzz-target=");
> > +
> > +    fuzz_target = fuzz_get_target(target_name);
> > +    if (!fuzz_target) {
> > +        usage(**argv);
> > +    }
> > +
> > +    fuzz_qts = qtest_setup();
> > +
> > +    if (!fuzz_target) {
>
> This is dead code since fuzz_target was already checked above.  Please
> remove this if statement.
>
> > +        fprintf(stderr, "Error: Fuzz fuzz_target name %s not found\n",
> > +                target_name);
> > +        usage(**argv);
> > +    }
> > +
> > +    if (fuzz_target->pre_vm_init) {
> > +        fuzz_target->pre_vm_init();
> > +    }
> > +
> > +    /* Run QEMU's softmmu main with the fuzz-target dependent arguments */
> > +    char *init_cmdline = fuzz_target->get_init_cmdline(fuzz_target);
>
> Where is init_cmdline freed or should this be const char *?
>
> > +    wordexp_t result;
> > +    wordexp(init_cmdline, &result, 0);
>
> What is the purpose of word expansion here?
The fuzz target devs can specify arguments in a single string and not 
worry about calculating the argc and **argv - we take care of it for them.

> > +
> > +    qemu_init(result.we_wordc, result.we_wordv, NULL);
> > +
> > +    if (fuzz_target->pre_fuzz) {
> > +        fuzz_target->pre_fuzz(fuzz_qts);
> > +    }
> > +
> > +    return 0;
> > +}
> > diff --git a/tests/fuzz/fuzz.h b/tests/fuzz/fuzz.h
> > new file mode 100644
> > index 0000000000..b569b622d7
> > --- /dev/null
> > +++ b/tests/fuzz/fuzz.h
> > @@ -0,0 +1,66 @@
> > +/*
> > + * fuzzing driver
> > + *
> > + * Copyright Red Hat Inc., 2019
> > + *
> > + * Authors:
> > + *  Alexander Bulekov   <address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#ifndef FUZZER_H_
> > +#define FUZZER_H_
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/units.h"
> > +#include "qapi/error.h"
> > +#include "exec/memory.h"
> > +#include "tests/libqtest.h"
> > +
> > +
>
> Some documentation would be nice:
...
> Does the caller need to call g_free() on the returned string?  Please
> document this.
...
> s/to to/to/
...
> Please also mention that QEMU has been initialized at this point.
...
> "makes a copy of *target" -> does this mean the argument type can be
> const FuzzTarget *target?

Thanks - I made changes to address these.
-Alex

--
===
I recently changed my last name from Oleinik to Bulekov
===