Peter Xu <[email protected]> writes:
> On Thu, Jan 22, 2026 at 08:24:56PM -0300, Fabiano Rosas wrote:
>> Fix a couple of leaks detected by Coverity. Both are currently
>> harmless because the visitor in the setter can never fail and the
>> whole of the getter is unused, it's only purpose at the moment is to
>> provide a complete implementation of the StrOrNull property.
>>
>> Fixes: CID 1643919
>> Fixes: CID 1643920
>> Reported-by: Peter Maydell <[email protected]>
>> Signed-off-by: Fabiano Rosas <[email protected]>
>> ---
>> CI run: https://gitlab.com/farosas/qemu/-/pipelines/2280325023
>> ---
>> migration/options.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/migration/options.c b/migration/options.c
>> index 9a5a39c886..9dc44a3736 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -225,6 +225,7 @@ static void get_StrOrNull(Object *obj, Visitor *v, const
>> char *name,
>> str_or_null = g_new0(StrOrNull, 1);
>> str_or_null->type = QTYPE_QSTRING;
>> str_or_null->u.s = g_strdup("");
>> + *ptr = str_or_null;
>
> Yep, fixes the leak.
>
> However does it then mean a get() can internally update this Property? It
> used to be NULL, now it's QTYPE_STRING "".
>
> It may make slightly more sense to me to have zero side effect on a get()
> call. Say, keeping *ptr==NULL after get() returns (as before), while we can
> use a temp string ("") for the visitor.
>
> I randomly checked another get() provider, e.g. get_drive() does that.
> Should we follow?
>
Hmm, I'm actually inclined to assert. This is code for the very specific
purpose of allowing compatibility of -global tls-* options which are
properties of the s->parameters object.
The only way the property could be NULL in the getter is if the setter
has failed. Is there a way we could make the setter fail without it
being a programming error? I don't see it.
- the default for the property is an empty string.
- setting via set-parameters bypasses the qdev code entirely.
- device_add is not reachable because of dc->user_creatable = false.
- object_set_propv and qom_set use a string visitor, so no type mismatch
to trigger the qobject_to(QString, qobj) cast.
>> } else {
>> /* the setter doesn't allow QNULL */
>> assert(str_or_null->type != QTYPE_QNULL);
>> @@ -245,6 +246,7 @@ static void set_StrOrNull(Object *obj, Visitor *v, const
>> char *name,
>> */
>> str_or_null->type = QTYPE_QSTRING;
>> if (!visit_type_str(v, name, &str_or_null->u.s, errp)) {
>> + qapi_free_StrOrNull(str_or_null);
>
> This looks correct. Or, simpler way is we only do g_new0() after the
> visit_type_str() successfully returned (and pass a temp char* into it).
>
I'll update it.
>> return;
>> }
>>
>> --
>> 2.51.0
>>
