1. Patchew
  2. QEMU
  3. View series

[PATCH] fuzz: Expect the cmdline in a freeable GString

Alexander Bulekov posted 1 patch 3 years, 9 months ago
Test checkpatch passed
Test docker-mingw@fedora passed
Test FreeBSD passed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200714174616.20709-1-alxndr@bu.edu
Maintainers: Thomas Huth <thuth@redhat.com>, Alexander Bulekov <alxndr@bu.edu>, Paolo Bonzini <pbonzini@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Bandan Das <bsd@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>
tests/qtest/fuzz/fuzz.c        | 13 ++++++-------
tests/qtest/fuzz/fuzz.h        |  6 +++---
tests/qtest/fuzz/i440fx_fuzz.c |  4 ++--
tests/qtest/fuzz/qos_fuzz.c    |  6 +++---
4 files changed, 14 insertions(+), 15 deletions(-)
[PATCH] fuzz: Expect the cmdline in a freeable GString
Posted by Alexander Bulekov 3 years, 9 months ago 
In the initial FuzzTarget, get_init_cmdline returned a char *. With this
API, we had no guarantee about where the string came from. For example,
i440fx-qtest-reboot-fuzz simply returned a pointer to a string literal,
while the QOS-based targets build the arguments out in a GString an
return the gchar *str pointer. Since we did not try to free the cmdline,
we have a leak for any targets that do not simply return string
literals. Clean up this mess by forcing fuzz-targets to return
a GString, that we can free.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/fuzz.c        | 13 ++++++-------
 tests/qtest/fuzz/fuzz.h        |  6 +++---
 tests/qtest/fuzz/i440fx_fuzz.c |  4 ++--
 tests/qtest/fuzz/qos_fuzz.c    |  6 +++---
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
index 0b66e43409..6bc17ef313 100644
--- a/tests/qtest/fuzz/fuzz.c
+++ b/tests/qtest/fuzz/fuzz.c
@@ -199,16 +199,15 @@ int LLVMFuzzerInitialize(int *argc, char ***argv, char ***envp)
     }
 
     /* Run QEMU's softmmu main with the fuzz-target dependent arguments */
-    const char *init_cmdline = fuzz_target->get_init_cmdline(fuzz_target);
-    init_cmdline = g_strdup_printf("%s -qtest /dev/null -qtest-log %s",
-                                   init_cmdline,
-                                   getenv("QTEST_LOG") ? "/dev/fd/2"
-                                                       : "/dev/null");
-
+    GString *cmd_line = fuzz_target->get_init_cmdline(fuzz_target);
+    g_string_append_printf(cmd_line,
+                           " -qtest /dev/null -qtest-log %s",
+                           getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null");
 
     /* Split the runcmd into an argv and argc */
     wordexp_t result;
-    wordexp(init_cmdline, &result, 0);
+    wordexp(cmd_line->str, &result, 0);
+    g_string_free(cmd_line, true);
 
     qemu_init(result.we_wordc, result.we_wordv, NULL);
 
diff --git a/tests/qtest/fuzz/fuzz.h b/tests/qtest/fuzz/fuzz.h
index 72d5710f6c..9ca3d107c5 100644
--- a/tests/qtest/fuzz/fuzz.h
+++ b/tests/qtest/fuzz/fuzz.h
@@ -50,10 +50,10 @@ typedef struct FuzzTarget {
 
 
     /*
-     * returns the arg-list that is passed to qemu/softmmu init()
-     * Cannot be NULL
+     * Returns the arguments that are passed to qemu/softmmu init(). Freed by
+     * the caller.
      */
-    const char* (*get_init_cmdline)(struct FuzzTarget *);
+    GString *(*get_init_cmdline)(struct FuzzTarget *);
 
     /*
      * will run once, prior to running qemu/softmmu init.
diff --git a/tests/qtest/fuzz/i440fx_fuzz.c b/tests/qtest/fuzz/i440fx_fuzz.c
index e2f31e56f9..bf966d478b 100644
--- a/tests/qtest/fuzz/i440fx_fuzz.c
+++ b/tests/qtest/fuzz/i440fx_fuzz.c
@@ -158,9 +158,9 @@ static void i440fx_fuzz_qos_fork(QTestState *s,
 
 static const char *i440fx_qtest_argv = TARGET_NAME " -machine accel=qtest"
                                        " -m 0 -display none";
-static const char *i440fx_argv(FuzzTarget *t)
+static GString *i440fx_argv(FuzzTarget *t)
 {
-    return i440fx_qtest_argv;
+    return g_string_new(i440fx_qtest_argv);
 }
 
 static void fork_init(void)
diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c
index 0c68f5361f..d52f3ebd83 100644
--- a/tests/qtest/fuzz/qos_fuzz.c
+++ b/tests/qtest/fuzz/qos_fuzz.c
@@ -66,7 +66,7 @@ void *qos_allocate_objects(QTestState *qts, QGuestAllocator **p_alloc)
     return allocate_objects(qts, current_path + 1, p_alloc);
 }
 
-static const char *qos_build_main_args(void)
+static GString *qos_build_main_args(void)
 {
     char **path = fuzz_path_vec;
     QOSGraphNode *test_node;
@@ -88,7 +88,7 @@ static const char *qos_build_main_args(void)
     /* Prepend the arguments that we need */
     g_string_prepend(cmd_line,
             TARGET_NAME " -display none -machine accel=qtest -m 64 ");
-    return cmd_line->str;
+    return cmd_line;
 }
 
 /*
@@ -189,7 +189,7 @@ static void walk_path(QOSGraphNode *orig_path, int len)
     g_free(path_str);
 }
 
-static const char *qos_get_cmdline(FuzzTarget *t)
+static GString *qos_get_cmdline(FuzzTarget *t)
 {
     /*
      * Set a global variable that we use to identify the qos_path for our
-- 
2.26.2
Re: [PATCH] fuzz: Expect the cmdline in a freeable GString
Posted by Darren Kenny 3 years, 9 months ago 
On Tuesday, 2020-07-14 at 13:46:16 -04, Alexander Bulekov wrote:
> In the initial FuzzTarget, get_init_cmdline returned a char *. With this
> API, we had no guarantee about where the string came from. For example,
> i440fx-qtest-reboot-fuzz simply returned a pointer to a string literal,
> while the QOS-based targets build the arguments out in a GString an
> return the gchar *str pointer. Since we did not try to free the cmdline,
> we have a leak for any targets that do not simply return string
> literals. Clean up this mess by forcing fuzz-targets to return
> a GString, that we can free.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

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

> ---
>  tests/qtest/fuzz/fuzz.c        | 13 ++++++-------
>  tests/qtest/fuzz/fuzz.h        |  6 +++---
>  tests/qtest/fuzz/i440fx_fuzz.c |  4 ++--
>  tests/qtest/fuzz/qos_fuzz.c    |  6 +++---
>  4 files changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
> index 0b66e43409..6bc17ef313 100644
> --- a/tests/qtest/fuzz/fuzz.c
> +++ b/tests/qtest/fuzz/fuzz.c
> @@ -199,16 +199,15 @@ int LLVMFuzzerInitialize(int *argc, char ***argv, char ***envp)
>      }
>  
>      /* Run QEMU's softmmu main with the fuzz-target dependent arguments */
> -    const char *init_cmdline = fuzz_target->get_init_cmdline(fuzz_target);
> -    init_cmdline = g_strdup_printf("%s -qtest /dev/null -qtest-log %s",
> -                                   init_cmdline,
> -                                   getenv("QTEST_LOG") ? "/dev/fd/2"
> -                                                       : "/dev/null");
> -
> +    GString *cmd_line = fuzz_target->get_init_cmdline(fuzz_target);
> +    g_string_append_printf(cmd_line,
> +                           " -qtest /dev/null -qtest-log %s",
> +                           getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null");
>  
>      /* Split the runcmd into an argv and argc */
>      wordexp_t result;
> -    wordexp(init_cmdline, &result, 0);
> +    wordexp(cmd_line->str, &result, 0);
> +    g_string_free(cmd_line, true);
>  
>      qemu_init(result.we_wordc, result.we_wordv, NULL);
>  
> diff --git a/tests/qtest/fuzz/fuzz.h b/tests/qtest/fuzz/fuzz.h
> index 72d5710f6c..9ca3d107c5 100644
> --- a/tests/qtest/fuzz/fuzz.h
> +++ b/tests/qtest/fuzz/fuzz.h
> @@ -50,10 +50,10 @@ typedef struct FuzzTarget {
>  
>  
>      /*
> -     * returns the arg-list that is passed to qemu/softmmu init()
> -     * Cannot be NULL
> +     * Returns the arguments that are passed to qemu/softmmu init(). Freed by
> +     * the caller.
>       */
> -    const char* (*get_init_cmdline)(struct FuzzTarget *);
> +    GString *(*get_init_cmdline)(struct FuzzTarget *);
>  
>      /*
>       * will run once, prior to running qemu/softmmu init.
> diff --git a/tests/qtest/fuzz/i440fx_fuzz.c b/tests/qtest/fuzz/i440fx_fuzz.c
> index e2f31e56f9..bf966d478b 100644
> --- a/tests/qtest/fuzz/i440fx_fuzz.c
> +++ b/tests/qtest/fuzz/i440fx_fuzz.c
> @@ -158,9 +158,9 @@ static void i440fx_fuzz_qos_fork(QTestState *s,
>  
>  static const char *i440fx_qtest_argv = TARGET_NAME " -machine accel=qtest"
>                                         " -m 0 -display none";
> -static const char *i440fx_argv(FuzzTarget *t)
> +static GString *i440fx_argv(FuzzTarget *t)
>  {
> -    return i440fx_qtest_argv;
> +    return g_string_new(i440fx_qtest_argv);
>  }
>  
>  static void fork_init(void)
> diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c
> index 0c68f5361f..d52f3ebd83 100644
> --- a/tests/qtest/fuzz/qos_fuzz.c
> +++ b/tests/qtest/fuzz/qos_fuzz.c
> @@ -66,7 +66,7 @@ void *qos_allocate_objects(QTestState *qts, QGuestAllocator **p_alloc)
>      return allocate_objects(qts, current_path + 1, p_alloc);
>  }
>  
> -static const char *qos_build_main_args(void)
> +static GString *qos_build_main_args(void)
>  {
>      char **path = fuzz_path_vec;
>      QOSGraphNode *test_node;
> @@ -88,7 +88,7 @@ static const char *qos_build_main_args(void)
>      /* Prepend the arguments that we need */
>      g_string_prepend(cmd_line,
>              TARGET_NAME " -display none -machine accel=qtest -m 64 ");
> -    return cmd_line->str;
> +    return cmd_line;
>  }
>  
>  /*
> @@ -189,7 +189,7 @@ static void walk_path(QOSGraphNode *orig_path, int len)
>      g_free(path_str);
>  }
>  
> -static const char *qos_get_cmdline(FuzzTarget *t)
> +static GString *qos_get_cmdline(FuzzTarget *t)
>  {
>      /*
>       * Set a global variable that we use to identify the qos_path for our
> -- 
> 2.26.2
Re: [PATCH] fuzz: Expect the cmdline in a freeable GString
Posted by Thomas Huth 3 years, 9 months ago 
On 14/07/2020 19.46, Alexander Bulekov wrote:
> In the initial FuzzTarget, get_init_cmdline returned a char *. With this
> API, we had no guarantee about where the string came from. For example,
> i440fx-qtest-reboot-fuzz simply returned a pointer to a string literal,
> while the QOS-based targets build the arguments out in a GString an
> return the gchar *str pointer. Since we did not try to free the cmdline,
> we have a leak for any targets that do not simply return string
> literals. Clean up this mess by forcing fuzz-targets to return
> a GString, that we can free.
> 
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  tests/qtest/fuzz/fuzz.c        | 13 ++++++-------
>  tests/qtest/fuzz/fuzz.h        |  6 +++---
>  tests/qtest/fuzz/i440fx_fuzz.c |  4 ++--
>  tests/qtest/fuzz/qos_fuzz.c    |  6 +++---
>  4 files changed, 14 insertions(+), 15 deletions(-)
      /* Prepend the arguments that we need */
>      g_string_prepend(cmd_line,
>              TARGET_NAME " -display none -machine accel=qtest -m 64 ");
> -    return cmd_line->str;
> +    return cmd_line;
>  }
>  
>  /*
> @@ -189,7 +189,7 @@ static void walk_path(QOSGraphNode *orig_path, int len)
>      g_free(path_str);
>  }
>  
> -static const char *qos_get_cmdline(FuzzTarget *t)
> +static GString *qos_get_cmdline(FuzzTarget *t)
>  {
>      /*
>       * Set a global variable that we use to identify the qos_path for our
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

... and queued to my "qtest-next" branch.

 Thanks,
  Thomas

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.