Re: [PATCH 2/2] migration: Fix error handling after dup in file migration

[Daniel P . Berrangé]

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.maydell@linaro.org>
> Signed-off-by: Fabiano Rosas <farosas@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.

So a weak

  Reviewed-by: Daniel P. Berrangé <berrange@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.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|