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

[Fabiano Rosas <farosas@suse.de>]

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.