[PATCH 0/6] qtest: Fix some memory issues

[PATCH 0/6] qtest: Fix some memory issues

[Fabiano Rosas]

I'm trying to get the qtests to run with asan/valgrind without any
warnings so I can run a test before every PR. Here are a few fixes.

CI run: https://gitlab.com/farosas/qemu/-/pipelines/1580782506

Fabiano Rosas (6):
  tests/qtest/migration: Do proper cleanup in the dirty_limit test
  tests/qtest/migration: Initialize buffer in probe_o_direct_support
  tests/qtest/bios-tables-test: Free tables at dump_aml_files
  tests/qtest/virtio-iommu-test: Don't pass uninitialized data into
    qtest_memwrite
  tests/qtest/qos-test: Plug a couple of leaks
  tests/qtest/test-x86-cpuid-compat: Free allocated memory

 tests/qtest/bios-tables-test.c      |  1 +
 tests/qtest/migration-helpers.c     |  1 +
 tests/qtest/migration-test.c        |  5 +++++
 tests/qtest/qos-test.c              | 35 ++++++++++++++++++++---------
 tests/qtest/test-x86-cpuid-compat.c |  4 ++++
 tests/qtest/virtio-iommu-test.c     |  4 ++--
 6 files changed, 38 insertions(+), 12 deletions(-)

-- 
2.35.3

[PATCH 1/6] tests/qtest/migration: Do proper cleanup in the dirty_limit test

[Fabiano Rosas]

The dirty_limit test does two migrations in a row and is leaking the
first 'to' instance. Do proper cleanup.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 74d3000198..c3d54f1236 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -3682,6 +3682,11 @@ static void test_migrate_dirty_limit(void)
     migrate_cancel(from);
     wait_for_migration_status(from, "cancelled", NULL);
 
+    /* destination always fails after cancel */
+    migration_event_wait(to, "failed");
+    qtest_set_expected_status(to, EXIT_FAILURE);
+    qtest_quit(to);
+
     /* Check if dirty limit throttle switched off, set timeout 1ms */
     do {
         throttle_us_per_full =
-- 
2.35.3

Re: [PATCH 1/6] tests/qtest/migration: Do proper cleanup in the dirty_limit test

[Peter Xu]

On Mon, Dec 09, 2024 at 05:44:22PM -0300, Fabiano Rosas wrote:
> The dirty_limit test does two migrations in a row and is leaking the
> first 'to' instance. Do proper cleanup.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH 2/6] tests/qtest/migration: Initialize buffer in probe_o_direct_support

[Fabiano Rosas]

Valgrind complains about the probe_o_direct_support() function reading
from an uninitialized buffer. For probing O_DIRECT support we don't
actually need to write to the file, just make sure the pwrite call
doesn't reject the write. Still, write zeroes to the buffer to
suppress the warning.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 3f8ba7fa8e..981910ba35 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -496,6 +496,7 @@ bool probe_o_direct_support(const char *tmpfs)
 
     buf = qemu_try_memalign(len, len);
     g_assert(buf);
+    memset(buf, 0, len);
 
     ret = pwrite(fd, buf, len, offset);
     unlink(filename);
-- 
2.35.3

Re: [PATCH 2/6] tests/qtest/migration: Initialize buffer in probe_o_direct_support

[Philippe Mathieu-Daudé]

On 9/12/24 21:44, Fabiano Rosas wrote:
> Valgrind complains about the probe_o_direct_support() function reading
> from an uninitialized buffer. For probing O_DIRECT support we don't
> actually need to write to the file, just make sure the pwrite call
> doesn't reject the write. Still, write zeroes to the buffer to
> suppress the warning.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/migration-helpers.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index 3f8ba7fa8e..981910ba35 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -496,6 +496,7 @@ bool probe_o_direct_support(const char *tmpfs)
>   
>       buf = qemu_try_memalign(len, len);
>       g_assert(buf);

(we could directly use qemu_memalign here)

> +    memset(buf, 0, len);
>   
>       ret = pwrite(fd, buf, len, offset);
>       unlink(filename);

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/6] tests/qtest/migration: Initialize buffer in probe_o_direct_support

[Peter Xu]

On Mon, Dec 09, 2024 at 05:44:23PM -0300, Fabiano Rosas wrote:
> Valgrind complains about the probe_o_direct_support() function reading
> from an uninitialized buffer. For probing O_DIRECT support we don't
> actually need to write to the file, just make sure the pwrite call
> doesn't reject the write. Still, write zeroes to the buffer to
> suppress the warning.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH 3/6] tests/qtest/bios-tables-test: Free tables at dump_aml_files

[Fabiano Rosas]

The dump_aml_files() function calls load_expected_aml() to allocate
the tables but never frees it. Add the missing call to
free_test_data().

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/bios-tables-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 16d0ffbdf6..1cf4e3f7ef 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -292,6 +292,7 @@ static void dump_aml_files(test_data *data, bool rebuild)
 
         g_free(aml_file);
     }
+    free_test_data(&exp_data);
 }
 
 static bool create_tmp_asl(AcpiSdtTable *sdt)
-- 
2.35.3

Re: [PATCH 3/6] tests/qtest/bios-tables-test: Free tables at dump_aml_files

[Philippe Mathieu-Daudé]

On 9/12/24 21:44, Fabiano Rosas wrote:
> The dump_aml_files() function calls load_expected_aml() to allocate
> the tables but never frees it. Add the missing call to
> free_test_data().
> 

This is also Coverity CID 1549449 (RESOURCE_LEAK)

> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/bios-tables-test.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 16d0ffbdf6..1cf4e3f7ef 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -292,6 +292,7 @@ static void dump_aml_files(test_data *data, bool rebuild)
>   
>           g_free(aml_file);
>       }
> +    free_test_data(&exp_data);
>   }
>   
>   static bool create_tmp_asl(AcpiSdtTable *sdt)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 4/6] tests/qtest/virtio-iommu-test: Don't pass uninitialized data into qtest_memwrite

[Fabiano Rosas]

Valgrind complains about:

Use of uninitialised value of size 8
&
Conditional jump or move depends on uninitialised value(s)

both at:
   at 0x5265931: _itoa_word (_itoa.c:180)
   by 0x527EEC7: __vfprintf_internal (vfprintf-internal.c:1687)
   by 0x528C8B0: __vsprintf_internal (iovsprintf.c:96)
   by 0x526B920: sprintf (sprintf.c:30)
   by 0x1296C7: qtest_memwrite (libqtest.c:1273)
   by 0x193C04: send_map (virtio-iommu-test.c:125)
   by 0x194392: test_attach_detach (virtio-iommu-test.c:214)
   by 0x17BDE7: run_one_test (qos-test.c:181)
   by 0x4B0699D: test_case_run (gtestutils.c:2900)
   by 0x4B0699D: g_test_run_suite_internal (gtestutils.c:2988)
   by 0x4B068B2: g_test_run_suite_internal (gtestutils.c:3005)
   by 0x4B068B2: g_test_run_suite_internal (gtestutils.c:3005)
   by 0x4B068B2: g_test_run_suite_internal (gtestutils.c:3005)
 Uninitialised value was created by a stack allocation
   at 0x193AFD: send_map (virtio-iommu-test.c:103)

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/virtio-iommu-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/virtio-iommu-test.c b/tests/qtest/virtio-iommu-test.c
index afb225971d..98ffa27912 100644
--- a/tests/qtest/virtio-iommu-test.c
+++ b/tests/qtest/virtio-iommu-test.c
@@ -105,7 +105,7 @@ static int send_map(QTestState *qts, QVirtioIOMMU *v_iommu,
     QVirtQueue *vq = v_iommu->vq;
     uint64_t ro_addr, wr_addr;
     uint32_t free_head;
-    struct virtio_iommu_req_map req;
+    struct virtio_iommu_req_map req = {};
     size_t ro_size = sizeof(req) - sizeof(struct virtio_iommu_req_tail);
     size_t wr_size = sizeof(struct virtio_iommu_req_tail);
     struct virtio_iommu_req_tail buffer;
@@ -147,7 +147,7 @@ static int send_unmap(QTestState *qts, QVirtioIOMMU *v_iommu,
     QVirtQueue *vq = v_iommu->vq;
     uint64_t ro_addr, wr_addr;
     uint32_t free_head;
-    struct virtio_iommu_req_unmap req;
+    struct virtio_iommu_req_unmap req = {};
     size_t ro_size = sizeof(req) - sizeof(struct virtio_iommu_req_tail);
     size_t wr_size = sizeof(struct virtio_iommu_req_tail);
     struct virtio_iommu_req_tail buffer;
-- 
2.35.3

Re: [PATCH 4/6] tests/qtest/virtio-iommu-test: Don't pass uninitialized data into qtest_memwrite

[Philippe Mathieu-Daudé]

On 9/12/24 21:44, Fabiano Rosas wrote:
> Valgrind complains about:
> 
> Use of uninitialised value of size 8
> &
> Conditional jump or move depends on uninitialised value(s)
> 
> both at:
>     at 0x5265931: _itoa_word (_itoa.c:180)
>     by 0x527EEC7: __vfprintf_internal (vfprintf-internal.c:1687)
>     by 0x528C8B0: __vsprintf_internal (iovsprintf.c:96)
>     by 0x526B920: sprintf (sprintf.c:30)
>     by 0x1296C7: qtest_memwrite (libqtest.c:1273)
>     by 0x193C04: send_map (virtio-iommu-test.c:125)
>     by 0x194392: test_attach_detach (virtio-iommu-test.c:214)
>     by 0x17BDE7: run_one_test (qos-test.c:181)
>     by 0x4B0699D: test_case_run (gtestutils.c:2900)
>     by 0x4B0699D: g_test_run_suite_internal (gtestutils.c:2988)
>     by 0x4B068B2: g_test_run_suite_internal (gtestutils.c:3005)
>     by 0x4B068B2: g_test_run_suite_internal (gtestutils.c:3005)
>     by 0x4B068B2: g_test_run_suite_internal (gtestutils.c:3005)
>   Uninitialised value was created by a stack allocation
>     at 0x193AFD: send_map (virtio-iommu-test.c:103)
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/virtio-iommu-test.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 5/6] tests/qtest/qos-test: Plug a couple of leaks

[Fabiano Rosas]

The walk_path() function of qos-test.c, which walks the graph and adds
tests to the test suite uses GLib's g_test_add_data_func_full()
function:

g_test_add_data_func_full (const char     *testpath,
                           gpointer        test_data,
                           GTestDataFunc   test_func,
                           GDestroyNotify  data_free_func)

Despite GLib's documentation stating that @data_free_func is a
destructor for @test_data, this is not the case. The destructor is
supposed to be paired with a constructor, which GLib only accepts via
g_test_create_case().

Providing externally allocated data plus a destructor function only
works if the test is guaranteed to execute, otherwise the test_data is
never deallocated.

Due to how subprocessess are implemented in qos-test, each test gets
added twice and an extra test gets added per subprocess. In a regular
run, the extra subprocess will not be executed and in a single test
run (-p), none of the other tests will be executed (+1 per
subprocess), leaking 'path_vec' and 'subprocess_path'.

Fix this by storing all the path vectors in a list and freeing them
all at the end of the program (including subprocess invocations) and
moving the allocation of 'subprocess_path' into run_one_subprocess().

While here add some documentation explaining why the graph needs to be
walked twice and tests re-added.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/qos-test.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index 114f6bef27..67abc660b5 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -31,6 +31,7 @@
 #include "libqos/qos_external.h"
 
 static char *old_path;
+static GSList *path_vecs;
 
 
 /**
@@ -183,11 +184,16 @@ static void run_one_test(const void *arg)
 
 static void subprocess_run_one_test(const void *arg)
 {
-    const gchar *path = arg;
-    g_test_trap_subprocess(path, 180 * G_USEC_PER_SEC,
+    char **path_vec = (char **) arg;
+    gchar *path = g_strjoinv("/", path_vec + 1);
+    gchar *subprocess_path = g_strdup_printf("/%s/subprocess", path);
+
+    g_test_trap_subprocess(subprocess_path, 180 * G_USEC_PER_SEC,
                            G_TEST_SUBPROCESS_INHERIT_STDOUT |
                            G_TEST_SUBPROCESS_INHERIT_STDERR);
     g_test_trap_assert_passed();
+    g_free(path);
+    g_free(subprocess_path);
 }
 
 static void destroy_pathv(void *arg)
@@ -239,6 +245,7 @@ static void walk_path(QOSGraphNode *orig_path, int len)
     GString *cmd_line = g_string_new("");
     GString *cmd_line2 = g_string_new("");
 
+    path_vecs = g_slist_append(path_vecs, path_vec);
     path = qos_graph_get_node(node_name); /* root */
     node_name = qos_graph_edge_get_dest(path->path_edge); /* machine name */
 
@@ -298,15 +305,15 @@ static void walk_path(QOSGraphNode *orig_path, int len)
     path_vec[0] = g_string_free(cmd_line, false);
 
     if (path->u.test.subprocess) {
-        gchar *subprocess_path = g_strdup_printf("/%s/%s/subprocess",
-                                                 qtest_get_arch(), path_str);
-        qtest_add_data_func_full(path_str, subprocess_path,
-                                 subprocess_run_one_test, g_free);
-        g_test_add_data_func_full(subprocess_path, path_vec,
-                                  run_one_test, destroy_pathv);
+        gchar *subprocess_path = g_strdup_printf("%s/%s", path_str,
+                                                 "subprocess");
+
+        qtest_add_data_func(path_str, path_vec, subprocess_run_one_test);
+        qtest_add_data_func(subprocess_path, path_vec, run_one_test);
+
+        g_free(subprocess_path);
     } else {
-        qtest_add_data_func_full(path_str, path_vec,
-                                 run_one_test, destroy_pathv);
+        qtest_add_data_func(path_str, path_vec, run_one_test);
     }
 
     g_free(path_str);
@@ -332,6 +339,13 @@ int main(int argc, char **argv, char** envp)
 
     if (g_test_subprocess()) {
         qos_printf("qos_test running single test in subprocess\n");
+
+        /*
+         * Although this invocation was done to run a single test in a
+         * subprocess, gtester doesn't expose the test name, so still
+         * execute the rest of main(), including adding all tests once
+         * more in order for g_test_run() to find the /subprocess.
+         */
     }
 
     if (g_test_verbose()) {
@@ -354,5 +368,6 @@ int main(int argc, char **argv, char** envp)
     qtest_end();
     qos_graph_destroy();
     g_free(old_path);
+    g_slist_free_full(path_vecs, (GDestroyNotify)destroy_pathv);
     return 0;
 }
-- 
2.35.3

[PATCH 6/6] tests/qtest/test-x86-cpuid-compat: Free allocated memory

[Fabiano Rosas]

Free the test arguments after test execution.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/test-x86-cpuid-compat.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qtest/test-x86-cpuid-compat.c b/tests/qtest/test-x86-cpuid-compat.c
index b9e7e5ef7b..45f35ab9b0 100644
--- a/tests/qtest/test-x86-cpuid-compat.c
+++ b/tests/qtest/test-x86-cpuid-compat.c
@@ -65,6 +65,8 @@ static void test_cpuid_prop(const void *data)
 
     qobject_unref(value);
     g_free(path);
+    g_free((void *)args->cmdline);
+    g_free((void *)data);
 }
 
 static void add_cpuid_test(const char *name, const char *cpu,
@@ -161,6 +163,8 @@ static void test_feature_flag(const void *data)
     qobject_unref(present);
     qobject_unref(filtered);
     g_free(path);
+    g_free((void *)args->cmdline);
+    g_free((void *)data);
 }
 
 /*
-- 
2.35.3

Re: [PATCH 0/6] qtest: Fix some memory issues

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> I'm trying to get the qtests to run with asan/valgrind without any
> warnings so I can run a test before every PR. Here are a few fixes.
>
> CI run: https://gitlab.com/farosas/qemu/-/pipelines/1580782506
>
> Fabiano Rosas (6):
>   tests/qtest/migration: Do proper cleanup in the dirty_limit test
>   tests/qtest/migration: Initialize buffer in probe_o_direct_support
>   tests/qtest/bios-tables-test: Free tables at dump_aml_files
>   tests/qtest/virtio-iommu-test: Don't pass uninitialized data into
>     qtest_memwrite
>   tests/qtest/qos-test: Plug a couple of leaks
>   tests/qtest/test-x86-cpuid-compat: Free allocated memory
>
>  tests/qtest/bios-tables-test.c      |  1 +
>  tests/qtest/migration-helpers.c     |  1 +
>  tests/qtest/migration-test.c        |  5 +++++
>  tests/qtest/qos-test.c              | 35 ++++++++++++++++++++---------
>  tests/qtest/test-x86-cpuid-compat.c |  4 ++++
>  tests/qtest/virtio-iommu-test.c     |  4 ++--
>  6 files changed, 38 insertions(+), 12 deletions(-)

Queuing 1-4 for qtest-next.