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

[Alexander Bulekov <alxndr@bu.edu>]

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   <alxndr@bu.edu>
> 
> 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   <alxndr@bu.edu>
>> + *
>> + * 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
===