Re: [Qemu-devel] [PATCH for-4.1 v2] nbd: Initialize reply on failure

[Andrey Shinkevich]

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 <address@hidden>
> Reported-by: Andrey Shinkevich <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>   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 <address@hidden>
-- 
With the best regards,
Andrey Shinkevich