Re: [PATCH] migration/options: Fix leaks in StrOrNull accessors

[Fabiano Rosas <farosas@suse.de>]

Prasad Pandit <ppandit@redhat.com> writes:

> On Fri, 23 Jan 2026 at 04:56, Fabiano Rosas <farosas@suse.de> wrote:
>> Fix a couple of leaks detected by Coverity. Both are currently
>> harmless because the visitor in the setter can never fail and the
>> whole of the getter is unused, it's only purpose at the moment is to
>
> * unused, it's -> unused. Its
>

Thanks

>> provide a complete implementation of the StrOrNull property.
>>
>> Fixes: CID 1643919
>> Fixes: CID 1643920
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> CI run: https://gitlab.com/farosas/qemu/-/pipelines/2280325023
>> ---
>>  migration/options.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/migration/options.c b/migration/options.c
>> index 9a5a39c886..9dc44a3736 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -225,6 +225,7 @@ static void get_StrOrNull(Object *obj, Visitor *v, const char *name,
>>          str_or_null = g_new0(StrOrNull, 1);
>>          str_or_null->type = QTYPE_QSTRING;
>>          str_or_null->u.s = g_strdup("");
>> +        *ptr = str_or_null;
>>      } else {
>>          /* the setter doesn't allow QNULL */
>>          assert(str_or_null->type != QTYPE_QNULL);
>> @@ -245,6 +246,7 @@ static void set_StrOrNull(Object *obj, Visitor *v, const char *name,
>>       */
>>      str_or_null->type = QTYPE_QSTRING;
>
> * Do we need to add:  str_or_null->u.s = g_strdup("");  here?
>

It would leak, the visitor will allocate new memory for it below.

>>      if (!visit_type_str(v, name, &str_or_null->u.s, errp)) {
>> +        qapi_free_StrOrNull(str_or_null);
>>          return;
>>      }
>>
>> --
>
> * visit_type_str() failure is handled in set_StrOrNull, but not in
> get_StrOrNull? Do we need to add qapi_free_StrOrNull(str_or_null) in
> get_StrOrNull()?
>

I'd rather leave to the caller. The errp will be set, I think it's
enough. Looking at the other instances of visit_type_str in qdev, they
all simply return without any handling. I think in practice it's
unlikely that the string visitors will ever set errp because they are
just a g_strdup() usually.

> * Otherwise it looks okay.
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
>
> Thank you.
> ---
>   - Prasad