Re: [PATCH 1/5] qtest/qom-test: Plug memory leak with -p

[Markus Armbruster <armbru@redhat.com>]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 7/25/2025 9:50 AM, Markus Armbruster wrote:
>> The machine name g_strdup()ed by add_machine_test_case() is freed by
>> test_machine().  Since the former runs for all machines, whereas the
>> latter runs only for the selected test case's machines, this leaks the
>> names of machines not selected, if any.  Harmless, but fix it anyway:
>> there is no need to dup in the first place, so don't.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/qtest/qom-test.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/tests/qtest/qom-test.c b/tests/qtest/qom-test.c
>> index 4ade1c728c..cb5dbfe329 100644
>> --- a/tests/qtest/qom-test.c
>> +++ b/tests/qtest/qom-test.c
>> @@ -220,7 +220,6 @@ static void test_machine(gconstpointer data)
>>      qobject_unref(response);
>>      qtest_quit(qts);
>> -    g_free((void *)machine);
>>  }
>>  
>>  static void add_machine_test_case(const char *mname)
>> @@ -228,7 +227,7 @@ static void add_machine_test_case(const char *mname)
>>      char *path;
>>      path = g_strdup_printf("qom/%s", mname);
>> -    qtest_add_data_func(path, g_strdup(mname), test_machine);
>> +    qtest_add_data_func(path, mname, test_machine);
>>      g_free(path);
>>  }
>
> This will break if qtest_cb_for_every_machine ever composes a machine name on the
> stack and passes that to add_machine_test_case.  Unlikely, but the strdup makes it
> future proof.

Hmm.

qtest obtains machine names via QMP on demand.  This is
qtest_get_machines().  Once gotten, they live forever.

Used to live forever, actually: commit 41b2eba4e5c (tests/qtest: Allow
qtest_get_machines to use an alternate QEMU binary) throws them away
when qtest_get_machines() is asked for another QEMU's machine names.
migrate_start() does that.  It appears get each one's machine names
twice, because it switches back and forth.  Wasteful.

Anyway, you have a point: more stupid shit happens below the hood than I
expected, and even more may be added in the future.

Moreover, at least test-hmp has the same leak.  Plugging it here and not
there makes no sense.  I'm dropping this patch for now.

> Also, mname is not the only leak.  path is also leaked when only a subset of
> tests are run:
>
>   qtest_add_data_func(path, ...)
>     gchar *path = g_strdup_printf(...)
>     g_test_add_data_func(path, ...)
>
> Leaking seems to be par for this course.  Maybe not worth fixing.

valgrind shows the machine name leak my patch plugs.  It does not show
@path leaking.

Let's have a closer look:

    static void add_machine_test_case(const char *mname)
    {
        char *path;

        path = g_strdup_printf("qom/%s", mname);
        qtest_add_data_func(path, mname, test_machine);
        g_free(path);
    }

Always frees @path.

    void qtest_add_data_func(const char *str, const void *data,
                             void (*fn)(const void *))
    {
        gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
        g_test_add_data_func(path, data, fn);
        g_free(path);
    }

Always frees @path.

g_test_add_data_func()'s contract[*] on its first argument: "The data is
owned by the caller of the function."

I can't see a leak.


[*] https://docs.gtk.org/glib/func.test_add_data_func.html