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

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> On 23/01/2023 14.32, Fabiano Rosas wrote:
>> 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.
>
> Ah, thanks for the explanation, I really got that wrong.
>
> But I think g_test_queue_destroy() might still be the wrong thing to use 
> here. I added a fprintf() to your new qtest_free_machine_info() funcion, and 
> it seems to be called once at the end of the very first test already.

Yes, because we currently only use qtest_get_machines to construct the
test paths. So it is only needed before the tests.

The g_test_queue_destroy function is actually attaching the callback to
the first test.

> So if anything else calls qtest_get_machines() again after the first
> test finished, it will crash due to the dangling static machine
> pointer.

Ah, my reply about it being fine was while looking at the v2 which sets
'machines' to NULL. So we would go around qtest_get_machines once more and
register qtest_free_machine_info a second time. Every time we have to
fetch the machines, we register the cleanup callback.

> So maybe the static machine pointer should be moved outside of the function 
> and then be released from qtest_quit() instead?

Hm.. let me give it a try. I'm not sure if it works because we call
qtest_quit while still using the machines array. Maybe if I move it out
and do the cleanup in qtest_cb_for_every_machine...

> (Also, it's valgrind that you used, isn't it? - I wonder why it's 
> complaining here at all since the pointer to the memory should still be 
> valid at the end?)

valgrind is complaining about the memory not being explicitly freed at
any point. I guess "leak" was not the most precise term to use in the
commit message.