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

[Peter Xu <peterx@redhat.com>]

On Mon, Jan 26, 2026 at 04:16:11PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> 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 <peter.maydell@linaro.org>
> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> ---
> >> 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.

I agree, assert would even be better.  Then, maybe let's add a quick
comment for it?  Something like:

    /*
     * Assert for both str_or_null be present and not QNULL. 
     * Guaranteed because ...
     */
    assert(str_or_null && str_or_null->type != QTYPE_QNULL);

-- 
Peter Xu