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 17:51, Fabiano Rosas <farosas@suse.de> wrote:
>> >> 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("");                 <== [1]
>> >> +        *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.
>
> * Then do we need to remove it from get_StrOrNull()?  <== [1] above.
>

The visitor behaves differently depending on the direction
(input/output), the getter is providing a new object with a default
empty string.

>> 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.
>
> * Hmmn, okay. Coverity reported a leak for it in set_StrOrNull(), not
> other places?
>

Well, the setter is slightly different because this property is a
wrapper that contains a string, so the string visitor in the setter can
(in theory) fail and that would indeed leak (the outer object's
memory). But the other properties are returning the string directly,
without any extra memory allocation so, as in this getter, they simply
return on (the theoretical) failure.

IOW, there is nothing to leak in the other setters and the other
getters, on failure leave the caller to deal with the memory that was
already allocated.

Now, I just double-checked and what I said in the previous email is not
entirely accurate:

  "I think in practice it's unlikely that the string visitors will ever
  set errp because they are just a g_strdup() usually." -- me

the input visitor can actually set errp, but it does so before
allocating the string.

> Thank you.
> ---
>   - Prasad