Re: [Qemu-devel] [PATCH for-2.0 39/47] block: vdi bounds check qemu-io tests

[Jeff Cody <jcody@redhat.com>]

On Sat, Mar 29, 2014 at 12:22:58AM +0100, Max Reitz wrote:
> On 26.03.2014 13:06, Stefan Hajnoczi wrote:
> >From: Jeff Cody <jcody@redhat.com>
> >
> >This test checks for proper bounds checking of some VDI input
> >headers.  The following is checked:
> >
> >1. Max image size (1024TB) with the appropriate Blocks In Image
> >    value (0x3fffffff) is detected as valid.
> >
> >2. Image size exceeding max (1024TB) is seen as invalid
> >
> >3. Valid image size but with Blocks In Image value that is too
> >    small fails
> >
> >4. Blocks In Image size exceeding max (0x3fffffff) is seen as invalid
> >
> >5. 64MB image, with 64 Blocks In Image, and 1MB Block Size is seen
> >    as valid
> >
> >6. Block Size < 1MB not supported
> >
> >7. Block Size > 1MB not supported
> >
> >Signed-off-by: Jeff Cody <jcody@redhat.com>
> >Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >---
> >  tests/qemu-iotests/084     | 104 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/084.out |  33 ++++++++++++++
> >  tests/qemu-iotests/group   |   1 +
> >  3 files changed, 138 insertions(+)
> >  create mode 100755 tests/qemu-iotests/084
> >  create mode 100644 tests/qemu-iotests/084.out
> >
> >diff --git a/tests/qemu-iotests/084 b/tests/qemu-iotests/084
> >new file mode 100755
> >index 0000000..10a5a65
> >--- /dev/null
> >+++ b/tests/qemu-iotests/084
> >@@ -0,0 +1,104 @@
> >+#!/bin/bash
> >+#
> >+# Test case for VDI header corruption; image too large, and too many blocks
> >+#
> >+# Copyright (C) 2013 Red Hat, Inc.
> >+#
> >+# This program is free software; you can redistribute it and/or modify
> >+# it under the terms of the GNU General Public License as published by
> >+# the Free Software Foundation; either version 2 of the License, or
> >+# (at your option) any later version.
> >+#
> >+# This program is distributed in the hope that it will be useful,
> >+# but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >+# GNU General Public License for more details.
> >+#
> >+# You should have received a copy of the GNU General Public License
> >+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >+#
> >+
> >+# creator
> >+owner=jcody@redhat.com
> >+
> >+seq=`basename $0`
> >+echo "QA output created by $seq"
> >+
> >+here=`pwd`
> >+tmp=/tmp/$$
> >+status=1	# failure is the default!
> >+
> >+_cleanup()
> >+{
> >+	_cleanup_test_img
> >+}
> >+trap "_cleanup; exit \$status" 0 1 2 3 15
> >+
> >+# get standard environment, filters and checks
> >+. ./common.rc
> >+. ./common.filter
> >+
> >+# This tests vdi-specific header fields
> >+_supported_fmt vdi
> >+_supported_proto generic
> >+_supported_os Linux
> >+
> >+ds_offset=368  # disk image size field offset
> >+bs_offset=376  # block size field offset
> >+bii_offset=384 # block in image field offset
> >+
> >+echo
> >+echo "=== Testing image size bounds ==="
> >+echo
> >+_make_test_img 64M
> >+
> >+# check for image size too large
> >+# poke max image size, and appropriate blocks_in_image value
> >+echo "Test 1: Maximum size (1024 TB):"
> >+poke_file "$TEST_IMG" "$ds_offset" "\x00\x00\xf0\xff\xff\xff\x03\x00"
> >+poke_file "$TEST_IMG" "$bii_offset" "\xff\xff\xff\x3f"
> >+_img_info
> >+
> >+echo
> >+echo "Test 2: Size too large (1024TB + 1)"
> >+# This should be too large (-EINVAL):
> >+poke_file "$TEST_IMG" "$ds_offset" "\x00\x00\xf1\xff\xff\xff\x03\x00"
> >+_img_info
> >+
> >+echo
> >+echo "Test 3: Size valid (64M), but Blocks In Image too small (63)"
> >+# This sets the size to 64M, but with a blocks_in_image size that is
> >+# too small
> >+poke_file "$TEST_IMG" "$ds_offset" "\x00\x00\x00\x04\x00\x00\x00\x00"
> >+# For a 64M image, we would need a blocks_in_image value of at least 64,
> >+# so 63 should be too small and give us -ENOTSUP
> >+poke_file "$TEST_IMG" "$bii_offset" "\x3f\x00\x00\x00"
> >+_img_info
> >+
> >+echo
> >+echo "Test 4: Size valid (64M), but Blocks In Image exceeds max allowed"
> >+# Now check the bounds of blocks_in_image - 0x3fffffff should be the max
> >+# value here, and we should get -ENOTSUP
> >+poke_file "$TEST_IMG" "$bii_offset" "\x00\x00\x00\x40"
> >+_img_info
> >+
> >+# Finally, 1MB is the only block size supported.  Verify that
> >+# a value != 1MB results in error, both smaller and larger
> >+echo
> >+echo "Test 5: Valid Image: 64MB, Blocks In Image 64, Block Size 1MB"
> >+poke_file "$TEST_IMG" "$bii_offset" "\x40\x00\x00\x00" # reset bii to valid
> >+poke_file "$TEST_IMG" "$bs_offset" "\x00\x00\x10\x00"  # valid
> >+_img_info
> >+echo
> >+echo "Test 6: Block Size != 1MB; too small test (1MB - 1)"
> >+poke_file "$TEST_IMG" "$bs_offset" "\xff\xff\x0f\x00"  # invalid (too small)
> >+_img_info
> >+echo
> >+echo "Test 7: Block Size != 1MB; too large test (1MB + 1)"
> >+poke_file "$TEST_IMG" "$bs_offset" "\x00\x00\x11\x00"  # invalid (too large)
> 
> 0x110000 is not 1 MB + 1.
>

Indeed it is not.  I changed the test, but forgot to update the
comment.

I already submitted a v2; Stefan, do you want me to submit a v3 with a
comment change, or do you want to fix up the comment when applying the
patch (note you'd need to change the .out file as well)?

> >+_img_info
> >+# success, all done
> >+echo
> >+echo "*** done"
> >+rm -f $seq.full
> >+status=0
> >diff --git a/tests/qemu-iotests/084.out b/tests/qemu-iotests/084.out
> >new file mode 100644
> >index 0000000..1e320f5
> >--- /dev/null
> >+++ b/tests/qemu-iotests/084.out
> >@@ -0,0 +1,33 @@
> >+QA output created by 084
> >+
> >+=== Testing image size bounds ===
> >+
> >+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> >+Test 1: Maximum size (1024 TB):
> >+image: TEST_DIR/t.IMGFMT
> >+file format: IMGFMT
> >+virtual size: 1024T (1125899905794048 bytes)
> >+cluster_size: 1048576
> >+
> >+Test 2: Size too large (1024TB + 1)
> >+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Invalid argument
> 
> Hm, maybe E2BIG would be better here.
> 

In the v2 series, this was changed to ENOTSUP, since we consciously
don't support the larger size, even though the size may be valid.


> >+
> >+Test 3: Size valid (64M), but Blocks In Image too small (63)
> >+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': unsupported VDI image (disk size 67108864, image bitmap has room for 66060288)
> >+
> >+Test 4: Size valid (64M), but Blocks In Image exceeds max allowed
> >+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': unsupported VDI image (too many blocks)
> >+
> >+Test 5: Valid Image: 64MB, Blocks In Image 64, Block Size 1MB
> >+image: TEST_DIR/t.IMGFMT
> >+file format: IMGFMT
> >+virtual size: 64M (67108864 bytes)
> >+cluster_size: 1048576
> >+
> >+Test 6: Block Size != 1MB; too small test (1MB - 1)
> >+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': unsupported VDI image (sector size 1048575 is not 1048576)
> >+
> >+Test 7: Block Size != 1MB; too large test (1MB + 1)
> >+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': unsupported VDI image (sector size 1114112 is not 1048576)
> 
> As can be seen here, 0x110000 really does not equal 1 MB + 1.
> 

:)

> >+
> >+*** done
> >diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> >index ed44f35..c51640c 100644
> >--- a/tests/qemu-iotests/group
> >+++ b/tests/qemu-iotests/group
> >@@ -89,6 +89,7 @@
> >  081 rw auto
> >  082 rw auto quick
> >  083 rw auto
> >+084 img auto
> >  085 rw auto
> >  086 rw auto quick
> >  087 rw auto
> 
> Albeit the comment being wrong (which should be fixed) and EINVAL
> being a little bit confusing for too big images (there should be a
> follow-up patch for this):
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>