Re: [PATCH] migration: Fix double-free on error path

[Peter Xu <peterx@redhat.com>]

On Tue, Nov 25, 2025 at 01:59:54PM +0100, Markus Armbruster wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> 
> > Hi
> >
> > On Tue, Nov 25, 2025 at 11:06 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Fixes: ffaa1b50a879 (migration: Use warn_reportf_err() where appropriate)
> >> Resolves: Coverity CID 1643463
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> >> ---
> >>  migration/multifd.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/migration/multifd.c b/migration/multifd.c
> >> index 6210454838..3203dc98e1 100644
> >> --- a/migration/multifd.c
> >> +++ b/migration/multifd.c
> >> @@ -450,7 +450,7 @@ static void multifd_send_set_error(Error *err)
> >>   */
> >>  static void migration_ioc_shutdown_gracefully(QIOChannel *ioc)
> >>  {
> >> -    g_autoptr(Error) local_err = NULL;
> >> +    Error *local_err = NULL;
> >>
> >>      if (!migration_has_failed(migrate_get_current()) &&
> >>          object_dynamic_cast((Object *)ioc, TYPE_QIO_CHANNEL_TLS)) {
> >> --
> >> 2.49.0
> >>
> >
> > Maybe warn_reportf_err() should take a Error **err instead, and clear
> > it (and accept NULL values)
> 
> Our deallocating functions don't work that way.
> 
> Having them take a pointer by reference and clear it gets rid of *one*
> dangling reference.  There may be more.

True.  However I need to confess I like Marc-André's proposal.. Normally we
only have one Error object, or >1 objects.

The only thing I'm not sure is such design doesn't match with the error API
(e.g. current form matches the more famous error_report_err(), and likely
others that I'm not familiar).  So at least this will need some more
thoughts before all the code churns.

Thanks,

> 
> Coverity is fairly good at finding the kind of use after free this could
> avoid.
> 
> Error ** parameters are almost always for returning errors.  Not having
> to wonder what such a parameter is for makes code easier to read.

-- 
Peter Xu