qtest/qom-test: Leak plug, faster, better coverage

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

Posted by Markus Armbruster 3 months, 3 weeks ago

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);
 }
 
-- 
2.49.0

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

Posted by Steven Sistare 3 months, 1 week ago

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.

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.

- Steve

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

Posted by Markus Armbruster 3 months, 1 week ago

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

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

Posted by Steven Sistare 3 months, 1 week ago

On 8/5/2025 2:54 AM, Markus Armbruster wrote:
> 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

Agreed.

- Steve