Re: [Qemu-devel] [PATCH v2] vmdk: Fix integer overflow in offset calculation

[Max Reitz <mreitz@redhat.com>]

On 19.09.2014 13:52, Max Reitz wrote:
> On 15.09.2014 04:32, Fam Zheng wrote:
>> This fixes the bug introduced by commit c6ac36e (vmdk: Optimize cluster
>> allocation).
>>
>> $ ~/build/master/qemu-io /stor/vm/arch.vmdk -c 'write 2G 1k'
>> write failed: Invalid argument
>>
>> Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>   block/vmdk.c               |  2 +-
>>   tests/qemu-iotests/005     | 10 +++++++++-
>>   tests/qemu-iotests/005.out | 10 +++++++++-
>>   3 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index a1cb911..3fd7738 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -1113,7 +1113,7 @@ static int get_cluster_offset(BlockDriverState 
>> *bs,
>>       uint32_t min_count, *l2_table;
>>       bool zeroed = false;
>>       int64_t ret;
>> -    int32_t cluster_sector;
>> +    int64_t cluster_sector;
>>         if (m_data) {
>>           m_data->valid = 0;
>> diff --git a/tests/qemu-iotests/005 b/tests/qemu-iotests/005
>> index ba1236d..fc8944c 100755
>> --- a/tests/qemu-iotests/005
>> +++ b/tests/qemu-iotests/005
>> @@ -59,7 +59,7 @@ fi
>>     echo
>>   echo "creating large image"
>> -_make_test_img 5000G
>> +_make_test_img 16T
>>     echo
>>   echo "small read"
>> @@ -69,6 +69,14 @@ echo
>>   echo "small write"
>>   $QEMU_IO -c "write 8192 4096" "$TEST_IMG" | _filter_qemu_io
>>   +echo
>> +echo "small read at high offset"
>> +$QEMU_IO -c "read 4T 4096" "$TEST_IMG" | _filter_qemu_io
>> +
>> +echo
>> +echo "small write at high offset"
>> +$QEMU_IO -c "write 4T 4096" "$TEST_IMG" | _filter_qemu_io
>> +
>>   # success, all done
>>   echo "*** done"
>>   rm -f $seq.full
>> diff --git a/tests/qemu-iotests/005.out b/tests/qemu-iotests/005.out
>> index 2d3e7df..fd6aed9 100644
>> --- a/tests/qemu-iotests/005.out
>> +++ b/tests/qemu-iotests/005.out
>> @@ -1,7 +1,7 @@
>>   QA output created by 005
>>     creating large image
>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=5368709120000
>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=17592186044416
>>     small read
>>   read 4096/4096 bytes at offset 1024
>> @@ -10,4 +10,12 @@ read 4096/4096 bytes at offset 1024
>>   small write
>>   wrote 4096/4096 bytes at offset 8192
>>   4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +
>> +small read at high offset
>> +read 4096/4096 bytes at offset 4398046511104
>> +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +
>> +small write at high offset
>> +wrote 4096/4096 bytes at offset 4398046511104
>> +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>   *** done
>
> Okay, this test works for VMDK. However, now this test no longer works 
> with raw, at least not on my system (ftruncate() fails). So we could 
> either exempt raw from this test like vpc (which is probably fine 
> since I don't see the point in trying to create such huge raw images; 
> if it works for other image formats, that should be fine) or we (you) 
> cannot reuse this test.

Oh, I forgot to add: I only tested qcow2, vmdk and raw; so there might 
be other image formats which no longer work with this test. I'm 
completely fine with excluding all of them from this, because failure to 
pass it would then be format-specific and no longer a general problem of 
the block layer (which this generic test is probably for).

Max

> In case you opt for the former (exempt raw like vpc):
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>