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

[Markus Armbruster <armbru@redhat.com>]

Peter Xu <peterx@redhat.com> writes:

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

g_free(), g_realloc(), freeaddrinfo(), qapi_free_T(), visit_free(),
qcrypto_FOO_free(), aio_task_pool_free(), qemu_opts_free(),
timer_free(), ...

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

The error.h functions and macros that free an Error object are:

* error_free(), error_free_or_abort()

  These take a single Error * argument.

* error_report_err(), warn_report_err(), error_reportf_err(),
  warn_reportf_err(). warn_report_err_once_cond(),
  warn_report_err_once()

  These take also a single Error * argument.

* error_propagate(), error_propagate_prepend()

  These take an Error ** destination, and an Error * object to be
  propagated.  They take ownership of the latter.  They either store it
  in the destination, or free it.

  Changing the latter to Error ** makes these functions vulnerable to
  swapped arguments.  See "Error ** parameters are almost always for
  returning errors" below.

* ERRP_GUARD()

  Includes automatic error_propagate() on return, immune to use after
  free.

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

My main argument remains this one: our deallocating functions don't work
that way.  For better or worse.

I don't want the Error API free differently.