Peter Xu <pet...@redhat.com> writes:
> On Mon, Jun 30, 2025 at 04:59:10PM -0300, Fabiano Rosas wrote:
>> The documentation of qobject_from_jsonv() states that it takes
>> ownership of any %p arguments passed in.
>>
>> Next patches will add config-passing to the tests, so take an extra
>> reference in the migrate_qmp* functions to ensure the config is not
>> freed from under us.
>>
>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>> ---
>> tests/qtest/migration/migration-qmp.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qtest/migration/migration-qmp.c
>> b/tests/qtest/migration/migration-qmp.c
>> index fb59741b2c..d82ac8c750 100644
>> --- a/tests/qtest/migration/migration-qmp.c
>> +++ b/tests/qtest/migration/migration-qmp.c
>> @@ -97,7 +97,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
>> }
>>
>> err = qtest_qmp_assert_failure_ref(
>> - who, "{ 'execute': 'migrate', 'arguments': %p}", args);
>> + who, "{ 'execute': 'migrate', 'arguments': %p}",
>> + qdict_clone_shallow(args));
>>
>> g_assert(qdict_haskey(err, "desc"));
>>
>> @@ -136,7 +137,8 @@ void migrate_qmp(QTestState *who, QTestState *to, const
>> char *uri,
>> }
>>
>> qtest_qmp_assert_success(who,
>> - "{ 'execute': 'migrate', 'arguments': %p}",
>> args);
>> + "{ 'execute': 'migrate', 'arguments': %p}",
>> + qdict_clone_shallow(args));
>> }
>>
>> void migrate_set_capability(QTestState *who, const char *capability,
>> @@ -174,7 +176,7 @@ void migrate_incoming_qmp(QTestState *to, const char
>> *uri, QObject *channels,
>> migrate_set_capability(to, "events", true);
>>
>> rsp = qtest_qmp(to, "{ 'execute': 'migrate-incoming', 'arguments': %p}",
>> - args);
>> + qdict_clone_shallow(args));
>
> Isn't it intentional to pass over the ownership in the three places here?
> I don't see otherwise where args got freed.
>
Hmm, I think I remember the issue being that qobject_from_jsonv() freed
before the object it reached QMP, so indeed this needs to be freed
before the migrate_qmp functions return.
I don't really understand why ASAN didn't spot it though. I'll fix, thanks!
> OTOH, I saw there're yet another three similar usages of %p in framework.c:
>
> x1:migration [migration-params-caps-no-config]$ git grep -A1 %p framework.c
> framework.c: migrate_qmp_fail(from, args->connect_uri, NULL, "{
> 'config': %p }",
> framework.c- args->start.config);
> --
> framework.c: migrate_qmp(from, to, args->connect_uri, NULL, "{ 'config':
> %p }",
> framework.c- args->start.config);
> --
> framework.c: migrate_incoming_qmp(to, args->connect_uri, NULL, "{
> 'config': %p }",
> framework.c- args->start.config);
>
> They seem to be suspecious instead, as they seem to have lost ownership of
> args->start.config, so args->start.config can start to point to garbage?
>
>>
>> if (!qdict_haskey(rsp, "return")) {
>> g_autoptr(GString) s = qobject_to_json_pretty(QOBJECT(rsp), true);
>> --
>> 2.35.3
>>
