[PATCH 0/2] migration: mapped-ram fixes

[PATCH 0/2] migration: mapped-ram fixes

[Fabiano Rosas]

Hi,

Here are the fixes for the dup() issues found by Coverity.

@Peter Xu, I fixed the leak you found, but I'm holding on to that patch
because I noticed we're not freeing the ioc in the error paths. I'll
need to add some infrastructure to be able to cancel the glib polling
(qio_channel_add_watch_full) when the channel creation fails before
the source has connnected.

CI run: https://gitlab.com/farosas/qemu/-/pipelines/1209529440

Fabiano Rosas (2):
  io: Introduce qio_channel_file_new_dupfd
  migration: Fix error handling after dup in file migration

 include/io/channel-file.h | 18 ++++++++++++++++++
 io/channel-file.c         | 12 ++++++++++++
 migration/fd.c            |  9 ++++-----
 migration/file.c          | 14 +++++++-------
 4 files changed, 41 insertions(+), 12 deletions(-)

-- 
2.35.3

[PATCH 1/2] io: Introduce qio_channel_file_new_dupfd

[Fabiano Rosas]

Add a new helper function for creating a QIOChannelFile channel with a
duplicated file descriptor. This saves the calling code from having to
do error checking on the dup() call.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 include/io/channel-file.h | 18 ++++++++++++++++++
 io/channel-file.c         | 12 ++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/include/io/channel-file.h b/include/io/channel-file.h
index 50e8eb1138..d373a4e44d 100644
--- a/include/io/channel-file.h
+++ b/include/io/channel-file.h
@@ -68,6 +68,24 @@ struct QIOChannelFile {
 QIOChannelFile *
 qio_channel_file_new_fd(int fd);
 
+/**
+ * qio_channel_file_new_dupfd:
+ * @fd: the file descriptor
+ * @errp: pointer to initialized error object
+ *
+ * Create a new IO channel object for a file represented by the @fd
+ * parameter. Like qio_channel_file_new_fd(), but the @fd is first
+ * duplicated with dup().
+ *
+ * The channel will own the duplicated file descriptor and will take
+ * responsibility for closing it, the original FD is owned by the
+ * caller.
+ *
+ * Returns: the new channel object
+ */
+QIOChannelFile *
+qio_channel_file_new_dupfd(int fd, Error **errp);
+
 /**
  * qio_channel_file_new_path:
  * @path: the file path
diff --git a/io/channel-file.c b/io/channel-file.c
index d4706fa592..cbdd03bf21 100644
--- a/io/channel-file.c
+++ b/io/channel-file.c
@@ -45,6 +45,18 @@ qio_channel_file_new_fd(int fd)
     return ioc;
 }
 
+QIOChannelFile *
+qio_channel_file_new_dupfd(int fd, Error **errp)
+{
+    int newfd = dup(fd);
+
+    if (newfd < 0) {
+        error_setg_errno(errp, errno, "Could not dup FD %d", fd);
+        return NULL;
+    }
+
+    return qio_channel_file_new_fd(newfd);
+}
 
 QIOChannelFile *
 qio_channel_file_new_path(const char *path,
-- 
2.35.3

Re: [PATCH 1/2] io: Introduce qio_channel_file_new_dupfd

[Daniel P. Berrangé]

On Mon, Mar 11, 2024 at 08:33:34PM -0300, Fabiano Rosas wrote:
> Add a new helper function for creating a QIOChannelFile channel with a
> duplicated file descriptor. This saves the calling code from having to
> do error checking on the dup() call.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  include/io/channel-file.h | 18 ++++++++++++++++++
>  io/channel-file.c         | 12 ++++++++++++
>  2 files changed, 30 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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

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

[Fabiano Rosas]

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());
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);
-- 
2.35.3

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

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

[Peter Xu]

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

For this one, I think it constantly leak one IOC even if no failure
triggers..

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

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

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

[Daniel P. Berrangé]

On Tue, Mar 12, 2024 at 08:22:18AM -0400, Peter Xu wrote:
> 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.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.
> 
> For this one, I think it constantly leak one IOC even if no failure
> triggers..
> 
> > 
> > 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.
> 
> 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.

Yes, that makes sense as an approach.

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