Hi Alex,
On Saturday, 2023-02-04 at 23:29:44 -05, Alexander Bulekov wrote:
> Signed-off-by: Alexander Bulekov <alx...@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.ke...@oracle.com>
Thanks,
Darren.
