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

[Philippe Mathieu-Daudé]

On 7/19/19 5:03 PM, Eric Blake wrote:
> We've had two separate reports of a caller 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, it's easier to guarantee that reply will always
> be set by nbd_co_receive_one_chunk() even on failure.
> 
> Reported-by: Thomas Huth <address@hidden>
> Reported-by: Andrey Shinkevich <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> ---
> 
> Replaces: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg04477.html
> Replaces: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03712.html
> 
>  block/nbd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 8d565cc624ec..f751a8e633e5 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -640,6 +640,7 @@ static coroutine_fn int nbd_co_receive_one_chunk(
>                                            request_ret, qiov, payload, errp);
> 
>      if (ret < 0) {
> +        memset(reply, 0, sizeof *reply);

I never had problem with sizeof without parenthesis, but here I find it
not easy to review.

Anyhow, this definitively looks like 4.1 material.

Preferently with sizeof(), but I don't mind, so:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

>          s->quit = true;
>      } else {
>          /* For assert at loop start in nbd_connection_entry */
>