Fabiano Rosas <[email protected]> writes:
> Markus Armbruster <[email protected]> writes:
>
>> Fabiano Rosas <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Fabiano Rosas <[email protected]>
>>> ---
>>> 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.
