| Message ID | 20230205042951.3570008-4-alxndr@bu.edu |
|---|---|
| State | New |
| Headers | show |
| Series | Retire Fork-Based Fuzzing | expand |
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.
On 230213 1426, Darren Kenny wrote: > 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? Yes. The fuzzer should handle timeouts directly now. -Alex
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); -} - /* * 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); } static void usage(void) @@ -825,7 +772,6 @@ static void generic_pre_fuzz(QTestState *s) { GHashTableIter iter; MemoryRegion *mr; - QPCIBus *pcibus; char **result; GString *name_pattern; @@ -883,12 +829,6 @@ static void generic_pre_fuzz(QTestState *s) printf("No fuzzable memory regions found...\n"); exit(1); } - - pcibus = qpci_new_pc(s, NULL); - g_ptr_array_foreach(fuzzable_pci_devices, pci_enum, pcibus); - qpci_free_pc(pcibus); - - counter_shm_init(); } /*
Signed-off-by: Alexander Bulekov <alxndr@bu.edu> --- tests/qtest/fuzz/generic_fuzz.c | 106 +++++++------------------------- 1 file changed, 23 insertions(+), 83 deletions(-)