Fix a couple of possible leaks detected by Coverity. Both are currently harmless. This code is only used for the very specific purpose of maintaining compatibility of a few migration options which can be set via QEMU command line (-global migration.tls-*). The command line interface is not supported and only used during development and testing.
1) The setter function set_StrOrNull() is invoked whenever the -global migration.tls-* command line options are set. The way it could leak is that the temporary "StrOrNull *str_or_null" object is allocated before calling the visitor, which could fail and cause an early return of the function, leaving *ptr unset and str_or_null leaking. 2) The getter function get_StrOrNull() is unreachable code. It's only there to provide a complete implementation of the property. Still, the way it could leak is that the temporary "StrOrNull *str_or_null" might be allocated and is simply never returned to the caller nor freed. Fix the possible leaks: 1) at set_StrOrNull(): change the allocation of str_or_null to happen only after the visit call has returned successfully. 2) at get_StrOrNull(): assert that the object is non-NULL, there is no need for a temporary object. The reason it should be non-NULL is that the property is initialized by the default setter of the qdev property. The initialization is unlikely to fail because the call to the setter is setup by qdev, which has boilerplate ensuring the to-be-set object is allocated and of the correct type. Moreover, passing NULL via command line to -global migration.tls-* is not possible. A programming error could result in an invalid call to the setter, which would leave the object NULL and cause a crash in the getter, but that's not a worthwhile scenario to protect against given the low probability of this code being even reached. While here, update the comment about why there's no QNULL in this StrOrNull property to be more clear. Fixes: CID 1643919 Fixes: CID 1643920 Cc: Markus Armbruster <[email protected]> Reported-by: Peter Maydell <[email protected]> Reviewed-by: Peter Xu <[email protected]> Reviewed-by: Prasad Pandit <[email protected]> Link: https://lore.kernel.org/qemu-devel/[email protected] Signed-off-by: Fabiano Rosas <[email protected]> --- migration/options.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/migration/options.c b/migration/options.c index f33b297929..658c578191 100644 --- a/migration/options.c +++ b/migration/options.c @@ -220,14 +220,12 @@ static void get_StrOrNull(Object *obj, Visitor *v, const char *name, StrOrNull **ptr = object_field_prop_ptr(obj, prop); 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 by qdev. 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); } @@ -236,16 +234,25 @@ static void set_StrOrNull(Object *obj, Visitor *v, const char *name, { const Property *prop = opaque; StrOrNull **ptr = object_field_prop_ptr(obj, prop); - StrOrNull *str_or_null = g_new0(StrOrNull, 1); + 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 property only applies to the command line usage of + * migration's TLS options (-global migration.tls-*) where the + * NULL value cannot be provided as input (only strings are + * allowed). Therefore, this StrOrNull implementation never + * produces a QNULL value to avoid ever returning values outside + * the range of what was previously handled by consumers of the + * TLS options. */ + 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
