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

[Thomas Huth]

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. So if 
anything else calls qtest_get_machines() again after the first test 
finished, it will crash due to the dangling static machine pointer.

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

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

 Thomas