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

Reply via email to