Re: [Qemu-devel] [PATCH v2 0/8] nbd block status base:allocation

[Eric Blake <eblake@redhat.com>]

On 03/12/2018 10:11 PM, Eric Blake wrote:

>>
> 
>>    CC      block/nbd-client.o
>>    CC      block/sheepdog.o
>> /var/tmp/patchew-tester-tmp-erqpie2w/src/block/nbd-client.c: In 
>> function ‘nbd_client_co_block_status’:
>> /var/tmp/patchew-tester-tmp-erqpie2w/src/block/nbd-client.c:890:15: 
>> error: ‘extent.flags’ may be used uninitialized in this function 
>> [-Werror=maybe-uninitialized]
>>       NBDExtent extent;
>>                 ^~~~~~
>> /var/tmp/patchew-tester-tmp-erqpie2w/src/block/nbd-client.c:925:19: 
>> error: ‘extent.length’ may be used uninitialized in this function 
>> [-Werror=maybe-uninitialized]
>>       *pnum = extent.length;
>>               ~~~~~~^~~~~~~
> 
> May be a false positive where the compiler merely can't see through the 
> logic, or it might be a real bug; but I suspect either way that the 
> solution is to initialize this (I'm guessing patch 5), as in:
> 
> NBDExtent extent = { 0 };

Not a sufficient fix - it's probably a real bug in that extent is never 
initialized if the NBD_FOREACH_REPLY_CHUNK() loop in 
nbd_co_receive_blockstatus_reply() never encounters an 
NBD_REPLY_TYPE_BLOCK_STATUS chunk.  A malicious server could just reply 
with NBD_REPLY_TYPE_NONE (no status reported after all), but the block 
layer contract requires us to make progress or return an error.  So, if 
the server didn't give us any extents, we need to turn it into an error 
even if the server did not.

> 
> Will squash that in when I get to that part of the review.
> 

And I forgot to do it before the pull request v1, oops.  Here's what I'm 
squashing in for v2:

diff --git i/block/nbd-client.c w/block/nbd-client.c
index be160052cb1..486d73f1c63 100644
--- i/block/nbd-client.c
+++ w/block/nbd-client.c
@@ -694,6 +694,7 @@ static int 
nbd_co_receive_blockstatus_reply(NBDClientSession *s,
      Error *local_err = NULL;
      bool received = false;

+    extent->length = 0;
      NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply,
                              NULL, &reply, &payload)
      {
@@ -734,6 +735,13 @@ static int 
nbd_co_receive_blockstatus_reply(NBDClientSession *s,
          payload = NULL;
      }

+    if (!extent->length && !iter.err) {
+        error_setg(&iter.err,
+                   "Server did not reply with any status extents");
+        if (!iter.ret) {
+            iter.ret = -EIO;
+        }
+    }
      error_propagate(errp, iter.err);
      return iter.ret;
  }
@@ -919,6 +927,7 @@ int coroutine_fn 
nbd_client_co_block_status(BlockDriverState *bs,
          return ret;
      }

+    assert(extent.length);
      *pnum = extent.length;
      return (extent.flags & NBD_STATE_HOLE ? 0 : BDRV_BLOCK_DATA) |
             (extent.flags & NBD_STATE_ZERO ? BDRV_BLOCK_ZERO : 0);



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