Re: [PATCH] fuzz: Fix leak when assembling datadir path string

[Alexander Bulekov]

On 200717 1847, Thomas Huth wrote:
> On 17/07/2020 18.35, Alexander Bulekov wrote:
> > We freed the string containing the final datadir path, but did not free
> > the path to the executable's directory that we get from
> > g_path_get_dirname(). Fix that.
> > 
> > Reported-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > ---
> > 
> > I ran it with Thomas' fixed build-oss-fuzz job:
> > https://gitlab.com/a1xndr/qemu/-/jobs/644463736
> 
> Looks like the fuzzer triggered a crash there, see line 5850 ...
> shouldn't the job fail in that case? ... i.e. is the fuzzer still
> exiting with return code 0?

Ah. We run each input in a forked process. If the child crashes, the
parent can continue forking+fuzzing, as if nothing happened. This also
unfortunately means that the job might succeed even if there is a crash
in the actual fuzz target code, as long as the error only happens in the
child processes. Maybe we could add an env variable to have the parent
exit -1 if the child crashes, but then the job would fail even for
non-fuzzer issues (such as this virtio-net crash).

-Alex

> 
> > diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
> > index 6bc17ef313..031594a686 100644
> > --- a/tests/qtest/fuzz/fuzz.c
> > +++ b/tests/qtest/fuzz/fuzz.c
> > @@ -143,7 +143,7 @@ int LLVMFuzzerInitialize(int *argc, char ***argv, char 
> > ***envp)
> >  {
> >  
> >      char *target_name;
> > -    char *dir;
> > +    char *bindir, *datadir;
> >      bool serialize = false;
> >  
> >      /* Initialize qgraph and modules */
> > @@ -164,11 +164,13 @@ int LLVMFuzzerInitialize(int *argc, char ***argv, 
> > char ***envp)
> >           * location of the executable. Using this we add exec_dir/pc-bios 
> > to
> >           * the datadirs.
> >           */
> > -        dir = g_build_filename(g_path_get_dirname(**argv), "pc-bios", 
> > NULL);
> > -        if (g_file_test(dir, G_FILE_TEST_IS_DIR)) {
> > -            qemu_add_data_dir(dir);
> > +        bindir = g_path_get_dirname(**argv);
> > +        datadir = g_build_filename(bindir, "pc-bios", NULL);
> > +        g_free(bindir);
> > +        if (g_file_test(datadir, G_FILE_TEST_IS_DIR)) {
> > +            qemu_add_data_dir(datadir);
> >          }
> > -        g_free(dir);
> > +        g_free(datadir);
> >      } else if (*argc > 1) {  /* The target is specified as an argument */
> >          target_name = (*argv)[1];
> >          if (!strstr(target_name, "--fuzz-target=")) {
> > 
> 
> Patch looks fine, thanks!
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>