Commit 81c219ac authored by Eric Blake's avatar Eric Blake Committed by Kevin Wolf
Browse files

block: Guarantee that *file is set on bdrv_get_block_status() 



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.  Messed up in
commit 67a0fd2a (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-stable@nongnu.org
Signed-off-by: default avatarEric Blake <eblake@redhat.com>
Reviewed-by: default avatarFam Zheng <famz@redhat.com>
Reviewed-by: default avatarMax Reitz <mreitz@redhat.com>
Reviewed-by: default avatarJohn Snow <jsnow@redhat.com>
Signed-off-by: default avatarKevin Wolf <kwolf@redhat.com>
parent 64ebf556

block/io.c

+3 −2
Original line number Diff line number Diff line
@@ -1734,6 +1734,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;
@@ -1757,11 +1758,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, 
        }

        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);
@@ -1771,7 +1772,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;

tests/qemu-iotests/177

+3 −0
Original line number Diff line number Diff line
@@ -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

tests/qemu-iotests/177.out

+2 −0
Original line number Diff line number Diff line
@@ -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       json:{"image": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}, "driver": "blkdebug", "align": "4k"}

No errors were found on the image.

*** done