Re: [PATCH 03/10] fuzz/generic-fuzz: use reboots instead of forks to reset state

[Darren Kenny]

Hi Alex,

On Saturday, 2023-02-04 at 23:29:44 -05, Alexander Bulekov wrote:
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  tests/qtest/fuzz/generic_fuzz.c | 106 +++++++-------------------------
>  1 file changed, 23 insertions(+), 83 deletions(-)
>
> diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
> index 7326f6840b..c2e5642150 100644
> --- a/tests/qtest/fuzz/generic_fuzz.c
> +++ b/tests/qtest/fuzz/generic_fuzz.c
> @@ -18,7 +18,6 @@
>  #include "tests/qtest/libqtest.h"
>  #include "tests/qtest/libqos/pci-pc.h"
>  #include "fuzz.h"
> -#include "fork_fuzz.h"
>  #include "string.h"
>  #include "exec/memory.h"
>  #include "exec/ramblock.h"
> @@ -29,6 +28,8 @@
>  #include "generic_fuzz_configs.h"
>  #include "hw/mem/sparse-mem.h"
>  
> +static void pci_enum(gpointer pcidev, gpointer bus);
> +
>  /*
>   * SEPARATOR is used to separate "operations" in the fuzz input
>   */
> @@ -589,30 +590,6 @@ static void op_disable_pci(QTestState *s, const unsigned 
> char *data, size_t len)
>      pci_disabled = true;
>  }
>  
> -static void handle_timeout(int sig)
> -{
> -    if (qtest_log_enabled) {
> -        fprintf(stderr, "[Timeout]\n");
> -        fflush(stderr);
> -    }
> -
> -    /*
> -     * If there is a crash, libfuzzer/ASAN forks a child to run an
> -     * "llvm-symbolizer" process for printing out a pretty stacktrace. It
> -     * communicates with this child using a pipe.  If we timeout+Exit, while
> -     * libfuzzer is still communicating with the llvm-symbolizer child, we 
> will
> -     * be left with an orphan llvm-symbolizer process. Sometimes, this 
> appears
> -     * to lead to a deadlock in the forkserver. Use waitpid to check if there
> -     * are any waitable children. If so, exit out of the signal-handler, and
> -     * let libfuzzer finish communicating with the child, and exit, on its 
> own.
> -     */
> -    if (waitpid(-1, NULL, WNOHANG) == 0) {
> -        return;
> -    }
> -
> -    _Exit(0);
> -}
> -
>  /*
>

I'm presuming that the timeout is being left to the fuzz orchestrator
now, rather than us managing it directly in our own way?

>   * Here, we interpret random bytes from the fuzzer, as a sequence of 
> commands.
>   * Some commands can be variable-width, so we use a separator, SEPARATOR, to
> @@ -669,64 +646,34 @@ static void generic_fuzz(QTestState *s, const unsigned 
> char *Data, size_t Size)
>      size_t cmd_len;
>      uint8_t op;
>  
> -    if (fork() == 0) {
> -        struct sigaction sact;
> -        struct itimerval timer;
> -        sigset_t set;
> -        /*
> -         * Sometimes the fuzzer will find inputs that take quite a long time 
> to
> -         * process. Often times, these inputs do not result in new coverage.
> -         * Even if these inputs might be interesting, they can slow down the
> -         * fuzzer, overall. Set a timeout for each command to avoid hurting
> -         * performance, too much
> -         */
> -        if (timeout) {
> -
> -            sigemptyset(&sact.sa_mask);
> -            sact.sa_flags   = SA_NODEFER;
> -            sact.sa_handler = handle_timeout;
> -            sigaction(SIGALRM, &sact, NULL);
> -
> -            sigemptyset(&set);
> -            sigaddset(&set, SIGALRM);
> -            pthread_sigmask(SIG_UNBLOCK, &set, NULL);
> -
> -            memset(&timer, 0, sizeof(timer));
> -            timer.it_value.tv_sec = timeout / USEC_IN_SEC;
> -            timer.it_value.tv_usec = timeout % USEC_IN_SEC;
> -        }
> +    op_clear_dma_patterns(s, NULL, 0);
> +    pci_disabled = false;
>  
> -        op_clear_dma_patterns(s, NULL, 0);
> -        pci_disabled = false;
> +    QPCIBus *pcibus = qpci_new_pc(s, NULL);
> +    g_ptr_array_foreach(fuzzable_pci_devices, pci_enum, pcibus);
> +    qpci_free_pc(pcibus);
>  
> -        while (cmd && Size) {
> -            /* Reset the timeout, each time we run a new command */
> -            if (timeout) {
> -                setitimer(ITIMER_REAL, &timer, NULL);
> -            }
> +    while (cmd && Size) {
> +        /* Reset the timeout, each time we run a new command */
>  
> -            /* Get the length until the next command or end of input */
> -            nextcmd = memmem(cmd, Size, SEPARATOR, strlen(SEPARATOR));
> -            cmd_len = nextcmd ? nextcmd - cmd : Size;
> +        /* Get the length until the next command or end of input */
> +        nextcmd = memmem(cmd, Size, SEPARATOR, strlen(SEPARATOR));
> +        cmd_len = nextcmd ? nextcmd - cmd : Size;
>  
> -            if (cmd_len > 0) {
> -                /* Interpret the first byte of the command as an opcode */
> -                op = *cmd % (sizeof(ops) / sizeof((ops)[0]));
> -                ops[op](s, cmd + 1, cmd_len - 1);
> +        if (cmd_len > 0) {
> +            /* Interpret the first byte of the command as an opcode */
> +            op = *cmd % (sizeof(ops) / sizeof((ops)[0]));
> +            ops[op](s, cmd + 1, cmd_len - 1);
>  
> -                /* Run the main loop */
> -                flush_events(s);
> -            }
> -            /* Advance to the next command */
> -            cmd = nextcmd ? nextcmd + sizeof(SEPARATOR) - 1 : nextcmd;
> -            Size = Size - (cmd_len + sizeof(SEPARATOR) - 1);
> -            g_array_set_size(dma_regions, 0);
> +            /* Run the main loop */
> +            flush_events(s);
>          }
> -        _Exit(0);
> -    } else {
> -        flush_events(s);
> -        wait(0);
> +        /* Advance to the next command */
> +        cmd = nextcmd ? nextcmd + sizeof(SEPARATOR) - 1 : nextcmd;
> +        Size = Size - (cmd_len + sizeof(SEPARATOR) - 1);
> +        g_array_set_size(dma_regions, 0);
>      }
> +    fuzz_reboot(s);
>

Guess this should be changed too if the declared function is too.

These are only nits, so:

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren.