Re: [Qemu-devel] [PATCH v4 7/7] block/nbd: NBDReply is used being uninitialized

[Andrey Shinkevich]

On 19/07/2019 17:44, Eric Blake wrote:
> On 7/19/19 9:34 AM, Eric Blake wrote:
>> On 7/19/19 4:40 AM, Andrey Shinkevich wrote:
>>> In case nbd_co_receive_one_chunk() fails in
>>> nbd_reply_chunk_iter_receive(), 'NBDReply reply' parameter is used in
>>> the check nbd_reply_is_simple() without being initialized. The iotest
>>> 083 does not pass under the Valgrind: $./check -nbd -valgrind 083.
>>> The alternative solution is to swap the operands in the condition:
>>> 'if (s->quit || nbd_reply_is_simple(reply))'
>>>
>>> Signed-off-by: Andrey Shinkevich <address@hidden>
>>> ---
>>>   block/nbd.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Huh. Very similar to
>> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03712.html, but
>> affects a different function. I can queue this one through my NBD tree
>> to get both in my rc2 pull request.
>>
>> Reviewed-by: Eric Blake <address@hidden>
> 
> Actually, since this is the second patch on the same topic, I'm
> wondering if it's better to use the following one-liner to fix BOTH
> issues and without relying on a gcc extension:
> 
> diff --git i/block/nbd.c w/block/nbd.c
> index 8d565cc624ec..f751a8e633e5 100644
> --- i/block/nbd.c
> +++ w/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);

The call to memset() consumes more CPU time. I don't know how frequent 
the "ret < 0" path is. The initialization ' = {0}' is cheaper.
Is it safe to swap the operands in the condition in 
nbd_reply_chunk_iter_receive():
'if (s->quit || nbd_reply_is_simple(reply))' ?

Andrey

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

-- 
With the best regards,
Andrey Shinkevich