On Tue, Mar 12, 2024 at 09:57:58AM +0000, Daniel P. Berrangé wrote: > On Mon, Mar 11, 2024 at 08:33:35PM -0300, Fabiano Rosas wrote: > > The file migration code was allowing a possible -1 from a failed call > > to dup() to propagate into the new QIOFileChannel::fd before checking > > for validity. Coverity doesn't like that, possibly due to the the > > lseek(-1, ...) call that would ensue before returning from the channel > > creation routine. > > > > Use the newly introduced qio_channel_file_dupfd() to properly check > > the return of dup() before proceeding. > > > > Fixes: CID 1539961 > > Fixes: CID 1539965 > > Fixes: CID 1539960 > > Fixes: 2dd7ee7a51 ("migration/multifd: Add incoming QIOChannelFile support") > > Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI") > > Reported-by: Peter Maydell <peter.mayd...@linaro.org> > > Signed-off-by: Fabiano Rosas <faro...@suse.de> > > --- > > migration/fd.c | 9 ++++----- > > migration/file.c | 14 +++++++------- > > 2 files changed, 11 insertions(+), 12 deletions(-) > > > > diff --git a/migration/fd.c b/migration/fd.c > > index d4ae72d132..4e2a63a73d 100644 > > --- a/migration/fd.c > > +++ b/migration/fd.c > > @@ -80,6 +80,7 @@ static gboolean fd_accept_incoming_migration(QIOChannel > > *ioc, > > void fd_start_incoming_migration(const char *fdname, Error **errp) > > { > > QIOChannel *ioc; > > + QIOChannelFile *fioc; > > int fd = monitor_fd_param(monitor_cur(), fdname, errp); > > if (fd == -1) { > > return; > > @@ -103,15 +104,13 @@ void fd_start_incoming_migration(const char *fdname, > > Error **errp) > > int channels = migrate_multifd_channels(); > > > > while (channels--) { > > - ioc = QIO_CHANNEL(qio_channel_file_new_fd(dup(fd))); > > - > > - if (QIO_CHANNEL_FILE(ioc)->fd == -1) { > > - error_setg(errp, "Failed to duplicate fd %d", fd); > > + fioc = qio_channel_file_new_dupfd(fd, errp); > > + if (!fioc) { > > return; > > } > > > > qio_channel_set_name(ioc, "migration-fd-incoming"); > > - qio_channel_add_watch_full(ioc, G_IO_IN, > > + qio_channel_add_watch_full(QIO_CHANNEL(fioc), G_IO_IN, > > fd_accept_incoming_migration, > > NULL, NULL, > > > > g_main_context_get_thread_default()); > > Nothing is free'ing the already created channels, if this while() > loop fails on the 2nd or later iterations. > > > diff --git a/migration/file.c b/migration/file.c > > index 164b079966..d458f48269 100644 > > --- a/migration/file.c > > +++ b/migration/file.c > > @@ -58,12 +58,13 @@ bool file_send_channel_create(gpointer opaque, Error > > **errp) > > int fd = fd_args_get_fd(); > > > > if (fd && fd != -1) { > > - ioc = qio_channel_file_new_fd(dup(fd)); > > + ioc = qio_channel_file_new_dupfd(fd, errp); > > } else { > > ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, > > errp); > > - if (!ioc) { > > - goto out; > > - } > > + } > > + > > + if (!ioc) { > > + goto out; > > } > > > > multifd_channel_connect(opaque, QIO_CHANNEL(ioc)); > > @@ -147,10 +148,9 @@ void file_start_incoming_migration(FileMigrationArgs > > *file_args, Error **errp) > > NULL, NULL, > > g_main_context_get_thread_default()); > > > > - fioc = qio_channel_file_new_fd(dup(fioc->fd)); > > + fioc = qio_channel_file_new_dupfd(fioc->fd, errp); > > > > - if (!fioc || fioc->fd == -1) { > > - error_setg(errp, "Error creating migration incoming channel"); > > + if (!fioc) { > > break; > > } > > } while (++i < channels); > > Again, nothing is free'ing when the loops fails on 2nd or later > iterations.
For this one, I think it constantly leak one IOC even if no failure triggers.. > > So a weak > > Reviewed-by: Daniel P. Berrangé <berra...@redhat.com> > > on the basis that it fixes the bugs that it claims to fix, but there > are more bugs that still need fixing here. For the other issue, Fabiano - I think there's one easy way to workaround and avoid bothering with "how to remove a registered IO watch" is we create the IOCs in a loop first, register the IO watches only if all succeeded. I'll queue the series first to fix the reported issue. Thanks, -- Peter Xu