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.3On 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 :|
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
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 :|
© 2016 - 2024 Red Hat, Inc.