On Wed, 05/24 15:28, Eric Blake wrote:
> We document that *file is valid if the return is not an error and
> includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract
> when a driver (such as blkdebug) lacks a callback. Broken in
> commit 67a0fd2 (v2.6), when we added the file parameter.
>
> Enhance qemu-iotest 177 to cover this, using a sequence that would
> print garbage or even SEGV, because it was dererefencing through
> uninitialized memory. [The resulting test output shows that we
> have less-than-ideal block status from the blkdebug driver, but
> that's a separate fix coming up soon.]
>
> Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is
> enough to fix the crash, but we can go one step further: always
> setting *file, even on error, means that a broken caller that
> blindly dereferences file without checking for error is now more
> likely to get a reliable SEGV instead of randomly acting on garbage,
> making it easier to diagnose such buggy callers. Adding an
> assertion that file is set where expected doesn't hurt either.
>
> CC: qemu-sta...@nongnu.org
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> v2: drop redundant assignment
> ---
> block/io.c | 5 +++--
> tests/qemu-iotests/177 | 3 +++
> tests/qemu-iotests/177.out | 2 ++
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index fdd7485..8e6c3fe 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1749,6 +1749,7 @@ static int64_t coroutine_fn
> bdrv_co_get_block_status(BlockDriverState *bs,
> int64_t n;
> int64_t ret, ret2;
>
> + *file = NULL;
> total_sectors = bdrv_nb_sectors(bs);
> if (total_sectors < 0) {
> return total_sectors;
> @@ -1769,11 +1770,11 @@ static int64_t coroutine_fn
> bdrv_co_get_block_status(BlockDriverState *bs,
> ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
> if (bs->drv->protocol_name) {
> ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
> + *file = bs;
> }
> return ret;
> }
>
> - *file = NULL;
> bdrv_inc_in_flight(bs);
> ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
> file);
> @@ -1783,7 +1784,7 @@ static int64_t coroutine_fn
> bdrv_co_get_block_status(BlockDriverState *bs,
> }
>
> if (ret & BDRV_BLOCK_RAW) {
> - assert(ret & BDRV_BLOCK_OFFSET_VALID);
> + assert(ret & BDRV_BLOCK_OFFSET_VALID && *file);
> ret = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
> *pnum, pnum, file);
> goto out;
> diff --git a/tests/qemu-iotests/177 b/tests/qemu-iotests/177
> index 2005c17..f8ed8fb 100755
> --- a/tests/qemu-iotests/177
> +++ b/tests/qemu-iotests/177
> @@ -43,6 +43,7 @@ _supported_proto file
> CLUSTER_SIZE=1M
> size=128M
> options=driver=blkdebug,image.driver=qcow2
> +nested_opts=image.file.driver=file,image.file.filename=$TEST_IMG
>
> echo
> echo "== setting up files =="
> @@ -106,6 +107,8 @@ function verify_io()
> }
>
> verify_io | $QEMU_IO -r "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IMG map --image-opts "$options,$nested_opts,align=4k" \
> + | _filter_qemu_img_map
>
> _check_test_img
>
> diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out
> index e887542..b754ed4 100644
> --- a/tests/qemu-iotests/177.out
> +++ b/tests/qemu-iotests/177.out
> @@ -45,5 +45,7 @@ read 30408704/30408704 bytes at offset 80740352
> 29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> read 23068672/23068672 bytes at offset 111149056
> 22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +Offset Length File
> +0 0x8000000 blkdebug::TEST_DIR/t.IMGFMT
> No errors were found on the image.
> *** done
> --
> 2.9.4
>
Reviewed-by: Fam Zheng <f...@redhat.com>
