From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
To: Eric Blake <eblake@redhat.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: Kevin Wolf <kwolf@redhat.com>, Thomas Huth <thuth@redhat.com>,
"philmd@redhat.com" <philmd@redhat.com>,
"open list:Network Block Dev..." <qemu-block@nongnu.org>,
Max Reitz <mreitz@redhat.com>
Subject: Re: [Qemu-devel] [PATCH for-4.1 v2] nbd: Initialize reply on failure
Date: Mon, 22 Jul 2019 10:19:45 +0000 [thread overview]
Message-ID: <cbbdbcaf-63a7-58d2-af0d-9499c880043a@virtuozzo.com> (raw)
In-Reply-To: <20190719172001.19770-1-eblake@redhat.com>
On 19/07/2019 20:20, Eric Blake wrote:
> We've had two separate reports of different callers running into use
> of uninitialized data if s->quit is set (one detected by gcc -O3,
> another by valgrind), due to checking 'nbd_reply_is_simple(reply) ||
> s->quit' in the wrong order. Rather than chasing down which callers
> need to pre-initialize reply, and whether there are any other
> uninitialized uses, it's easier to guarantee that reply will always be
> set by nbd_co_receive_one_chunk() even on failure.
>
> The uninitialized use happens to be harmless (the only time the
> variable is uninitialized is if s->quit is set, so the conditional
> results in the same action regardless of what was read from reply),
> and was introduced in commit 65e01d47.
>
> In fixing the problem, it can also be seen that all (one) callers pass
> in a non-NULL reply, so there is a dead condtional to also be cleaned
> up.
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Reported-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> block/nbd.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index 81edabbf35ed..57c1a205811a 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -640,12 +640,11 @@ static coroutine_fn int nbd_co_receive_one_chunk(
> request_ret, qiov, payload, errp);
>
> if (ret < 0) {
> + memset(reply, 0, sizeof(*reply));
> s->quit = true;
> } else {
> /* For assert at loop start in nbd_connection_entry */
> - if (reply) {
> - *reply = s->reply;
> - }
> + *reply = s->reply;
> s->reply.handle = 0;
> }
>
Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
--
With the best regards,
Andrey Shinkevich
prev parent reply other threads:[~2019-07-22 10:20 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-19 17:20 [Qemu-devel] [PATCH for-4.1 v2] nbd: Initialize reply on failure Eric Blake
2019-07-19 17:23 ` Philippe Mathieu-Daudé
2019-07-22 10:19 ` Andrey Shinkevich [this message]
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=cbbdbcaf-63a7-58d2-af0d-9499c880043a@virtuozzo.com \
--to=andrey.shinkevich@virtuozzo.com \
--cc=eblake@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=philmd@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=thuth@redhat.com \
/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).