Re: [PATCH] tests/qtest: Plug memory leaks in qtest_get_machines

[Thomas Huth <thuth@redhat.com>]

On 20/01/2023 20.44, Fabiano Rosas wrote:
> These leaks can be avoided:
> 
>   759 bytes in 61 blocks are still reachable in loss record 56 of 60
>      at 0x4034744: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>      by 0x4A88518: g_malloc (in /usr/lib64/libglib-2.0.so.0.7000.5)
>      by 0x4AA313E: g_strdup (in /usr/lib64/libglib-2.0.so.0.7000.5)
>      by 0x12083E: qtest_get_machines (libqtest.c:1323)
>      by 0x12098C: qtest_cb_for_every_machine (libqtest.c:1348)
>      by 0x11556C: main (test-hmp.c:160)
> 
>   992 bytes in 1 blocks are still reachable in loss record 57 of 60
>      at 0x4034744: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>      by 0x4A88518: g_malloc (in /usr/lib64/libglib-2.0.so.0.7000.5)
>      by 0x120725: qtest_get_machines (libqtest.c:1313)
>      by 0x12098C: qtest_cb_for_every_machine (libqtest.c:1348)
>      by 0x11556C: main (test-hmp.c:160)
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/libqtest.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 6b2216cb20..65abac5029 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -1285,6 +1285,18 @@ struct MachInfo {
>       char *alias;
>   };
>   
> +static void qtest_free_machine_info(gpointer data)
> +{
> +    struct MachInfo *machines = data;
> +    int i;
> +
> +    for (i = 0; machines[i].name != NULL; i++) {
> +        g_free((void *)machines[i].name); > +        g_free((void *)machines[i].alias);

I'd suggest setting .name and .alias to NULL after freeing them, to avoid 
that danling pointers are left behind.

> +    }
> +    g_free(machines);
> +}
> +
>   /*
>    * Returns an array with pointers to the available machine names.
>    * The terminating entry has the name set to NULL.
> @@ -1336,6 +1348,8 @@ static struct MachInfo *qtest_get_machines(void)
>       qobject_unref(response);
>   
>       memset(&machines[idx], 0, sizeof(struct MachInfo)); /* Terminating entry */
> +    g_test_queue_destroy(qtest_free_machine_info, machines);

So this frees the machines structure...

>       return machines;

... but here it gets returned, too? ... that looks wrong. Did you maybe 
rather want to free it at the end of qtest_cb_for_every_machine() and 
qtest_has_machine ?

  Thomas

>   }
>