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