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

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

[Fabiano Rosas]

Fix a couple of leaks detected by Coverity. Both are currently
harmless.

- set_StrOrNull: the visitor should never fail unless there's a
programming error and a property of different type has been passed in.

Change it to only allocate memory after the visit call has returned
successfully.

- get_StrOrNull: the whole of the getter is unused, it's only purpose at
the moment is to provide a complete implementation of the StrOrNull
property. If it were used, it would always receive a non-NULL pointer
because this property is part of s->parameters and always initialized
by the setter.

Assert non-NULL instead of allocating a new object.

Fixes: CID 1643919
Fixes: CID 1643920
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/options.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index 1ffe85a2d8..93d11bba60 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -216,36 +216,36 @@ const size_t migration_properties_count = ARRAY_SIZE(migration_properties);
 static void get_StrOrNull(Object *obj, Visitor *v, const char *name,
                           void *opaque, Error **errp)
 {
-    const Property *prop = opaque;
-    StrOrNull **ptr = object_field_prop_ptr(obj, prop);
+    StrOrNull **ptr = object_field_prop_ptr(obj, opaque);
     StrOrNull *str_or_null = *ptr;
 
-    if (!str_or_null) {
-        str_or_null = g_new0(StrOrNull, 1);
-        str_or_null->type = QTYPE_QSTRING;
-        str_or_null->u.s = g_strdup("");
-    } else {
-        /* the setter doesn't allow QNULL */
-        assert(str_or_null->type != QTYPE_QNULL);
-    }
+    /*
+     * The property should never be NULL because it's part of
+     * s->parameters and a default value is always set. It should also
+     * never be QNULL as the setter doesn't allow it.
+     */
+    assert(str_or_null && str_or_null->type != QTYPE_QNULL);
     visit_type_str(v, name, &str_or_null->u.s, errp);
 }
 
 static void set_StrOrNull(Object *obj, Visitor *v, const char *name,
                           void *opaque, Error **errp)
 {
-    const Property *prop = opaque;
-    StrOrNull **ptr = object_field_prop_ptr(obj, prop);
-    StrOrNull *str_or_null = g_new0(StrOrNull, 1);
+    StrOrNull **ptr = object_field_prop_ptr(obj, opaque);
+    StrOrNull *str_or_null;
+    char *str;
+
+    if (!visit_type_str(v, name, &str, errp)) {
+        return;
+    }
 
     /*
      * Only str to keep compatibility, QNULL was never used via
      * command line.
      */
+    str_or_null = g_new0(StrOrNull, 1);
     str_or_null->type = QTYPE_QSTRING;
-    if (!visit_type_str(v, name, &str_or_null->u.s, errp)) {
-        return;
-    }
+    str_or_null->u.s = str;
 
     qapi_free_StrOrNull(*ptr);
     *ptr = str_or_null;
-- 
2.51.0

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

[Peter Xu]

On Tue, Jan 27, 2026 at 12:09:16PM -0300, Fabiano Rosas wrote:
> Fix a couple of leaks detected by Coverity. Both are currently
> harmless.
> 
> - set_StrOrNull: the visitor should never fail unless there's a
> programming error and a property of different type has been passed in.
> 
> Change it to only allocate memory after the visit call has returned
> successfully.
> 
> - get_StrOrNull: the whole of the getter is unused, it's only purpose at
> the moment is to provide a complete implementation of the StrOrNull
> property. If it were used, it would always receive a non-NULL pointer
> because this property is part of s->parameters and always initialized
> by the setter.
> 
> Assert non-NULL instead of allocating a new object.
> 
> Fixes: CID 1643919
> Fixes: CID 1643920
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

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

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> Fix a couple of leaks detected by Coverity. Both are currently
> harmless.

Details?  See below.

> - set_StrOrNull: the visitor should never fail unless there's a
> programming error and a property of different type has been passed in.

Really?  See below.

> Change it to only allocate memory after the visit call has returned
> successfully.
>
> - get_StrOrNull: the whole of the getter is unused, it's only purpose at
> the moment is to provide a complete implementation of the StrOrNull
> property. If it were used, it would always receive a non-NULL pointer
> because this property is part of s->parameters and always initialized
> by the setter.

Which "received" pointer exactly would always be non-null?  See below.

> Assert non-NULL instead of allocating a new object.
>
> Fixes: CID 1643919
> Fixes: CID 1643920
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/options.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/migration/options.c b/migration/options.c
> index 1ffe85a2d8..93d11bba60 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -216,36 +216,36 @@ const size_t migration_properties_count = ARRAY_SIZE(migration_properties);
>  static void get_StrOrNull(Object *obj, Visitor *v, const char *name,
>                            void *opaque, Error **errp)
>  {
> -    const Property *prop = opaque;

Yes, we don't actually need the variable.  However, it serves as
documentation of what @opaque is supposed to be.

For what it's worth, the property accessors defined in hw/core all
declare the variable

> -    StrOrNull **ptr = object_field_prop_ptr(obj, prop);
> +    StrOrNull **ptr = object_field_prop_ptr(obj, opaque);
>      StrOrNull *str_or_null = *ptr;
>  
> -    if (!str_or_null) {
> -        str_or_null = g_new0(StrOrNull, 1);
> -        str_or_null->type = QTYPE_QSTRING;
> -        str_or_null->u.s = g_strdup("");

The memory allocated here is never freed.  Harmless, because the code is
unreachable.

> -    } else {
> -        /* the setter doesn't allow QNULL */
> -        assert(str_or_null->type != QTYPE_QNULL);
> -    }
> +    /*
> +     * The property should never be NULL because it's part of
> +     * s->parameters and a default value is always set. It should also
> +     * never be QNULL as the setter doesn't allow it.
> +     */
> +    assert(str_or_null && str_or_null->type != QTYPE_QNULL);

Actually, @str_or_null cannot be null, because @obj isn't null, and
object_field_prop_ptr(obj, prop) returns a pointer into @obj.

>      visit_type_str(v, name, &str_or_null->u.s, errp);
>  }
>  
>  static void set_StrOrNull(Object *obj, Visitor *v, const char *name,
>                            void *opaque, Error **errp)
>  {
> -    const Property *prop = opaque;
> -    StrOrNull **ptr = object_field_prop_ptr(obj, prop);
> -    StrOrNull *str_or_null = g_new0(StrOrNull, 1);
> +    StrOrNull **ptr = object_field_prop_ptr(obj, opaque);
> +    StrOrNull *str_or_null;
> +    char *str;
> +
> +    if (!visit_type_str(v, name, &str, errp)) {
> +        return;
> +    }
>  
>      /*
>       * Only str to keep compatibility, QNULL was never used via
>       * command line.
>       */

This comment is from Fabiano's recent commit be346eb6673 (migration: Add
a qdev property for StrOrNull).  Fabiano, did you mean to write "Only
StrOrNull to keep compatibility"?

> +    str_or_null = g_new0(StrOrNull, 1);
>      str_or_null->type = QTYPE_QSTRING;
> -    if (!visit_type_str(v, name, &str_or_null->u.s, errp)) {

We leak @str_or_null here.  You plug the leak by delaying the allocation
until after visit_type_str().  Good.

> -        return;
> -    }
> +    str_or_null->u.s = str;
>  
>      qapi_free_StrOrNull(*ptr);
>      *ptr = str_or_null;

The function fails only when visit_type_str() fails, and
visit_type_str() fails only when v->type_str() fails.

Why are you certain it won't?

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

[Fabiano Rosas]

Markus Armbruster <armbru@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
>> Fix a couple of leaks detected by Coverity. Both are currently
>> harmless.
>
> Details?  See below.
>
>> - set_StrOrNull: the visitor should never fail unless there's a
>> programming error and a property of different type has been passed in.
>
> Really?  See below.
>
>> Change it to only allocate memory after the visit call has returned
>> successfully.
>>
>> - get_StrOrNull: the whole of the getter is unused, it's only purpose at
>> the moment is to provide a complete implementation of the StrOrNull
>> property. If it were used, it would always receive a non-NULL pointer
>> because this property is part of s->parameters and always initialized
>> by the setter.
>
> Which "received" pointer exactly would always be non-null?  See below.
>

Not technically "received", but derived from what has been received.

>> Assert non-NULL instead of allocating a new object.
>>
>> Fixes: CID 1643919
>> Fixes: CID 1643920
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/options.c | 32 ++++++++++++++++----------------
>>  1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/migration/options.c b/migration/options.c
>> index 1ffe85a2d8..93d11bba60 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -216,36 +216,36 @@ const size_t migration_properties_count = ARRAY_SIZE(migration_properties);
>>  static void get_StrOrNull(Object *obj, Visitor *v, const char *name,
>>                            void *opaque, Error **errp)
>>  {
>> -    const Property *prop = opaque;
>
> Yes, we don't actually need the variable.  However, it serves as
> documentation of what @opaque is supposed to be.
>
> For what it's worth, the property accessors defined in hw/core all
> declare the variable
>
>> -    StrOrNull **ptr = object_field_prop_ptr(obj, prop);
>> +    StrOrNull **ptr = object_field_prop_ptr(obj, opaque);
>>      StrOrNull *str_or_null = *ptr;
>>  
>> -    if (!str_or_null) {
>> -        str_or_null = g_new0(StrOrNull, 1);
>> -        str_or_null->type = QTYPE_QSTRING;
>> -        str_or_null->u.s = g_strdup("");
>
> The memory allocated here is never freed.  Harmless, because the code is
> unreachable.
>
>> -    } else {
>> -        /* the setter doesn't allow QNULL */
>> -        assert(str_or_null->type != QTYPE_QNULL);
>> -    }
>> +    /*
>> +     * The property should never be NULL because it's part of
>> +     * s->parameters and a default value is always set. It should also
>> +     * never be QNULL as the setter doesn't allow it.
>> +     */
>> +    assert(str_or_null && str_or_null->type != QTYPE_QNULL);
>
> Actually, @str_or_null cannot be null, because @obj isn't null, and
> object_field_prop_ptr(obj, prop) returns a pointer into @obj.
>

And that pointer must point to zeroes at some time. It cannot be that it
comes pre-set with the property value. I'm describing that it won't
point to zeroes anymore because the setter called from
.set_default_value will have already set that memory to some meaningful
value.

>>      visit_type_str(v, name, &str_or_null->u.s, errp);
>>  }
>>  
>>  static void set_StrOrNull(Object *obj, Visitor *v, const char *name,
>>                            void *opaque, Error **errp)
>>  {
>> -    const Property *prop = opaque;
>> -    StrOrNull **ptr = object_field_prop_ptr(obj, prop);
>> -    StrOrNull *str_or_null = g_new0(StrOrNull, 1);
>> +    StrOrNull **ptr = object_field_prop_ptr(obj, opaque);
>> +    StrOrNull *str_or_null;
>> +    char *str;
>> +
>> +    if (!visit_type_str(v, name, &str, errp)) {
>> +        return;
>> +    }
>>  
>>      /*
>>       * Only str to keep compatibility, QNULL was never used via
>>       * command line.
>>       */
>
> This comment is from Fabiano's recent commit be346eb6673 (migration: Add
> a qdev property for StrOrNull).  Fabiano, did you mean to write "Only
> StrOrNull to keep compatibility"?
>

No, it's "only str" indeed. The point is that -global tls-creds NULL was
never supported, so now that we have a property, this setter will never
create a QNULL to avoid ever passing a QNULL to code that used to
consume tls-creds and doesn't expect a QNULL value (the "compatibility"
part).

>> +    str_or_null = g_new0(StrOrNull, 1);
>>      str_or_null->type = QTYPE_QSTRING;
>> -    if (!visit_type_str(v, name, &str_or_null->u.s, errp)) {
>
> We leak @str_or_null here.  You plug the leak by delaying the allocation
> until after visit_type_str().  Good.
>
>> -        return;
>> -    }
>> +    str_or_null->u.s = str;
>>  
>>      qapi_free_StrOrNull(*ptr);
>>      *ptr = str_or_null;
>
> The function fails only when visit_type_str() fails, and
> visit_type_str() fails only when v->type_str() fails.
>
> Why are you certain it won't?

By inspection, I only see the failure points at qobject_input_type_str:

    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
    ...
    if (!qobj) {
        return false;
    }

    qstr = qobject_to(QString, qobj);
    if (!qstr) {
        error_setg(errp, "Invalid parameter type for '%s', expected: string",
                   full_name(qiv, name));
        return false;
    }

I'm relying on qdev's boilerplate not allowing incorrect invocations
from the user. If code has been written that allows the .set function to
be reached with a wrong type or a incorrect member name inside of a
struct, for instance, then I'm calling it a programming error.

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

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Fix a couple of leaks detected by Coverity. Both are currently
>>> harmless.
>>
>> Details?  See below.
>>
>>> - set_StrOrNull: the visitor should never fail unless there's a
>>> programming error and a property of different type has been passed in.
>>
>> Really?  See below.
>>
>>> Change it to only allocate memory after the visit call has returned
>>> successfully.
>>>
>>> - get_StrOrNull: the whole of the getter is unused, it's only purpose at
>>> the moment is to provide a complete implementation of the StrOrNull
>>> property. If it were used, it would always receive a non-NULL pointer
>>> because this property is part of s->parameters and always initialized
>>> by the setter.
>>
>> Which "received" pointer exactly would always be non-null?  See below.
>>
>
> Not technically "received", but derived from what has been received.
>
>>> Assert non-NULL instead of allocating a new object.
>>>
>>> Fixes: CID 1643919
>>> Fixes: CID 1643920
>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>  migration/options.c | 32 ++++++++++++++++----------------
>>>  1 file changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/migration/options.c b/migration/options.c
>>> index 1ffe85a2d8..93d11bba60 100644
>>> --- a/migration/options.c
>>> +++ b/migration/options.c
>>> @@ -216,36 +216,36 @@ const size_t migration_properties_count = ARRAY_SIZE(migration_properties);
>>>  static void get_StrOrNull(Object *obj, Visitor *v, const char *name,
>>>                            void *opaque, Error **errp)
>>>  {
>>> -    const Property *prop = opaque;
>>
>> Yes, we don't actually need the variable.  However, it serves as
>> documentation of what @opaque is supposed to be.
>>
>> For what it's worth, the property accessors defined in hw/core all
>> declare the variable
>>
>>> -    StrOrNull **ptr = object_field_prop_ptr(obj, prop);
>>> +    StrOrNull **ptr = object_field_prop_ptr(obj, opaque);
>>>      StrOrNull *str_or_null = *ptr;
>>>  
>>> -    if (!str_or_null) {
>>> -        str_or_null = g_new0(StrOrNull, 1);
>>> -        str_or_null->type = QTYPE_QSTRING;
>>> -        str_or_null->u.s = g_strdup("");
>>
>> The memory allocated here is never freed.  Harmless, because the code is
>> unreachable.
>>
>>> -    } else {
>>> -        /* the setter doesn't allow QNULL */
>>> -        assert(str_or_null->type != QTYPE_QNULL);
>>> -    }
>>> +    /*
>>> +     * The property should never be NULL because it's part of
>>> +     * s->parameters and a default value is always set. It should also
>>> +     * never be QNULL as the setter doesn't allow it.
>>> +     */
>>> +    assert(str_or_null && str_or_null->type != QTYPE_QNULL);
>>
>> Actually, @str_or_null cannot be null, because @obj isn't null, and
>> object_field_prop_ptr(obj, prop) returns a pointer into @obj.
>>
>
> And that pointer must point to zeroes at some time. It cannot be that it
> comes pre-set with the property value. I'm describing that it won't
> point to zeroes anymore because the setter called from
> .set_default_value will have already set that memory to some meaningful
> value.

The comment's "should never be NULL" part does not make sense to me.
Can we phrase it more clearly?

>>>      visit_type_str(v, name, &str_or_null->u.s, errp);
>>>  }
>>>  
>>>  static void set_StrOrNull(Object *obj, Visitor *v, const char *name,
>>>                            void *opaque, Error **errp)
>>>  {
>>> -    const Property *prop = opaque;
>>> -    StrOrNull **ptr = object_field_prop_ptr(obj, prop);
>>> -    StrOrNull *str_or_null = g_new0(StrOrNull, 1);
>>> +    StrOrNull **ptr = object_field_prop_ptr(obj, opaque);
>>> +    StrOrNull *str_or_null;
>>> +    char *str;
>>> +
>>> +    if (!visit_type_str(v, name, &str, errp)) {
>>> +        return;
>>> +    }
>>>  
>>>      /*
>>>       * Only str to keep compatibility, QNULL was never used via
>>>       * command line.
>>>       */
>>
>> This comment is from Fabiano's recent commit be346eb6673 (migration: Add
>> a qdev property for StrOrNull).  Fabiano, did you mean to write "Only
>> StrOrNull to keep compatibility"?
>>
>
> No, it's "only str" indeed. The point is that -global tls-creds NULL was
> never supported, so now that we have a property, this setter will never
> create a QNULL to avoid ever passing a QNULL to code that used to
> consume tls-creds and doesn't expect a QNULL value (the "compatibility"
> part).

The comment confuses me.  I interpreted its first part as "This is only
str to keep compatibility" and went "wait, it's StrOrNull only for
compatibility!"

Can we phrase this more clearly?

>>> +    str_or_null = g_new0(StrOrNull, 1);
>>>      str_or_null->type = QTYPE_QSTRING;
>>> -    if (!visit_type_str(v, name, &str_or_null->u.s, errp)) {
>>
>> We leak @str_or_null here.  You plug the leak by delaying the allocation
>> until after visit_type_str().  Good.
>>
>>> -        return;
>>> -    }
>>> +    str_or_null->u.s = str;
>>>  
>>>      qapi_free_StrOrNull(*ptr);
>>>      *ptr = str_or_null;
>>
>> The function fails only when visit_type_str() fails, and
>> visit_type_str() fails only when v->type_str() fails.
>>
>> Why are you certain it won't?
>
> By inspection, I only see the failure points at qobject_input_type_str:
>
>     QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
>     ...
>     if (!qobj) {
>         return false;
>     }
>
>     qstr = qobject_to(QString, qobj);
>     if (!qstr) {
>         error_setg(errp, "Invalid parameter type for '%s', expected: string",
>                    full_name(qiv, name));
>         return false;
>     }
>
> I'm relying on qdev's boilerplate not allowing incorrect invocations
> from the user. If code has been written that allows the .set function to
> be reached with a wrong type or a incorrect member name inside of a
> struct, for instance, then I'm calling it a programming error.

Alright, you're constructing a non-local argument to show the memory
leak is actually unreachable.

Please work this more verbose version of the argument into the commit
message.