[for-4.1,v2] nbd: Initialize reply on failure
diff mbox series

Message ID 20190719172001.19770-1-eblake@redhat.com
State New
Headers show
Series
  • [for-4.1,v2] nbd: Initialize reply on failure
Related show

Commit Message

Eric Blake July 19, 2019, 5:20 p.m. UTC
We've had two separate reports of different callers 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, and whether there are any other
uninitialized uses, it's easier to guarantee that reply will always be
set by nbd_co_receive_one_chunk() even on failure.

The uninitialized use happens to be harmless (the only time the
variable is uninitialized is if s->quit is set, so the conditional
results in the same action regardless of what was read from reply),
and was introduced in commit 65e01d47.

In fixing the problem, it can also be seen that all (one) callers pass
in a non-NULL reply, so there is a dead condtional to also be cleaned
up.

Reported-by: Thomas Huth <thuth@redhat.com>
Reported-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/nbd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Philippe Mathieu-Daudé July 19, 2019, 5:23 p.m. UTC | #1
On 7/19/19 7:20 PM, Eric Blake wrote:
> We've had two separate reports of different callers 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, and whether there are any other
> uninitialized uses, it's easier to guarantee that reply will always be
> set by nbd_co_receive_one_chunk() even on failure.
> 
> The uninitialized use happens to be harmless (the only time the
> variable is uninitialized is if s->quit is set, so the conditional
> results in the same action regardless of what was read from reply),
> and was introduced in commit 65e01d47.
> 
> In fixing the problem, it can also be seen that all (one) callers pass
> in a non-NULL reply, so there is a dead condtional to also be cleaned

"conditional"

> up.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Reported-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/nbd.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 81edabbf35ed..57c1a205811a 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -640,12 +640,11 @@ 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;
> -        }
> +        *reply = s->reply;
>          s->reply.handle = 0;
>      }
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Andrey Shinkevich July 22, 2019, 10:19 a.m. UTC | #2
On 19/07/2019 20:20, Eric Blake wrote:
> We've had two separate reports of different callers 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, and whether there are any other
> uninitialized uses, it's easier to guarantee that reply will always be
> set by nbd_co_receive_one_chunk() even on failure.
> 
> The uninitialized use happens to be harmless (the only time the
> variable is uninitialized is if s->quit is set, so the conditional
> results in the same action regardless of what was read from reply),
> and was introduced in commit 65e01d47.
> 
> In fixing the problem, it can also be seen that all (one) callers pass
> in a non-NULL reply, so there is a dead condtional to also be cleaned
> up.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Reported-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   block/nbd.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 81edabbf35ed..57c1a205811a 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -640,12 +640,11 @@ 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;
> -        }
> +        *reply = s->reply;
>           s->reply.handle = 0;
>       }
> 

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

Patch
diff mbox series

diff --git a/block/nbd.c b/block/nbd.c
index 81edabbf35ed..57c1a205811a 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -640,12 +640,11 @@  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;
-        }
+        *reply = s->reply;
         s->reply.handle = 0;
     }