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

[Philippe Mathieu-Daudé]

On 7/19/19 7:15 PM, Eric Blake wrote:
> On 7/19/19 10:03 AM, 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>
>> ---
>>
> 
> Blech. Needs a v2.  Expanding context:
> 
> 
>> +++ 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);
>>          s->quit = true;
>>      } else {
>>          /* For assert at loop start in nbd_connection_entry */
>         if (reply) {
>             *reply = s->reply;
>         }
> 
> either callers can pass in reply==NULL (in which case the memset()
> dereferences NULL, oops), or always pass in non-NULL reply (in which

Oh good catch...

> case the null check is dead code).
>