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

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> 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 ?

g_test_queue_destroy will only call qtest_free_machine_info during the
test teardown phase:

#0  qtest_free_machine_info (data=0x555555677870) at 
../tests/qtest/libqtest.c:1289
#1  0x00007ffff7b1d9d1 in ?? () from /usr/lib64/libglib-2.0.so.0
#2  0x00007ffff7b1d8b3 in ?? () from /usr/lib64/libglib-2.0.so.0
#3  0x00007ffff7b1d8b3 in ?? () from /usr/lib64/libglib-2.0.so.0
#4  0x00007ffff7b1de82 in g_test_run_suite () from /usr/lib64/libglib-2.0.so.0
#5  0x00007ffff7b1deab in g_test_run () from /usr/lib64/libglib-2.0.so.0
#6  0x0000555555561221 in main (argc=<optimized out>, argv=<optimized
#out>) at ../tests/qtest/qom-test.c:12

As long as 'machines' is static and not being exposed to the tests, I
think this should be fine.