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

[Markus Armbruster <armbru@redhat.com>]

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?