[PATCH for-9.2 0/9] tests/qtest/migration-test: Fix various leaks

[PATCH for-9.2 0/9] tests/qtest/migration-test: Fix various leaks

[Peter Maydell]

This patchset fixes various leaks that show up if you run
migration-test under the clang leak-sanitizer. Since they're all
test code problems, this is 9.2 material. The one leak that was
really in the QEMU code I have sent a separate patch for:
https://patchew.org/QEMU/20240820144429.320176-1-peter.maydell@linaro.org/

You can repro these leaks by building a QEMU configured with
 '--cc=clang' '--cxx=clang++' '--enable-debug' '--target-list=x86_64-softmmu' '--enable-sanitizers'
and then running the test like:
(cd build/asan && \
 ASAN_OPTIONS="fast_unwind_on_malloc=0" \
 QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --tap -k )

thanks
-- PMM

Peter Maydell (9):
  tests/qtest/migration-test: Fix bootfile cleanup handling
  tests/qtest/migration-test: Don't leak resp in
    multifd_mapped_ram_fdset_end()
  tests/qtest/migration-test: Fix leaks in calc_dirtyrate_ready()
  tests/qtest/migration-helpers: Fix migrate_get_socket_address() leak
  tests/qtest/migration-test: Free QCRyptoTLSTestCertReq objects
  tests/unit/crypto-tls-x509-helpers: deinit privkey in test_tls_cleanup
  tests/qtest/migration-helpers: Don't dup argument to qdict_put_str()
  tests/qtest/migration-test: Don't strdup in get_dirty_rate()
  tests/qtest/migration-test: Don't leak QTestState in
    test_multifd_tcp_cancel()

 tests/unit/crypto-tls-x509-helpers.h |  6 +++++
 tests/qtest/migration-helpers.c      | 20 +++++++---------
 tests/qtest/migration-test.c         | 36 ++++++++++++++++++----------
 tests/unit/crypto-tls-x509-helpers.c | 13 ++++++++--
 4 files changed, 50 insertions(+), 25 deletions(-)

-- 
2.34.1

[PATCH for-9.2 1/9] tests/qtest/migration-test: Fix bootfile cleanup handling

[Peter Maydell]

If you invoke the migration-test binary in such a way that it doesn't run
any tests, then we never call bootfile_create(), and at the end of
main() bootfile_delete() will try to unlink(NULL), which is not valid.
This can happen if for instance you tell the test binary to run a
subset of tests that turns out to be empty, like this:

 (cd build/asan && QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --tap -k -p bang)
 # random seed: R02S6501b289ff8ced4231ba452c3a87bc6f
 # Skipping test: userfaultfd not available
 1..0
 ../../tests/qtest/migration-test.c:182:12: runtime error: null pointer passed as argument 1, which is declared to never be null
 /usr/include/unistd.h:858:48: note: nonnull attribute specified here

Conversely, because we call bootfile_create() once per test
but only call bootfile_delete() at the end of the whole test
run, we will leak the memory we used for bootpath when we
overwrite it.

Handle these by:
 * making bootfile_delete() handle not needing to do anything
   because bootfile_create() was never called
 * making bootfile_create() call bootfile_delete() first to
   tidy up any previous bootfile before it creates a fresh one

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I spotted this because I was trying to run a single subtest and
messed up the test name so it didn't match anything :-)
The second part was noticed by LeakSanitizer.
---
 tests/qtest/migration-test.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 70b606b8886..5cf238a4f05 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -144,12 +144,23 @@ static char *bootpath;
 #include "tests/migration/ppc64/a-b-kernel.h"
 #include "tests/migration/s390x/a-b-bios.h"
 
+static void bootfile_delete(void)
+{
+    if (!bootpath) {
+        return;
+    }
+    unlink(bootpath);
+    g_free(bootpath);
+    bootpath = NULL;
+}
+
 static void bootfile_create(char *dir, bool suspend_me)
 {
     const char *arch = qtest_get_arch();
     unsigned char *content;
     size_t len;
 
+    bootfile_delete();
     bootpath = g_strdup_printf("%s/bootsect", dir);
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         /* the assembled x86 boot sector should be exactly one sector large */
@@ -177,13 +188,6 @@ static void bootfile_create(char *dir, bool suspend_me)
     fclose(bootfile);
 }
 
-static void bootfile_delete(void)
-{
-    unlink(bootpath);
-    g_free(bootpath);
-    bootpath = NULL;
-}
-
 /*
  * Wait for some output in the serial output file,
  * we get an 'A' followed by an endless string of 'B's
-- 
2.34.1

Re: [PATCH for-9.2 1/9] tests/qtest/migration-test: Fix bootfile cleanup handling

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> If you invoke the migration-test binary in such a way that it doesn't run
> any tests, then we never call bootfile_create(), and at the end of
> main() bootfile_delete() will try to unlink(NULL), which is not valid.
> This can happen if for instance you tell the test binary to run a
> subset of tests that turns out to be empty, like this:
>
>  (cd build/asan && QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --tap -k -p bang)
>  # random seed: R02S6501b289ff8ced4231ba452c3a87bc6f
>  # Skipping test: userfaultfd not available
>  1..0
>  ../../tests/qtest/migration-test.c:182:12: runtime error: null pointer passed as argument 1, which is declared to never be null
>  /usr/include/unistd.h:858:48: note: nonnull attribute specified here
>
> Conversely, because we call bootfile_create() once per test
> but only call bootfile_delete() at the end of the whole test
> run, we will leak the memory we used for bootpath when we
> overwrite it.
>
> Handle these by:
>  * making bootfile_delete() handle not needing to do anything
>    because bootfile_create() was never called
>  * making bootfile_create() call bootfile_delete() first to
>    tidy up any previous bootfile before it creates a fresh one
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH for-9.2 2/9] tests/qtest/migration-test: Don't leak resp in multifd_mapped_ram_fdset_end()

[Peter Maydell]

In multifd_mapped_ram_fdset_end() we call qtest_qmp() but forgot
to unref the response QDict we get back, which means it is leaked:

Indirect leak of 4120 byte(s) in 1 object(s) allocated from:
    #0 0x55c0c095d318 in __interceptor_calloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f318) (BuildI
d: 07f667506452d6c467dbc06fd95191966d3e91b4)
    #1 0x7f186f939c50 in g_malloc0 debian/build/deb/../../../glib/gmem.c:161:13
    #2 0x55c0c0ae9b01 in qdict_new qobject/qdict.c:30:13
    #3 0x55c0c0afc16c in parse_object qobject/json-parser.c:317:12
    #4 0x55c0c0afb90f in parse_value qobject/json-parser.c:545:16
    #5 0x55c0c0afb579 in json_parser_parse qobject/json-parser.c:579:14
    #6 0x55c0c0afa21d in json_message_process_token qobject/json-streamer.c:92:12
    #7 0x55c0c0bca2e5 in json_lexer_feed_char qobject/json-lexer.c:313:13
    #8 0x55c0c0bc97ce in json_lexer_feed qobject/json-lexer.c:350:9
    #9 0x55c0c0afabbc in json_message_parser_feed qobject/json-streamer.c:121:5
    #10 0x55c0c09cbd52 in qmp_fd_receive tests/qtest/libqmp.c:86:9
    #11 0x55c0c09be69b in qtest_qmp_receive_dict tests/qtest/libqtest.c:760:12
    #12 0x55c0c09bca77 in qtest_qmp_receive tests/qtest/libqtest.c:741:27
    #13 0x55c0c09bee9d in qtest_vqmp tests/qtest/libqtest.c:812:12
    #14 0x55c0c09bd257 in qtest_qmp tests/qtest/libqtest.c:835:16
    #15 0x55c0c0a87747 in multifd_mapped_ram_fdset_end tests/qtest/migration-test.c:2393:12
    #16 0x55c0c0a85eb3 in test_file_common tests/qtest/migration-test.c:1978:9
    #17 0x55c0c0a746a3 in test_multifd_file_mapped_ram_fdset tests/qtest/migration-test.c:2437:5
    #18 0x55c0c0a93237 in migration_test_wrapper tests/qtest/migration-helpers.c:458:5
    #19 0x7f186f958aed in test_case_run debian/build/deb/../../../glib/gtestutils.c:2930:15
    #20 0x7f186f958aed in g_test_run_suite_internal debian/build/deb/../../../glib/gtestutils.c:3018:16
    #21 0x7f186f95880a in g_test_run_suite_internal debian/build/deb/../../../glib/gtestutils.c:3035:18
    #22 0x7f186f95880a in g_test_run_suite_internal debian/build/deb/../../../glib/gtestutils.c:3035:18
    #23 0x7f186f95880a in g_test_run_suite_internal debian/build/deb/../../../glib/gtestutils.c:3035:18
    #24 0x7f186f95880a in g_test_run_suite_internal debian/build/deb/../../../glib/gtestutils.c:3035:18
    #25 0x7f186f95880a in g_test_run_suite_internal debian/build/deb/../../../glib/gtestutils.c:3035:18
    #26 0x7f186f958faa in g_test_run_suite debian/build/deb/../../../glib/gtestutils.c:3109:18
    #27 0x7f186f959055 in g_test_run debian/build/deb/../../../glib/gtestutils.c:2231:7
    #28 0x7f186f959055 in g_test_run debian/build/deb/../../../glib/gtestutils.c:2218:1
    #29 0x55c0c0a6e427 in main tests/qtest/migration-test.c:4033:11

Unref the object after we've confirmed that it is what we expect.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/migration-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 5cf238a4f05..6aba527340b 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2395,6 +2395,7 @@ static void multifd_mapped_ram_fdset_end(QTestState *from, QTestState *to,
     g_assert(qdict_haskey(resp, "return"));
     fdsets = qdict_get_qlist(resp, "return");
     g_assert(fdsets && qlist_empty(fdsets));
+    qobject_unref(resp);
 }
 
 static void *multifd_mapped_ram_fdset_dio(QTestState *from, QTestState *to)
-- 
2.34.1

Re: [PATCH for-9.2 2/9] tests/qtest/migration-test: Don't leak resp in multifd_mapped_ram_fdset_end()

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> In multifd_mapped_ram_fdset_end() we call qtest_qmp() but forgot
> to unref the response QDict we get back, which means it is leaked:
>
> Indirect leak of 4120 byte(s) in 1 object(s) allocated from:
>     #0 0x55c0c095d318 in __interceptor_calloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f318) (BuildI
> d: 07f667506452d6c467dbc06fd95191966d3e91b4)
>     #1 0x7f186f939c50 in g_malloc0 debian/build/deb/../../../glib/gmem.c:161:13
>     #2 0x55c0c0ae9b01 in qdict_new qobject/qdict.c:30:13
>     #3 0x55c0c0afc16c in parse_object qobject/json-parser.c:317:12
>     #4 0x55c0c0afb90f in parse_value qobject/json-parser.c:545:16
>     #5 0x55c0c0afb579 in json_parser_parse qobject/json-parser.c:579:14
>     #6 0x55c0c0afa21d in json_message_process_token qobject/json-streamer.c:92:12
>     #7 0x55c0c0bca2e5 in json_lexer_feed_char qobject/json-lexer.c:313:13
>     #8 0x55c0c0bc97ce in json_lexer_feed qobject/json-lexer.c:350:9
>     #9 0x55c0c0afabbc in json_message_parser_feed qobject/json-streamer.c:121:5
>     #10 0x55c0c09cbd52 in qmp_fd_receive tests/qtest/libqmp.c:86:9
>     #11 0x55c0c09be69b in qtest_qmp_receive_dict tests/qtest/libqtest.c:760:12
>     #12 0x55c0c09bca77 in qtest_qmp_receive tests/qtest/libqtest.c:741:27
>     #13 0x55c0c09bee9d in qtest_vqmp tests/qtest/libqtest.c:812:12
>     #14 0x55c0c09bd257 in qtest_qmp tests/qtest/libqtest.c:835:16
>     #15 0x55c0c0a87747 in multifd_mapped_ram_fdset_end tests/qtest/migration-test.c:2393:12
>     #16 0x55c0c0a85eb3 in test_file_common tests/qtest/migration-test.c:1978:9
>     #17 0x55c0c0a746a3 in test_multifd_file_mapped_ram_fdset tests/qtest/migration-test.c:2437:5
>     #18 0x55c0c0a93237 in migration_test_wrapper tests/qtest/migration-helpers.c:458:5
>     #19 0x7f186f958aed in test_case_run debian/build/deb/../../../glib/gtestutils.c:2930:15
>     #20 0x7f186f958aed in g_test_run_suite_internal debian/build/deb/../../../glib/gtestutils.c:3018:16
>     #21 0x7f186f95880a in g_test_run_suite_internal debian/build/deb/../../../glib/gtestutils.c:3035:18
>     #22 0x7f186f95880a in g_test_run_suite_internal debian/build/deb/../../../glib/gtestutils.c:3035:18
>     #23 0x7f186f95880a in g_test_run_suite_internal debian/build/deb/../../../glib/gtestutils.c:3035:18
>     #24 0x7f186f95880a in g_test_run_suite_internal debian/build/deb/../../../glib/gtestutils.c:3035:18
>     #25 0x7f186f95880a in g_test_run_suite_internal debian/build/deb/../../../glib/gtestutils.c:3035:18
>     #26 0x7f186f958faa in g_test_run_suite debian/build/deb/../../../glib/gtestutils.c:3109:18
>     #27 0x7f186f959055 in g_test_run debian/build/deb/../../../glib/gtestutils.c:2231:7
>     #28 0x7f186f959055 in g_test_run debian/build/deb/../../../glib/gtestutils.c:2218:1
>     #29 0x55c0c0a6e427 in main tests/qtest/migration-test.c:4033:11
>
> Unref the object after we've confirmed that it is what we expect.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/qtest/migration-test.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 5cf238a4f05..6aba527340b 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -2395,6 +2395,7 @@ static void multifd_mapped_ram_fdset_end(QTestState *from, QTestState *to,
>      g_assert(qdict_haskey(resp, "return"));
>      fdsets = qdict_get_qlist(resp, "return");
>      g_assert(fdsets && qlist_empty(fdsets));
> +    qobject_unref(resp);
>  }
>  
>  static void *multifd_mapped_ram_fdset_dio(QTestState *from, QTestState *to)

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH for-9.2 3/9] tests/qtest/migration-test: Fix leaks in calc_dirtyrate_ready()

[Peter Maydell]

In calc_dirtyrate_ready() we g_strdup() a string but then never free it:

Direct leak of 19 byte(s) in 2 object(s) allocated from:
    #0 0x55ead613413e in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f13e) (BuildId: e7cd5c37b2987a1af682b43ee5240b98bb316737)
    #1 0x7f7a13d39738 in g_malloc debian/build/deb/../../../glib/gmem.c:128:13
    #2 0x7f7a13d4e583 in g_strdup debian/build/deb/../../../glib/gstrfuncs.c:361:17
    #3 0x55ead6266f48 in calc_dirtyrate_ready tests/qtest/migration-test.c:3409:14
    #4 0x55ead62669fe in wait_for_calc_dirtyrate_complete tests/qtest/migration-test.c:3422:13
    #5 0x55ead6253df7 in test_vcpu_dirty_limit tests/qtest/migration-test.c:3562:9
    #6 0x55ead626a407 in migration_test_wrapper tests/qtest/migration-helpers.c:456:5

We also fail to unref the QMP rsp_return, so we leak that also.

Rather than duplicating the string, use the in-place value from
the qdict, and then unref the qdict.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/migration-test.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 6aba527340b..9811047eeb4 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -3398,15 +3398,18 @@ static QDict *query_vcpu_dirty_limit(QTestState *who)
 static bool calc_dirtyrate_ready(QTestState *who)
 {
     QDict *rsp_return;
-    gchar *status;
+    const char *status;
+    bool ready;
 
     rsp_return = query_dirty_rate(who);
     g_assert(rsp_return);
 
-    status = g_strdup(qdict_get_str(rsp_return, "status"));
+    status = qdict_get_str(rsp_return, "status");
     g_assert(status);
+    ready = g_strcmp0(status, "measuring");
+    qobject_unref(rsp_return);
 
-    return g_strcmp0(status, "measuring");
+    return ready;
 }
 
 static void wait_for_calc_dirtyrate_complete(QTestState *who,
-- 
2.34.1

Re: [PATCH for-9.2 3/9] tests/qtest/migration-test: Fix leaks in calc_dirtyrate_ready()

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> In calc_dirtyrate_ready() we g_strdup() a string but then never free it:
>
> Direct leak of 19 byte(s) in 2 object(s) allocated from:
>     #0 0x55ead613413e in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f13e) (BuildId: e7cd5c37b2987a1af682b43ee5240b98bb316737)
>     #1 0x7f7a13d39738 in g_malloc debian/build/deb/../../../glib/gmem.c:128:13
>     #2 0x7f7a13d4e583 in g_strdup debian/build/deb/../../../glib/gstrfuncs.c:361:17
>     #3 0x55ead6266f48 in calc_dirtyrate_ready tests/qtest/migration-test.c:3409:14
>     #4 0x55ead62669fe in wait_for_calc_dirtyrate_complete tests/qtest/migration-test.c:3422:13
>     #5 0x55ead6253df7 in test_vcpu_dirty_limit tests/qtest/migration-test.c:3562:9
>     #6 0x55ead626a407 in migration_test_wrapper tests/qtest/migration-helpers.c:456:5
>
> We also fail to unref the QMP rsp_return, so we leak that also.
>
> Rather than duplicating the string, use the in-place value from
> the qdict, and then unref the qdict.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH for-9.2 4/9] tests/qtest/migration-helpers: Fix migrate_get_socket_address() leak

[Peter Maydell]

In migrate_get_socket_address() we leak the SocketAddressList:
 (cd build/asan && \
  ASAN_OPTIONS="fast_unwind_on_malloc=0:strip_path_prefix=/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/../../"
  QTEST_QEMU_BINARY=./qemu-system-x86_64 \
  ./tests/qtest/migration-test --tap -k -p /x86_64/migration/multifd/tcp/tls/psk/match )

[...]
Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x563d7f22f318 in __interceptor_calloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f318) (BuildId: 2ad6282fb5d076c863ab87f41a345d46dc965ded)
    #1 0x7f9de3b39c50 in g_malloc0 debian/build/deb/../../../glib/gmem.c:161:13
    #2 0x563d7f3a119c in qobject_input_start_list qapi/qobject-input-visitor.c:336:17
    #3 0x563d7f390fbf in visit_start_list qapi/qapi-visit-core.c:80:10
    #4 0x563d7f3882ef in visit_type_SocketAddressList /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/qapi/qapi-visit-sockets.c:519:10
    #5 0x563d7f3658c9 in migrate_get_socket_address tests/qtest/migration-helpers.c:97:5
    #6 0x563d7f362e24 in migrate_get_connect_uri tests/qtest/migration-helpers.c:111:13
    #7 0x563d7f362bb2 in migrate_qmp tests/qtest/migration-helpers.c:222:23
    #8 0x563d7f3533cd in test_precopy_common tests/qtest/migration-test.c:1817:5
    #9 0x563d7f34dc1c in test_multifd_tcp_tls_psk_match tests/qtest/migration-test.c:3185:5
    #10 0x563d7f365337 in migration_test_wrapper tests/qtest/migration-helpers.c:458:5

The code fishes out the SocketAddress from the list to return it, and the
callers are freeing that, but nothing frees the list.

Since this function is called in only two places, the simple fix is to
make it return the SocketAddressList rather than just a SocketAddress,
and then the callers can easily access the SocketAddress, and free
the whole SocketAddressList when they're done.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/migration-helpers.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 84f49db85e0..7cbb9831e76 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -82,11 +82,10 @@ static QDict *SocketAddress_to_qdict(SocketAddress *addr)
     return dict;
 }
 
-static SocketAddress *migrate_get_socket_address(QTestState *who)
+static SocketAddressList *migrate_get_socket_address(QTestState *who)
 {
     QDict *rsp;
     SocketAddressList *addrs;
-    SocketAddress *addr;
     Visitor *iv = NULL;
     QObject *object;
 
@@ -95,36 +94,35 @@ static SocketAddress *migrate_get_socket_address(QTestState *who)
 
     iv = qobject_input_visitor_new(object);
     visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
-    addr = addrs->value;
     visit_free(iv);
 
     qobject_unref(rsp);
-    return addr;
+    return addrs;
 }
 
 static char *
 migrate_get_connect_uri(QTestState *who)
 {
-    SocketAddress *addrs;
+    SocketAddressList *addrs;
     char *connect_uri;
 
     addrs = migrate_get_socket_address(who);
-    connect_uri = SocketAddress_to_str(addrs);
+    connect_uri = SocketAddress_to_str(addrs->value);
 
-    qapi_free_SocketAddress(addrs);
+    qapi_free_SocketAddressList(addrs);
     return connect_uri;
 }
 
 static QDict *
 migrate_get_connect_qdict(QTestState *who)
 {
-    SocketAddress *addrs;
+    SocketAddressList *addrs;
     QDict *connect_qdict;
 
     addrs = migrate_get_socket_address(who);
-    connect_qdict = SocketAddress_to_qdict(addrs);
+    connect_qdict = SocketAddress_to_qdict(addrs->value);
 
-    qapi_free_SocketAddress(addrs);
+    qapi_free_SocketAddressList(addrs);
     return connect_qdict;
 }
 
-- 
2.34.1

Re: [PATCH for-9.2 4/9] tests/qtest/migration-helpers: Fix migrate_get_socket_address() leak

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> In migrate_get_socket_address() we leak the SocketAddressList:
>  (cd build/asan && \
>   ASAN_OPTIONS="fast_unwind_on_malloc=0:strip_path_prefix=/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/../../"
>   QTEST_QEMU_BINARY=./qemu-system-x86_64 \
>   ./tests/qtest/migration-test --tap -k -p /x86_64/migration/multifd/tcp/tls/psk/match )
>
> [...]
> Direct leak of 16 byte(s) in 1 object(s) allocated from:
>     #0 0x563d7f22f318 in __interceptor_calloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f318) (BuildId: 2ad6282fb5d076c863ab87f41a345d46dc965ded)
>     #1 0x7f9de3b39c50 in g_malloc0 debian/build/deb/../../../glib/gmem.c:161:13
>     #2 0x563d7f3a119c in qobject_input_start_list qapi/qobject-input-visitor.c:336:17
>     #3 0x563d7f390fbf in visit_start_list qapi/qapi-visit-core.c:80:10
>     #4 0x563d7f3882ef in visit_type_SocketAddressList /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/qapi/qapi-visit-sockets.c:519:10
>     #5 0x563d7f3658c9 in migrate_get_socket_address tests/qtest/migration-helpers.c:97:5
>     #6 0x563d7f362e24 in migrate_get_connect_uri tests/qtest/migration-helpers.c:111:13
>     #7 0x563d7f362bb2 in migrate_qmp tests/qtest/migration-helpers.c:222:23
>     #8 0x563d7f3533cd in test_precopy_common tests/qtest/migration-test.c:1817:5
>     #9 0x563d7f34dc1c in test_multifd_tcp_tls_psk_match tests/qtest/migration-test.c:3185:5
>     #10 0x563d7f365337 in migration_test_wrapper tests/qtest/migration-helpers.c:458:5
>
> The code fishes out the SocketAddress from the list to return it, and the
> callers are freeing that, but nothing frees the list.
>
> Since this function is called in only two places, the simple fix is to
> make it return the SocketAddressList rather than just a SocketAddress,
> and then the callers can easily access the SocketAddress, and free
> the whole SocketAddressList when they're done.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH for-9.2 5/9] tests/qtest/migration-test: Free QCRyptoTLSTestCertReq objects

[Peter Maydell]

In the migration test we create several TLS certificates with
the TLS_* macros from crypto-tls-x509-helpers.h. These macros
create both a QCryptoTLSCertReq object which must be deinitialized
and also an on-disk certificate file. The migration test currently
removes the on-disk file in test_migrate_tls_x509_finish() but
never deinitializes the QCryptoTLSCertReq, which means that memory
allocated as part of it is leaked:

Indirect leak of 2 byte(s) in 1 object(s) allocated from:
    #0 0x5558ba33712e in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f12e) (BuildId: 4c8618f663e538538cad19d35233124cea161491)
    #1 0x7f64afc131f4  (/lib/x86_64-linux-gnu/libtasn1.so.6+0x81f4) (BuildId: 2fde6ecb43c586fe4077118f771077aa1298e7ea)
    #2 0x7f64afc18d58 in asn1_write_value (/lib/x86_64-linux-gnu/libtasn1.so.6+0xdd58) (BuildId: 2fde6ecb43c586fe4077118f771077aa1298e7ea)
    #3 0x7f64af8fc678 in gnutls_x509_crt_set_version (/lib/x86_64-linux-gnu/libgnutls.so.30+0xe7678) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
    #4 0x5558ba470035 in test_tls_generate_cert tests/unit/crypto-tls-x509-helpers.c:234:5
    #5 0x5558ba464e4a in test_migrate_tls_x509_start_common tests/qtest/migration-test.c:1058:5
    #6 0x5558ba462c8a in test_migrate_tls_x509_start_default_host tests/qtest/migration-test.c:1123:12
    #7 0x5558ba45ab40 in test_precopy_common tests/qtest/migration-test.c:1786:21
    #8 0x5558ba450015 in test_precopy_unix_tls_x509_default_host tests/qtest/migration-test.c:2077:5
    #9 0x5558ba46d3c7 in migration_test_wrapper tests/qtest/migration-helpers.c:456:5

(and similar reports).

The only function currently provided to deinit a QCryptoTLSCertReq is
test_tls_discard_cert(), which also removes the on-disk certificate
file.  For the migration tests we need to retain the on-disk files
until we've finished running the test, so the simplest fix is to
provide a new function test_tls_deinit_cert() which does only the
cleanup of the QCryptoTLSCertReq, and call it in the right places.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/unit/crypto-tls-x509-helpers.h |  6 ++++++
 tests/qtest/migration-test.c         |  3 +++
 tests/unit/crypto-tls-x509-helpers.c | 12 ++++++++++--
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/tests/unit/crypto-tls-x509-helpers.h b/tests/unit/crypto-tls-x509-helpers.h
index 562c1606535..2a0f7c04fd8 100644
--- a/tests/unit/crypto-tls-x509-helpers.h
+++ b/tests/unit/crypto-tls-x509-helpers.h
@@ -73,6 +73,12 @@ void test_tls_generate_cert(QCryptoTLSTestCertReq *req,
 void test_tls_write_cert_chain(const char *filename,
                                gnutls_x509_crt_t *certs,
                                size_t ncerts);
+/*
+ * Deinitialize the QCryptoTLSTestCertReq, but don't delete the certificate
+ * file on disk. (The caller is then responsible for doing that themselves.
+ */
+void test_tls_deinit_cert(QCryptoTLSTestCertReq *req);
+/* Deinit the QCryptoTLSTestCertReq, and delete the certificate file */
 void test_tls_discard_cert(QCryptoTLSTestCertReq *req);
 
 void test_tls_init(const char *keyfile);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 9811047eeb4..a659609ccb0 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1062,12 +1062,15 @@ test_migrate_tls_x509_start_common(QTestState *from,
                                    QCRYPTO_TLS_TEST_CLIENT_HOSTILE_NAME :
                                    QCRYPTO_TLS_TEST_CLIENT_NAME,
                                    data->clientcert);
+        test_tls_deinit_cert(&servercertreq);
     }
 
     TLS_CERT_REQ_SIMPLE_SERVER(clientcertreq, cacertreq,
                                data->servercert,
                                args->certhostname,
                                args->certipaddr);
+    test_tls_deinit_cert(&clientcertreq);
+    test_tls_deinit_cert(&cacertreq);
 
     qtest_qmp_assert_success(from,
                              "{ 'execute': 'object-add',"
diff --git a/tests/unit/crypto-tls-x509-helpers.c b/tests/unit/crypto-tls-x509-helpers.c
index 3e74ec5b5d4..b316155d6a6 100644
--- a/tests/unit/crypto-tls-x509-helpers.c
+++ b/tests/unit/crypto-tls-x509-helpers.c
@@ -502,8 +502,7 @@ void test_tls_write_cert_chain(const char *filename,
     g_free(buffer);
 }
 
-
-void test_tls_discard_cert(QCryptoTLSTestCertReq *req)
+void test_tls_deinit_cert(QCryptoTLSTestCertReq *req)
 {
     if (!req->crt) {
         return;
@@ -511,6 +510,15 @@ void test_tls_discard_cert(QCryptoTLSTestCertReq *req)
 
     gnutls_x509_crt_deinit(req->crt);
     req->crt = NULL;
+}
+
+void test_tls_discard_cert(QCryptoTLSTestCertReq *req)
+{
+    if (!req->crt) {
+        return;
+    }
+
+    test_tls_deinit_cert(req);
 
     if (getenv("QEMU_TEST_DEBUG_CERTS") == NULL) {
         unlink(req->filename);
-- 
2.34.1

Re: [PATCH for-9.2 5/9] tests/qtest/migration-test: Free QCRyptoTLSTestCertReq objects

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> In the migration test we create several TLS certificates with
> the TLS_* macros from crypto-tls-x509-helpers.h. These macros
> create both a QCryptoTLSCertReq object which must be deinitialized
> and also an on-disk certificate file. The migration test currently
> removes the on-disk file in test_migrate_tls_x509_finish() but
> never deinitializes the QCryptoTLSCertReq, which means that memory
> allocated as part of it is leaked:
>
> Indirect leak of 2 byte(s) in 1 object(s) allocated from:
>     #0 0x5558ba33712e in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f12e) (BuildId: 4c8618f663e538538cad19d35233124cea161491)
>     #1 0x7f64afc131f4  (/lib/x86_64-linux-gnu/libtasn1.so.6+0x81f4) (BuildId: 2fde6ecb43c586fe4077118f771077aa1298e7ea)
>     #2 0x7f64afc18d58 in asn1_write_value (/lib/x86_64-linux-gnu/libtasn1.so.6+0xdd58) (BuildId: 2fde6ecb43c586fe4077118f771077aa1298e7ea)
>     #3 0x7f64af8fc678 in gnutls_x509_crt_set_version (/lib/x86_64-linux-gnu/libgnutls.so.30+0xe7678) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
>     #4 0x5558ba470035 in test_tls_generate_cert tests/unit/crypto-tls-x509-helpers.c:234:5
>     #5 0x5558ba464e4a in test_migrate_tls_x509_start_common tests/qtest/migration-test.c:1058:5
>     #6 0x5558ba462c8a in test_migrate_tls_x509_start_default_host tests/qtest/migration-test.c:1123:12
>     #7 0x5558ba45ab40 in test_precopy_common tests/qtest/migration-test.c:1786:21
>     #8 0x5558ba450015 in test_precopy_unix_tls_x509_default_host tests/qtest/migration-test.c:2077:5
>     #9 0x5558ba46d3c7 in migration_test_wrapper tests/qtest/migration-helpers.c:456:5
>
> (and similar reports).
>
> The only function currently provided to deinit a QCryptoTLSCertReq is
> test_tls_discard_cert(), which also removes the on-disk certificate
> file.  For the migration tests we need to retain the on-disk files
> until we've finished running the test, so the simplest fix is to
> provide a new function test_tls_deinit_cert() which does only the
> cleanup of the QCryptoTLSCertReq, and call it in the right places.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH for-9.2 6/9] tests/unit/crypto-tls-x509-helpers: deinit privkey in test_tls_cleanup

[Peter Maydell]

We create a gnutls_x509_privkey_t in test_tls_init(), but forget
to deinit it in test_tls_cleanup(), resulting in leaks
reported in hte migration test such as:

Indirect leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x55fa6d11c12e in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f12e) (BuildId: 852a267993587f557f50e5715f352f43720077ba)
    #1 0x7f073982685d in __gmp_default_allocate (/lib/x86_64-linux-gnu/libgmp.so.10+0xa85d) (BuildId: f110719303ddbea25a5e89ff730fec520eed67b0)
    #2 0x7f0739836193 in __gmpz_realloc (/lib/x86_64-linux-gnu/libgmp.so.10+0x1a193) (BuildId: f110719303ddbea25a5e89ff730fec520eed67b0)
    #3 0x7f0739836594 in __gmpz_import (/lib/x86_64-linux-gnu/libgmp.so.10+0x1a594) (BuildId: f110719303ddbea25a5e89ff730fec520eed67b0)
    #4 0x7f07398a91ed in nettle_mpz_set_str_256_u (/lib/x86_64-linux-gnu/libhogweed.so.6+0xb1ed) (BuildId: 3cc4a3474de72db89e9dcc93bfb95fe377f48c37)
    #5 0x7f073a146a5a  (/lib/x86_64-linux-gnu/libgnutls.so.30+0x131a5a) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
    #6 0x7f073a07192c  (/lib/x86_64-linux-gnu/libgnutls.so.30+0x5c92c) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
    #7 0x7f073a078333  (/lib/x86_64-linux-gnu/libgnutls.so.30+0x63333) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
    #8 0x7f073a0e8353  (/lib/x86_64-linux-gnu/libgnutls.so.30+0xd3353) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
    #9 0x7f073a0ef0ac in gnutls_x509_privkey_import (/lib/x86_64-linux-gnu/libgnutls.so.30+0xda0ac) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
    #10 0x55fa6d2547e3 in test_tls_load_key tests/unit/crypto-tls-x509-helpers.c:99:11
    #11 0x55fa6d25460c in test_tls_init tests/unit/crypto-tls-x509-helpers.c:128:15
    #12 0x55fa6d2495c4 in test_migrate_tls_x509_start_common tests/qtest/migration-test.c:1044:5
    #13 0x55fa6d24c23a in test_migrate_tls_x509_start_reject_anon_client tests/qtest/migration-test.c:1216:12
    #14 0x55fa6d23fb40 in test_precopy_common tests/qtest/migration-test.c:1789:21
    #15 0x55fa6d236b7c in test_precopy_tcp_tls_x509_reject_anon_client tests/qtest/migration-test.c:2614:5

(Oddly, there is no reported leak in the x509 unit tests, even though
those also use test_tls_init() and test_tls_cleanup().)

Deinit the privkey in test_tls_cleanup().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/unit/crypto-tls-x509-helpers.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/unit/crypto-tls-x509-helpers.c b/tests/unit/crypto-tls-x509-helpers.c
index b316155d6a6..2daecc416c6 100644
--- a/tests/unit/crypto-tls-x509-helpers.c
+++ b/tests/unit/crypto-tls-x509-helpers.c
@@ -135,6 +135,7 @@ void test_tls_init(const char *keyfile)
 void test_tls_cleanup(const char *keyfile)
 {
     asn1_delete_structure(&pkix_asn1);
+    gnutls_x509_privkey_deinit(privkey);
     unlink(keyfile);
 }
 
-- 
2.34.1

Re: [PATCH for-9.2 6/9] tests/unit/crypto-tls-x509-helpers: deinit privkey in test_tls_cleanup

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> We create a gnutls_x509_privkey_t in test_tls_init(), but forget
> to deinit it in test_tls_cleanup(), resulting in leaks
> reported in hte migration test such as:
>
> Indirect leak of 8 byte(s) in 1 object(s) allocated from:
>     #0 0x55fa6d11c12e in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f12e) (BuildId: 852a267993587f557f50e5715f352f43720077ba)
>     #1 0x7f073982685d in __gmp_default_allocate (/lib/x86_64-linux-gnu/libgmp.so.10+0xa85d) (BuildId: f110719303ddbea25a5e89ff730fec520eed67b0)
>     #2 0x7f0739836193 in __gmpz_realloc (/lib/x86_64-linux-gnu/libgmp.so.10+0x1a193) (BuildId: f110719303ddbea25a5e89ff730fec520eed67b0)
>     #3 0x7f0739836594 in __gmpz_import (/lib/x86_64-linux-gnu/libgmp.so.10+0x1a594) (BuildId: f110719303ddbea25a5e89ff730fec520eed67b0)
>     #4 0x7f07398a91ed in nettle_mpz_set_str_256_u (/lib/x86_64-linux-gnu/libhogweed.so.6+0xb1ed) (BuildId: 3cc4a3474de72db89e9dcc93bfb95fe377f48c37)
>     #5 0x7f073a146a5a  (/lib/x86_64-linux-gnu/libgnutls.so.30+0x131a5a) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
>     #6 0x7f073a07192c  (/lib/x86_64-linux-gnu/libgnutls.so.30+0x5c92c) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
>     #7 0x7f073a078333  (/lib/x86_64-linux-gnu/libgnutls.so.30+0x63333) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
>     #8 0x7f073a0e8353  (/lib/x86_64-linux-gnu/libgnutls.so.30+0xd3353) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
>     #9 0x7f073a0ef0ac in gnutls_x509_privkey_import (/lib/x86_64-linux-gnu/libgnutls.so.30+0xda0ac) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
>     #10 0x55fa6d2547e3 in test_tls_load_key tests/unit/crypto-tls-x509-helpers.c:99:11
>     #11 0x55fa6d25460c in test_tls_init tests/unit/crypto-tls-x509-helpers.c:128:15
>     #12 0x55fa6d2495c4 in test_migrate_tls_x509_start_common tests/qtest/migration-test.c:1044:5
>     #13 0x55fa6d24c23a in test_migrate_tls_x509_start_reject_anon_client tests/qtest/migration-test.c:1216:12
>     #14 0x55fa6d23fb40 in test_precopy_common tests/qtest/migration-test.c:1789:21
>     #15 0x55fa6d236b7c in test_precopy_tcp_tls_x509_reject_anon_client tests/qtest/migration-test.c:2614:5
>
> (Oddly, there is no reported leak in the x509 unit tests, even though
> those also use test_tls_init() and test_tls_cleanup().)
>
> Deinit the privkey in test_tls_cleanup().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH for-9.2 7/9] tests/qtest/migration-helpers: Don't dup argument to qdict_put_str()

[Peter Maydell]

In migrate_set_ports() we call qdict_put_str() with a value string
which we g_strdup(). However qdict_put_str() takes a copy of the
value string, it doesn't take ownership of it, so the g_strdup()
only results in a leak:

Direct leak of 6 byte(s) in 1 object(s) allocated from:
    #0 0x56298023713e in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f13e) (BuildId: b2b9174a5a54707a7f76bca51cdc95d2aa08bac1)
    #1 0x7fba0ad39738 in g_malloc debian/build/deb/../../../glib/gmem.c:128:13
    #2 0x7fba0ad4e583 in g_strdup debian/build/deb/../../../glib/gstrfuncs.c:361:17
    #3 0x56298036b16e in migrate_set_ports tests/qtest/migration-helpers.c:145:49
    #4 0x56298036ad1c in migrate_qmp tests/qtest/migration-helpers.c:228:9
    #5 0x56298035b3dd in test_precopy_common tests/qtest/migration-test.c:1820:5
    #6 0x5629803549dc in test_multifd_tcp_channels_none tests/qtest/migration-test.c:3077:5
    #7 0x56298036d427 in migration_test_wrapper tests/qtest/migration-helpers.c:456:5

Drop the unnecessary g_strdup() call.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/migration-helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 7cbb9831e76..a43d180c807 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -142,7 +142,7 @@ static void migrate_set_ports(QTestState *to, QList *channel_list)
             qdict_haskey(addr, "port") &&
             (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
                 addr_port = qdict_get_str(addr, "port");
-                qdict_put_str(addrdict, "port", g_strdup(addr_port));
+                qdict_put_str(addrdict, "port", addr_port);
         }
     }
 
-- 
2.34.1

Re: [PATCH for-9.2 7/9] tests/qtest/migration-helpers: Don't dup argument to qdict_put_str()

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> In migrate_set_ports() we call qdict_put_str() with a value string
> which we g_strdup(). However qdict_put_str() takes a copy of the
> value string, it doesn't take ownership of it, so the g_strdup()
> only results in a leak:
>
> Direct leak of 6 byte(s) in 1 object(s) allocated from:
>     #0 0x56298023713e in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f13e) (BuildId: b2b9174a5a54707a7f76bca51cdc95d2aa08bac1)
>     #1 0x7fba0ad39738 in g_malloc debian/build/deb/../../../glib/gmem.c:128:13
>     #2 0x7fba0ad4e583 in g_strdup debian/build/deb/../../../glib/gstrfuncs.c:361:17
>     #3 0x56298036b16e in migrate_set_ports tests/qtest/migration-helpers.c:145:49
>     #4 0x56298036ad1c in migrate_qmp tests/qtest/migration-helpers.c:228:9
>     #5 0x56298035b3dd in test_precopy_common tests/qtest/migration-test.c:1820:5
>     #6 0x5629803549dc in test_multifd_tcp_channels_none tests/qtest/migration-test.c:3077:5
>     #7 0x56298036d427 in migration_test_wrapper tests/qtest/migration-helpers.c:456:5
>
> Drop the unnecessary g_strdup() call.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/qtest/migration-helpers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index 7cbb9831e76..a43d180c807 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -142,7 +142,7 @@ static void migrate_set_ports(QTestState *to, QList *channel_list)
>              qdict_haskey(addr, "port") &&
>              (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
>                  addr_port = qdict_get_str(addr, "port");
> -                qdict_put_str(addrdict, "port", g_strdup(addr_port));
> +                qdict_put_str(addrdict, "port", addr_port);
>          }
>      }

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH for-9.2 8/9] tests/qtest/migration-test: Don't strdup in get_dirty_rate()

[Peter Maydell]

We g_strdup() the "status" string we get out of the qdict in
get_dirty_rate(), but we never free it.  Since we only use this
string while the dictionary is still valid, we don't need to strdup
at all; drop the unnecessary call to avoid this leak:

Direct leak of 18 byte(s) in 2 object(s) allocated from:
    #0 0x564b3e01913e in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f13e) (BuildId: d6403a811332fcc846f93c45e23abfd06d1e67c4)
    #1 0x7f2f278ff738 in g_malloc debian/build/deb/../../../glib/gmem.c:128:13
    #2 0x7f2f27914583 in g_strdup debian/build/deb/../../../glib/gstrfuncs.c:361:17
    #3 0x564b3e14bb5b in get_dirty_rate tests/qtest/migration-test.c:3447:14
    #4 0x564b3e138e00 in test_vcpu_dirty_limit tests/qtest/migration-test.c:3565:16
    #5 0x564b3e14f417 in migration_test_wrapper tests/qtest/migration-helpers.c:456:5

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/migration-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index a659609ccb0..04122120987 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -3435,7 +3435,7 @@ static void wait_for_calc_dirtyrate_complete(QTestState *who,
 static int64_t get_dirty_rate(QTestState *who)
 {
     QDict *rsp_return;
-    gchar *status;
+    const char *status;
     QList *rates;
     const QListEntry *entry;
     QDict *rate;
@@ -3444,7 +3444,7 @@ static int64_t get_dirty_rate(QTestState *who)
     rsp_return = query_dirty_rate(who);
     g_assert(rsp_return);
 
-    status = g_strdup(qdict_get_str(rsp_return, "status"));
+    status = qdict_get_str(rsp_return, "status");
     g_assert(status);
     g_assert_cmpstr(status, ==, "measured");
 
-- 
2.34.1

Re: [PATCH for-9.2 8/9] tests/qtest/migration-test: Don't strdup in get_dirty_rate()

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> We g_strdup() the "status" string we get out of the qdict in
> get_dirty_rate(), but we never free it.  Since we only use this
> string while the dictionary is still valid, we don't need to strdup
> at all; drop the unnecessary call to avoid this leak:
>
> Direct leak of 18 byte(s) in 2 object(s) allocated from:
>     #0 0x564b3e01913e in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f13e) (BuildId: d6403a811332fcc846f93c45e23abfd06d1e67c4)
>     #1 0x7f2f278ff738 in g_malloc debian/build/deb/../../../glib/gmem.c:128:13
>     #2 0x7f2f27914583 in g_strdup debian/build/deb/../../../glib/gstrfuncs.c:361:17
>     #3 0x564b3e14bb5b in get_dirty_rate tests/qtest/migration-test.c:3447:14
>     #4 0x564b3e138e00 in test_vcpu_dirty_limit tests/qtest/migration-test.c:3565:16
>     #5 0x564b3e14f417 in migration_test_wrapper tests/qtest/migration-helpers.c:456:5
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/qtest/migration-test.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index a659609ccb0..04122120987 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -3435,7 +3435,7 @@ static void wait_for_calc_dirtyrate_complete(QTestState *who,
>  static int64_t get_dirty_rate(QTestState *who)
>  {
>      QDict *rsp_return;
> -    gchar *status;
> +    const char *status;
>      QList *rates;
>      const QListEntry *entry;
>      QDict *rate;
> @@ -3444,7 +3444,7 @@ static int64_t get_dirty_rate(QTestState *who)
>      rsp_return = query_dirty_rate(who);
>      g_assert(rsp_return);
>  
> -    status = g_strdup(qdict_get_str(rsp_return, "status"));
> +    status = qdict_get_str(rsp_return, "status");
>      g_assert(status);
>      g_assert_cmpstr(status, ==, "measured");

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH for-9.2 9/9] tests/qtest/migration-test: Don't leak QTestState in test_multifd_tcp_cancel()

[Peter Maydell]

In test_multifd_tcp_cancel() we create three QEMU processes: 'from',
'to' and 'to2'.  We clean up (via qtest_quit()) 'from' and 'to2' when
we call test_migrate_end(), but never clean up 'to', which results in
this leak:

Direct leak of 336 byte(s) in 1 object(s) allocated from:
    #0 0x55e984fcd328 in __interceptor_calloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f328) (BuildId: 710d409b68bb04427009e9ca6e1b63ff8af785d3)
    #1 0x7f0878b39c50 in g_malloc0 debian/build/deb/../../../glib/gmem.c:161:13
    #2 0x55e98503a172 in qtest_spawn_qemu tests/qtest/libqtest.c:397:21
    #3 0x55e98502bc4a in qtest_init_internal tests/qtest/libqtest.c:471:9
    #4 0x55e98502c5b7 in qtest_init_with_env tests/qtest/libqtest.c:533:21
    #5 0x55e9850eef0f in test_migrate_start tests/qtest/migration-test.c:857:11
    #6 0x55e9850eb01d in test_multifd_tcp_cancel tests/qtest/migration-test.c:3297:9
    #7 0x55e985103407 in migration_test_wrapper tests/qtest/migration-helpers.c:456:5

Call qtest_quit() on 'to' to clean it up once it has exited.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/migration-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 04122120987..169ef0209c7 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -3322,6 +3322,7 @@ static void test_multifd_tcp_cancel(void)
     /* Make sure QEMU process "to" exited */
     qtest_set_expected_status(to, EXIT_FAILURE);
     qtest_wait_qemu(to);
+    qtest_quit(to);
 
     args = (MigrateStart){
         .only_target = true,
-- 
2.34.1

Re: [PATCH for-9.2 9/9] tests/qtest/migration-test: Don't leak QTestState in test_multifd_tcp_cancel()

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> In test_multifd_tcp_cancel() we create three QEMU processes: 'from',
> 'to' and 'to2'.  We clean up (via qtest_quit()) 'from' and 'to2' when
> we call test_migrate_end(), but never clean up 'to', which results in
> this leak:
>
> Direct leak of 336 byte(s) in 1 object(s) allocated from:
>     #0 0x55e984fcd328 in __interceptor_calloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f328) (BuildId: 710d409b68bb04427009e9ca6e1b63ff8af785d3)
>     #1 0x7f0878b39c50 in g_malloc0 debian/build/deb/../../../glib/gmem.c:161:13
>     #2 0x55e98503a172 in qtest_spawn_qemu tests/qtest/libqtest.c:397:21
>     #3 0x55e98502bc4a in qtest_init_internal tests/qtest/libqtest.c:471:9
>     #4 0x55e98502c5b7 in qtest_init_with_env tests/qtest/libqtest.c:533:21
>     #5 0x55e9850eef0f in test_migrate_start tests/qtest/migration-test.c:857:11
>     #6 0x55e9850eb01d in test_multifd_tcp_cancel tests/qtest/migration-test.c:3297:9
>     #7 0x55e985103407 in migration_test_wrapper tests/qtest/migration-helpers.c:456:5
>
> Call qtest_quit() on 'to' to clean it up once it has exited.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/qtest/migration-test.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 04122120987..169ef0209c7 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -3322,6 +3322,7 @@ static void test_multifd_tcp_cancel(void)
>      /* Make sure QEMU process "to" exited */
>      qtest_set_expected_status(to, EXIT_FAILURE);
>      qtest_wait_qemu(to);
> +    qtest_quit(to);
>  
>      args = (MigrateStart){
>          .only_target = true,

Reviewed-by: Fabiano Rosas <farosas@suse.de>