On 1/15/19 10:58 AM, Eric Blake wrote:
> On 1/15/19 10:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>>> @size is not size of the image, but size of the export, so it may be less than dev_offset
>>>> (qemu-nbd.c do "fd_size -= dev_offset" before "nbd_export_new(bs, dev_offset, fd_size, "
>>>
>>> But the assert is fine because patch 3/19 fixed qemu-nbd.c to never pass
>>> in dev_offset larger than size (it fails up front if dev_offset is out
>>> of bounds, whether from the -o command line option or from what it read
>>> from the partition header with the -P command line option).
>>>
>>
>> Don't follow =(
>>
>> Assume, image size 3M, and we have offset 2M, i.e. -o 2M.
>>
>> than in qemu-nbd.c, we have
>>
>> fd_size = blk_getlength(blk); # 3M
>> ...
>> fd_size -= dev_offset; # 1M
>> ...
>> export = nbd_export_new(bs, dev_offset, fd_size # bs, 2M, 1M
>>
>> in nbd_export_new:
>>
>> assert(dev_offset <= size); # 2M <= 1M
>>
>> fail.
> 
> Ouch, you are right. I don't need the assertion in server.c at all;
> because all callers pass in a validated size, but the validated size has
> no comparable relation to dev_offset.

Here's what I'm considering using instead, merely asserting that the
inputs are non-negative and do not overflow 63 bits:

diff --git i/nbd/server.c w/nbd/server.c
index c9937ccdc2a..3ebcbddaa2c 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -1495,11 +1495,12 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
off_t dev_offset, off_t size,
     exp->refcount = 1;
     QTAILQ_INIT(&exp->clients);
     exp->blk = blk;
+    assert(dev_offset >= 0 && dev_offset <= INT64_MAX);
     exp->dev_offset = dev_offset;
     exp->name = g_strdup(name);
     exp->description = g_strdup(description);
     exp->nbdflags = nbdflags;
-    assert(dev_offset <= size);
+    assert(size >= 0 && size <= INT64_MAX - dev_offset);
     exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);

     if (bitmap) {


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