| Message ID | 20240311233335.17299-3-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: mapped-ram fixes | expand |
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
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,
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
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);
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(-)