From 5cf42b1c1f75499b467701926d3c9691d27712e1 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 19 Jul 2019 12:20:01 -0500 Subject: [PATCH] nbd: Initialize reply on failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 conditional to also be cleaned up. Reported-by: Thomas Huth Reported-by: Andrey Shinkevich Signed-off-by: Eric Blake Message-Id: <20190719172001.19770-1-eblake@redhat.com> Reviewed-by: Philippe Mathieu-Daudé --- block/nbd.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/block/nbd.c b/block/nbd.c index 81edabbf35..57c1a20581 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; }