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. >> 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.
