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

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Jan 22, 2026 at 08:24:56PM -0300, Fabiano Rosas 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
>> 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;
>
> Yep, fixes the leak.
>
> However does it then mean a get() can internally update this Property?  It
> used to be NULL, now it's QTYPE_STRING "".
>
> It may make slightly more sense to me to have zero side effect on a get()
> call. Say, keeping *ptr==NULL after get() returns (as before), while we can
> use a temp string ("") for the visitor.
>
> I randomly checked another get() provider, e.g. get_drive() does that.
> Should we follow?
>

Hmm, I'm actually inclined to assert. This is code for the very specific
purpose of allowing compatibility of -global tls-* options which are
properties of the s->parameters object.

The only way the property could be NULL in the getter is if the setter
has failed. Is there a way we could make the setter fail without it
being a programming error? I don't see it.

- the default for the property is an empty string.

- setting via set-parameters bypasses the qdev code entirely.

- device_add is not reachable because of dc->user_creatable = false.

- object_set_propv and qom_set use a string visitor, so no type mismatch
  to trigger the qobject_to(QString, qobj) cast.

>>      } 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;
>>      if (!visit_type_str(v, name, &str_or_null->u.s, errp)) {
>> +        qapi_free_StrOrNull(str_or_null);
>
> This looks correct.  Or, simpler way is we only do g_new0() after the
> visit_type_str() successfully returned (and pass a temp char* into it).
>

I'll update it.

>>          return;
>>      }
>>  
>> -- 
>> 2.51.0
>>