diff mbox series

[3/3] qga: test: Add tests for `merge-output` flag

Message ID 2d68032724822784750df4edc4df0c578c878871.1677197937.git.dxu@dxuuu.xyz
State New
Headers show
Series qga: Add optional `merge-output` flag to guest-exec QAPI | expand

Commit Message

Daniel Xu Feb. 24, 2023, 12:34 a.m. UTC 
This commit adds a test to ensure `merge-output` functions as expected.
We also add a negative test to ensure we haven't regressed previous
functionality.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 tests/unit/test-qga.c | 128 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 111 insertions(+), 17 deletions(-)

Comments

Marc-André Lureau Feb. 27, 2023, 8:40 a.m. UTC | #1 
Hi

On Fri, Feb 24, 2023 at 8:31 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> This commit adds a test to ensure `merge-output` functions as expected.
> We also add a negative test to ensure we haven't regressed previous
> functionality.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>

Please check your patch with ASAN, you have use after-free introduced
by this change:
==664972==ERROR: AddressSanitizer: heap-use-after-free on address
0x621000135028 at pc 0x55e617a38b39 bp 0x7fff7fe85390 sp
0x7fff7fe85388
READ of size 8 at 0x621000135028 thread T0
    #0 0x55e617a38b38 in qdict_find ../qobject/qdict.c:96
    #1 0x55e617a39bea in qdict_get ../qobject/qdict.c:164
    #2 0x55e617a39bea in qdict_get_int ../qobject/qdict.c:209
    #3 0x55e6179e2519 in test_qga_guest_exec ../tests/unit/test-qga.c:807
    #4 0x7fbaa499dc7d in g_test_run_suite_internal
(/lib64/libglib-2.0.so.0+0x84c7d)
    #5 0x7fbaa499d9e4 in g_test_run_suite_internal
(/lib64/libglib-2.0.so.0+0x849e4)
    #6 0x7fbaa499e181 in g_test_run_suite (/lib64/libglib-2.0.so.0+0x85181)
    #7 0x7fbaa49966ec in g_test_run (/lib64/libglib-2.0.so.0+0x7d6ec)
    #8 0x55e6179da0ac in main ../tests/unit/test-qga.c:1083
    #9 0x7fbaa384a50f in __libc_start_call_main (/lib64/libc.so.6+0x2750f)
    #10 0x7fbaa384a5c8 in __libc_start_main@GLIBC_2.2.5
(/lib64/libc.so.6+0x275c8)
    #11 0x55e6179daf44 in _start
(/home/elmarco/src/qemu/build/tests/unit/test-qga+0x1bbf44)

thanks
> ---
>  tests/unit/test-qga.c | 128 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 111 insertions(+), 17 deletions(-)
>
> diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
> index 9d8e1d1cd3..0b3966024c 100644
> --- a/tests/unit/test-qga.c
> +++ b/tests/unit/test-qga.c
> @@ -755,6 +755,31 @@ static void test_qga_fsfreeze_status(gconstpointer fix)
>      g_assert_cmpstr(status, ==, "thawed");
>  }
>
> +static QDict *wait_for_guest_exec_completion(int fd, int64_t pid)
> +{
> +    QDict *ret = NULL;
> +    int64_t now;
> +    bool exited;
> +    QDict *val;
> +
> +    now = g_get_monotonic_time();
> +    do {
> +        ret = qmp_fd(fd,
> +                     "{'execute': 'guest-exec-status',"
> +                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
> +        g_assert_nonnull(ret);
> +        val = qdict_get_qdict(ret, "return");
> +        exited = qdict_get_bool(val, "exited");
> +        if (!exited) {
> +            qobject_unref(ret);
> +        }
> +    } while (!exited &&
> +             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
> +    g_assert(exited);
> +
> +    return ret;
> +}
> +
>  static void test_qga_guest_exec(gconstpointer fix)
>  {
>      const TestFixture *fixture = fix;
> @@ -762,9 +787,8 @@ static void test_qga_guest_exec(gconstpointer fix)
>      QDict *val;
>      const gchar *out;
>      g_autofree guchar *decoded = NULL;
> -    int64_t pid, now, exitcode;
> +    int64_t pid, exitcode;
>      gsize len;
> -    bool exited;
>
>      /* exec 'echo foo bar' */
>      ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
> @@ -777,21 +801,7 @@ static void test_qga_guest_exec(gconstpointer fix)
>      g_assert_cmpint(pid, >, 0);
>      qobject_unref(ret);
>
> -    /* wait for completion */
> -    now = g_get_monotonic_time();
> -    do {
> -        ret = qmp_fd(fixture->fd,
> -                     "{'execute': 'guest-exec-status',"
> -                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
> -        g_assert_nonnull(ret);
> -        val = qdict_get_qdict(ret, "return");
> -        exited = qdict_get_bool(val, "exited");
> -        if (!exited) {
> -            qobject_unref(ret);
> -        }
> -    } while (!exited &&
> -             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
> -    g_assert(exited);
> +    ret = wait_for_guest_exec_completion(fixture->fd, pid);
>
>      /* check stdout */
>      exitcode = qdict_get_int(val, "exitcode");
> @@ -802,6 +812,86 @@ static void test_qga_guest_exec(gconstpointer fix)
>      g_assert_cmpstr((char *)decoded, ==, "\" test_str \"");
>  }
>
> +static void test_qga_guest_exec_output_no_merge(gconstpointer fix)
> +{
> +    const TestFixture *fixture = fix;
> +    g_autoptr(QDict) ret = NULL;
> +    QDict *val;
> +    const gchar *out, *err;
> +    g_autofree guchar *out_decoded = NULL;
> +    g_autofree guchar *err_decoded = NULL;
> +    int64_t pid, exitcode;
> +    gsize len;
> +
> +    /* exec 'echo foo bar' */
> +    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
> +                 " 'path': '/bin/bash',"
> +                 " 'arg': [ '-c', 'for i in $(seq 4); do if (( $i %% 2 )); then echo stdout; else echo stderr 1>&2; fi; done;' ],"
> +                 " 'capture-output': true } }");
> +    g_assert_nonnull(ret);
> +    qmp_assert_no_error(ret);
> +    val = qdict_get_qdict(ret, "return");
> +    pid = qdict_get_int(val, "pid");
> +    g_assert_cmpint(pid, >, 0);
> +    qobject_unref(ret);
> +
> +    ret = wait_for_guest_exec_completion(fixture->fd, pid);
> +
> +    exitcode = qdict_get_int(val, "exitcode");
> +    g_assert_cmpint(exitcode, ==, 0);
> +
> +    /* check stdout */
> +    out = qdict_get_str(val, "out-data");
> +    out_decoded = g_base64_decode(out, &len);
> +    g_assert_cmpint(len, ==, 14);
> +    g_assert_cmpstr((char *)out_decoded, ==, "stdout\nstdout\n");
> +
> +    /* check stderr */
> +    err = qdict_get_try_str(val, "err-data");
> +    err_decoded = g_base64_decode(err, &len);
> +    g_assert_cmpint(len, ==, 14);
> +    g_assert_cmpstr((char *)err_decoded, ==, "stderr\nstderr\n");
> +}
> +
> +static void test_qga_guest_exec_output_merge(gconstpointer fix)
> +{
> +    const TestFixture *fixture = fix;
> +    g_autoptr(QDict) ret = NULL;
> +    QDict *val;
> +    const gchar *out, *err;
> +    g_autofree guchar *decoded = NULL;
> +    int64_t pid, exitcode;
> +    gsize len;
> +
> +    /* exec 'echo foo bar' */
> +    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
> +                 " 'path': '/bin/bash',"
> +                 " 'arg': [ '-c', 'for i in $(seq 4); do if (( $i %% 2 )); then echo stdout; else echo stderr 1>&2; fi; done;' ],"
> +                 " 'capture-output': true,"
> +                 " 'merge-output': true } }");
> +    g_assert_nonnull(ret);
> +    qmp_assert_no_error(ret);
> +    val = qdict_get_qdict(ret, "return");
> +    pid = qdict_get_int(val, "pid");
> +    g_assert_cmpint(pid, >, 0);
> +    qobject_unref(ret);
> +
> +    ret = wait_for_guest_exec_completion(fixture->fd, pid);
> +
> +    exitcode = qdict_get_int(val, "exitcode");
> +    g_assert_cmpint(exitcode, ==, 0);
> +
> +    /* check stdout */
> +    out = qdict_get_str(val, "out-data");
> +    decoded = g_base64_decode(out, &len);
> +    g_assert_cmpint(len, ==, 28);
> +    g_assert_cmpstr((char *)decoded, ==, "stdout\nstderr\nstdout\nstderr\n");
> +
> +    /* check stderr */
> +    err = qdict_get_try_str(val, "err-data");
> +    g_assert_null(err);
> +}
> +
>  static void test_qga_guest_exec_invalid(gconstpointer fix)
>  {
>      const TestFixture *fixture = fix;
> @@ -975,6 +1065,10 @@ int main(int argc, char **argv)
>      g_test_add_data_func("/qga/blockedrpcs", NULL, test_qga_blockedrpcs);
>      g_test_add_data_func("/qga/config", NULL, test_qga_config);
>      g_test_add_data_func("/qga/guest-exec", &fix, test_qga_guest_exec);
> +    g_test_add_data_func("/qga/guest-exec-output-no-merge", &fix,
> +                         test_qga_guest_exec_output_no_merge);
> +    g_test_add_data_func("/qga/guest-exec-output-merge", &fix,
> +                         test_qga_guest_exec_output_merge);
>      g_test_add_data_func("/qga/guest-exec-invalid", &fix,
>                           test_qga_guest_exec_invalid);
>      g_test_add_data_func("/qga/guest-get-osinfo", &fix,
> --
> 2.39.1
>
>
Daniel Xu Feb. 28, 2023, 1:36 a.m. UTC | #2 
Hi,

On Mon, Feb 27, 2023, at 1:40 AM, Marc-André Lureau wrote:
> Hi
>
> On Fri, Feb 24, 2023 at 8:31 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>>
>> This commit adds a test to ensure `merge-output` functions as expected.
>> We also add a negative test to ensure we haven't regressed previous
>> functionality.
>>
>> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
>
> Please check your patch with ASAN, you have use after-free introduced
> by this change:
> ==664972==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x621000135028 at pc 0x55e617a38b39 bp 0x7fff7fe85390 sp
> 0x7fff7fe85388
> READ of size 8 at 0x621000135028 thread T0
>     #0 0x55e617a38b38 in qdict_find ../qobject/qdict.c:96
>     #1 0x55e617a39bea in qdict_get ../qobject/qdict.c:164
>     #2 0x55e617a39bea in qdict_get_int ../qobject/qdict.c:209
>     #3 0x55e6179e2519 in test_qga_guest_exec ../tests/unit/test-qga.c:807
>     #4 0x7fbaa499dc7d in g_test_run_suite_internal
> (/lib64/libglib-2.0.so.0+0x84c7d)
>     #5 0x7fbaa499d9e4 in g_test_run_suite_internal
> (/lib64/libglib-2.0.so.0+0x849e4)
>     #6 0x7fbaa499e181 in g_test_run_suite (/lib64/libglib-2.0.so.0+0x85181)
>     #7 0x7fbaa49966ec in g_test_run (/lib64/libglib-2.0.so.0+0x7d6ec)
>     #8 0x55e6179da0ac in main ../tests/unit/test-qga.c:1083
>     #9 0x7fbaa384a50f in __libc_start_call_main (/lib64/libc.so.6+0x2750f)
>     #10 0x7fbaa384a5c8 in __libc_start_main@GLIBC_2.2.5
> (/lib64/libc.so.6+0x275c8)
>     #11 0x55e6179daf44 in _start
> (/home/elmarco/src/qemu/build/tests/unit/test-qga+0x1bbf44)

Ack. Will fix.

[...]

Thanks,
Daniel
diff mbox series

Patch

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index 9d8e1d1cd3..0b3966024c 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -755,6 +755,31 @@  static void test_qga_fsfreeze_status(gconstpointer fix)
     g_assert_cmpstr(status, ==, "thawed");
 }
 
+static QDict *wait_for_guest_exec_completion(int fd, int64_t pid)
+{
+    QDict *ret = NULL;
+    int64_t now;
+    bool exited;
+    QDict *val;
+
+    now = g_get_monotonic_time();
+    do {
+        ret = qmp_fd(fd,
+                     "{'execute': 'guest-exec-status',"
+                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
+        g_assert_nonnull(ret);
+        val = qdict_get_qdict(ret, "return");
+        exited = qdict_get_bool(val, "exited");
+        if (!exited) {
+            qobject_unref(ret);
+        }
+    } while (!exited &&
+             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
+    g_assert(exited);
+
+    return ret;
+}
+
 static void test_qga_guest_exec(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
@@ -762,9 +787,8 @@  static void test_qga_guest_exec(gconstpointer fix)
     QDict *val;
     const gchar *out;
     g_autofree guchar *decoded = NULL;
-    int64_t pid, now, exitcode;
+    int64_t pid, exitcode;
     gsize len;
-    bool exited;
 
     /* exec 'echo foo bar' */
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
@@ -777,21 +801,7 @@  static void test_qga_guest_exec(gconstpointer fix)
     g_assert_cmpint(pid, >, 0);
     qobject_unref(ret);
 
-    /* wait for completion */
-    now = g_get_monotonic_time();
-    do {
-        ret = qmp_fd(fixture->fd,
-                     "{'execute': 'guest-exec-status',"
-                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
-        g_assert_nonnull(ret);
-        val = qdict_get_qdict(ret, "return");
-        exited = qdict_get_bool(val, "exited");
-        if (!exited) {
-            qobject_unref(ret);
-        }
-    } while (!exited &&
-             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
-    g_assert(exited);
+    ret = wait_for_guest_exec_completion(fixture->fd, pid);
 
     /* check stdout */
     exitcode = qdict_get_int(val, "exitcode");
@@ -802,6 +812,86 @@  static void test_qga_guest_exec(gconstpointer fix)
     g_assert_cmpstr((char *)decoded, ==, "\" test_str \"");
 }
 
+static void test_qga_guest_exec_output_no_merge(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
+    const gchar *out, *err;
+    g_autofree guchar *out_decoded = NULL;
+    g_autofree guchar *err_decoded = NULL;
+    int64_t pid, exitcode;
+    gsize len;
+
+    /* exec 'echo foo bar' */
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
+                 " 'path': '/bin/bash',"
+                 " 'arg': [ '-c', 'for i in $(seq 4); do if (( $i %% 2 )); then echo stdout; else echo stderr 1>&2; fi; done;' ],"
+                 " 'capture-output': true } }");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+    val = qdict_get_qdict(ret, "return");
+    pid = qdict_get_int(val, "pid");
+    g_assert_cmpint(pid, >, 0);
+    qobject_unref(ret);
+
+    ret = wait_for_guest_exec_completion(fixture->fd, pid);
+
+    exitcode = qdict_get_int(val, "exitcode");
+    g_assert_cmpint(exitcode, ==, 0);
+
+    /* check stdout */
+    out = qdict_get_str(val, "out-data");
+    out_decoded = g_base64_decode(out, &len);
+    g_assert_cmpint(len, ==, 14);
+    g_assert_cmpstr((char *)out_decoded, ==, "stdout\nstdout\n");
+
+    /* check stderr */
+    err = qdict_get_try_str(val, "err-data");
+    err_decoded = g_base64_decode(err, &len);
+    g_assert_cmpint(len, ==, 14);
+    g_assert_cmpstr((char *)err_decoded, ==, "stderr\nstderr\n");
+}
+
+static void test_qga_guest_exec_output_merge(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
+    const gchar *out, *err;
+    g_autofree guchar *decoded = NULL;
+    int64_t pid, exitcode;
+    gsize len;
+
+    /* exec 'echo foo bar' */
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
+                 " 'path': '/bin/bash',"
+                 " 'arg': [ '-c', 'for i in $(seq 4); do if (( $i %% 2 )); then echo stdout; else echo stderr 1>&2; fi; done;' ],"
+                 " 'capture-output': true,"
+                 " 'merge-output': true } }");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+    val = qdict_get_qdict(ret, "return");
+    pid = qdict_get_int(val, "pid");
+    g_assert_cmpint(pid, >, 0);
+    qobject_unref(ret);
+
+    ret = wait_for_guest_exec_completion(fixture->fd, pid);
+
+    exitcode = qdict_get_int(val, "exitcode");
+    g_assert_cmpint(exitcode, ==, 0);
+
+    /* check stdout */
+    out = qdict_get_str(val, "out-data");
+    decoded = g_base64_decode(out, &len);
+    g_assert_cmpint(len, ==, 28);
+    g_assert_cmpstr((char *)decoded, ==, "stdout\nstderr\nstdout\nstderr\n");
+
+    /* check stderr */
+    err = qdict_get_try_str(val, "err-data");
+    g_assert_null(err);
+}
+
 static void test_qga_guest_exec_invalid(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
@@ -975,6 +1065,10 @@  int main(int argc, char **argv)
     g_test_add_data_func("/qga/blockedrpcs", NULL, test_qga_blockedrpcs);
     g_test_add_data_func("/qga/config", NULL, test_qga_config);
     g_test_add_data_func("/qga/guest-exec", &fix, test_qga_guest_exec);
+    g_test_add_data_func("/qga/guest-exec-output-no-merge", &fix,
+                         test_qga_guest_exec_output_no_merge);
+    g_test_add_data_func("/qga/guest-exec-output-merge", &fix,
+                         test_qga_guest_exec_output_merge);
     g_test_add_data_func("/qga/guest-exec-invalid", &fix,
                          test_qga_guest_exec_invalid);
     g_test_add_data_func("/qga/guest-get-osinfo", &fix,