migration/options: Fix leaks in StrOrNull accessors

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

Posted by Fabiano Rosas 2 days, 8 hours ago

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

Posted by Prasad Pandit 1 day, 22 hours ago

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

Posted by Fabiano Rosas 1 day, 20 hours ago

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

Posted by Prasad Pandit 1 day, 19 hours ago

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

Posted by Fabiano Rosas 1 day, 19 hours ago

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