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

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

[Fabiano Rosas]

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;
     } 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);
         return;
     }
 
-- 
2.51.0

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

[Prasad Pandit]

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

> 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?

>      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()?

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

Thank you.
---
  - Prasad

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

[Fabiano Rosas]

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

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

[Prasad Pandit]

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.

> 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?

Thank you.
---
  - Prasad

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

[Fabiano Rosas]

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

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

[Peter Xu]

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?

>      } 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).

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

-- 
Peter Xu

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

[Fabiano Rosas]

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
>> 

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

[Peter Xu]

On Mon, Jan 26, 2026 at 04:16:11PM -0300, Fabiano Rosas wrote:
> 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.

I agree, assert would even be better.  Then, maybe let's add a quick
comment for it?  Something like:

    /*
     * Assert for both str_or_null be present and not QNULL. 
     * Guaranteed because ...
     */
    assert(str_or_null && str_or_null->type != QTYPE_QNULL);

-- 
Peter Xu