From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Fabiano Rosas <farosas@suse.de>
Cc: qemu-devel@nongnu.org, Peter Xu <peterx@redhat.com>,
Peter Maydell <peter.maydell@linaro.org>
Subject: Re: [PATCH 2/2] migration: Fix error handling after dup in file migration
Date: Tue, 12 Mar 2024 09:57:58 +0000 [thread overview]
Message-ID: <ZfAnJjka5KHR1Mnu@redhat.com> (raw)
In-Reply-To: <20240311233335.17299-3-farosas@suse.de>
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 :|
next prev parent reply other threads:[~2024-03-12 9:58 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-11 23:33 [PATCH 0/2] migration: mapped-ram fixes Fabiano Rosas
2024-03-11 23:33 ` [PATCH 1/2] io: Introduce qio_channel_file_new_dupfd Fabiano Rosas
2024-03-12 9:52 ` Daniel P. Berrangé
2024-03-11 23:33 ` [PATCH 2/2] migration: Fix error handling after dup in file migration Fabiano Rosas
2024-03-12 9:57 ` Daniel P. Berrangé [this message]
2024-03-12 12:22 ` Peter Xu
2024-03-12 12:44 ` Daniel P. Berrangé
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZfAnJjka5KHR1Mnu@redhat.com \
--to=berrange@redhat.com \
--cc=farosas@suse.de \
--cc=peter.maydell@linaro.org \
--cc=peterx@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).