[v4,7/7] block/nbd: NBDReply is used being uninitialized
diff mbox series

Message ID 1563529204-3368-8-git-send-email-andrey.shinkevich@virtuozzo.com
State New
Headers show
Series
  • Allow Valgrind checking all QEMU processes
Related show

Commit Message

Andrey Shinkevich July 19, 2019, 9:40 a.m. UTC
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 <andrey.shinkevich@virtuozzo.com>
---
 block/nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eric Blake July 19, 2019, 2:34 p.m. UTC | #1
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 <andrey.shinkevich@virtuozzo.com>
> ---
>  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 <eblake@redhat.com>

> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 81edabb..8480ad4 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -786,7 +786,7 @@ static int nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t handle,
>                                          int *request_ret, Error **errp)
>  {
>      NBDReplyChunkIter iter;
> -    NBDReply reply;
> +    NBDReply reply = {};
>      void *payload = NULL;
>      Error *local_err = NULL;
>  
>
Andrey Shinkevich July 19, 2019, 2:43 p.m. UTC | #2
On 19/07/2019 17:34, 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 <andrey.shinkevich@virtuozzo.com>
>> ---
>>   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 <eblake@redhat.com>
> 

Many thanks,
Andrey

>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 81edabb..8480ad4 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -786,7 +786,7 @@ static int nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t handle,
>>                                           int *request_ret, Error **errp)
>>   {
>>       NBDReplyChunkIter iter;
>> -    NBDReply reply;
>> +    NBDReply reply = {};
>>       void *payload = NULL;
>>       Error *local_err = NULL;
>>   
>>
>
Eric Blake July 19, 2019, 2:44 p.m. UTC | #3
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 <andrey.shinkevich@virtuozzo.com>
>> ---
>>  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 <eblake@redhat.com>

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);
         s->quit = true;
     } else {
         /* For assert at loop start in nbd_connection_entry */
Andrey Shinkevich July 19, 2019, 3 p.m. UTC | #4
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 <andrey.shinkevich@virtuozzo.com>
>>> ---
>>>   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 <eblake@redhat.com>
> 
> 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 */
>
Eric Blake July 19, 2019, 3:15 p.m. UTC | #5
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 <andrey.shinkevich@virtuozzo.com>
>>>> ---
>>>>   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 <eblake@redhat.com>
>>
>> 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)?
Andrey Shinkevich July 19, 2019, 3:43 p.m. UTC | #6
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 <andrey.shinkevich@virtuozzo.com>
>>>>> ---
>>>>>    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 <eblake@redhat.com>
>>>
>>> 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

Patch
diff mbox series

diff --git a/block/nbd.c b/block/nbd.c
index 81edabb..8480ad4 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -786,7 +786,7 @@  static int nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t handle,
                                         int *request_ret, Error **errp)
 {
     NBDReplyChunkIter iter;
-    NBDReply reply;
+    NBDReply reply = {};
     void *payload = NULL;
     Error *local_err = NULL;