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

[Andrey Shinkevich]

On 19/07/2019 18:15, Eric Blake wrote:
> On 7/19/19 10:00 AM, Andrey Shinkevich wrote:
>>
>>
>> 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.
> 
> Wrong.  The 'ret < 0' path only happens on error, while the '= {0}' path
> happens on ALL cases including success (if you'll look at the generated
> assembly code, gcc should emit the same assembly sequence for
> zero-initialization of the struct, whether that is a call to memset() or
> just inline assignments of zeros based on the small size of the struct,
> whether or not your code uses memset or '={}').  You don't need to
> optimize the error case, because on error, your NBD connection is dead,
> and you have worse problems.  Pre-initialization that is going to be
> overwritten on success is worse (although probably immeasurably, because
> it is likely in the noise) than exactly one initialization on any
> control flow path.
> 
>> Is it safe to swap the operands in the condition in
>> nbd_reply_chunk_iter_receive():
>> 'if (s->quit || nbd_reply_is_simple(reply))' ?
> 
> Yes, swapping the conditional appears to fix the only use of reply where
> it is used uninitialized, at least in the current state of the code (but
> it took me longer to audit that).  So if we're going for a one-line fix
> for both observations of the problem, changing the conditional instead
> of a memset on error is also acceptable for now (and maybe makes the
> error case slightly faster, but that's not a big deal, because the error
> case already means the NBD connection has bigger problems) - but who
> knows what future uses of reply might creep in to an unsuspecting patch
> writer that doesn't see the (in)action at a distance?  Which way is more
> maintainable, proving that the low-level code always initializes the
> variable (easy, since that can be checked at a single function), or
> proving that all high-level uses may pass in an uninitialized variable
> that is still left uninit on error but that they only use the variable
> on success (harder, since now you have to audit every single caller to
> see how they use reply on failure)?
> 

Sounds reasonable. Clear now. So, I will detach this patch from the 
series in the next v5 version.

Andrey
-- 
With the best regards,
Andrey Shinkevich