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

[Fam Zheng]

On Sat, 09/13 20:01, Max Reitz wrote:
> On 12.09.2014 11:51, 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 <address@hidden>
> >Signed-off-by: Fam Zheng <address@hidden>
> >---
> >  block/vmdk.c               | 2 +-
> >  tests/qemu-iotests/059     | 6 ++++++
> >  tests/qemu-iotests/059.out | 7 +++++++
> >  3 files changed, 14 insertions(+), 1 deletion(-)
> >
> >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;
> 
> The fix is okay.
> 
> >diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
> >index 3c053c2..6ed2a25 100755
> >--- a/tests/qemu-iotests/059
> >+++ b/tests/qemu-iotests/059
> >@@ -126,6 +126,12 @@ _img_info
> >  $QEMU_IO -c "write -P 0xa 900G 512" "$TEST_IMG" | _filter_qemu_io
> >  $QEMU_IO -c "read -v 900G 1024" "$TEST_IMG" | _filter_qemu_io
> >+echo
> >+echo "=== Testing reading and writing at big offset ==="
> >+_make_test_img 4T
> >+$QEMU_IO -c "write -P 0xa 1T 1024" "$TEST_IMG" | _filter_qemu_io
> >+$QEMU_IO -c "read -P 0xa 1T 1024" "$TEST_IMG" | _filter_qemu_io
> >+
> >  # success, all done
> >  echo "*** done"
> >  rm -f $seq.full
> >diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
> >index 0dadba6..51c72a6 100644
> >--- a/tests/qemu-iotests/059.out
> >+++ b/tests/qemu-iotests/059.out
> >@@ -2332,4 +2332,11 @@ e1000003e0:  00 00 00 00 00 00 00 00 00 00 00 00 00 
> >00 00 00  ................
> >  e1000003f0:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
> > ................
> >  read 1024/1024 bytes at offset 966367641600
> >  1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >+
> >+=== Testing reading and writing at big offset ===
> >+Formatting 'TEST_DIR/iotest-version3.IMGFMT', fmt=IMGFMT size=4398046511104
> >+wrote 1024/1024 bytes at offset 1099511627776
> >+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >+read 1024/1024 bytes at offset 1099511627776
> >+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >  *** done
> 
> This test is not. It works even without the fix for me. The problem is that
> this does not depend on the guest disk size or the guest offset, but on the
> host offset. So the problem only appears if the image has been filled enough
> so that the host offsets grow large enough (apparently, at least).
> 
> However, the first host offset written to does depend on the image size. For
> 2 GB images, it's 0x90000; for 4 TB images, it's 0x20110000; and for 16 TB
> images, it finally overflows, *independently* on the guest offset written
> to. So I suggest you create a 16 TB image here (or maybe an image which is
> as large as possible, for testing purposes) and then you can write anywhere
> (maybe once at the very beginning and once at the very end, if that works).

Ah yes, thanks for pointing out! I will respin and do what you suggest in 005.

Fam