Peter Xu <[email protected]> writes: > On Tue, Nov 25, 2025 at 01:59:54PM +0100, Markus Armbruster wrote: >> Marc-André Lureau <[email protected]> writes: >> >> > Hi >> > >> > On Tue, Nov 25, 2025 at 11:06 AM Markus Armbruster <[email protected]> >> > wrote: >> >> >> >> Fixes: ffaa1b50a879 (migration: Use warn_reportf_err() where appropriate) >> >> Resolves: Coverity CID 1643463 >> >> Signed-off-by: Markus Armbruster <[email protected]> >> > >> > Reviewed-by: Marc-André Lureau <[email protected]> >> > >> >> --- >> >> 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.
