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

[Eric Blake]

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)?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature