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 <th...@redhat.com>
> Reported-by: Andrey Shinkevich <andrey.shinkev...@virtuozzo.com>
> Signed-off-by: Eric Blake <ebl...@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.shinkev...@virtuozzo.com>
--
With the best regards,
Andrey Shinkevich
