Re: [PATCH v3] qcow2: Avoid integer wraparound in qcow2_co_truncate()

[Eric Blake <eblake@redhat.com>]

On 5/4/20 10:52 AM, Alberto Garcia wrote:
> After commit f01643fb8b47e8a70c04bbf45e0f12a9e5bc54de when an image is
> extended and BDRV_REQ_ZERO_WRITE is set then the new clusters are
> zeroized.
> 
> The code however does not detect correctly situations when the old and
> the new end of the image are within the same cluster. The problem can
> be reproduced with these steps:
> 
>     qemu-img create -f qcow2 backing.qcow2 1M
>     qemu-img create -f qcow2 -F qcow2 -b backing.qcow2 top.qcow2
>     qemu-img resize --shrink top.qcow2 520k
>     qemu-img resize top.qcow2 567k
> 
> In the last step offset - zero_start causes an integer wraparound.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---

> +++ b/block/qcow2.c
> @@ -4239,15 +4239,17 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>            * requires a cluster-aligned start. The end may be unaligned if it is
>            * at the end of the image (which it is here).
>            */
> -        ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0);
> -        if (ret < 0) {
> -            error_setg_errno(errp, -ret, "Failed to zero out new clusters");
> -            goto fail;
> +        if (offset > zero_start) {
> +            ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0);
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret, "Failed to zero out new clusters");
> +                goto fail;
> +            }
>           }
>   
>           /* Write explicit zeros for the unaligned head */
>           if (zero_start > old_length) {
> -            uint64_t len = zero_start - old_length;
> +            uint64_t len = MIN(zero_start, offset) - old_length;

Yes, that's better.

> +++ b/tests/qemu-iotests/292
> @@ -0,0 +1,73 @@

> +_supported_fmt qcow2
> +_supported_proto file

Do we have to limit it to qcow2 and file?  Yes, it's testing a bugfix 
for qcow2, but are there other formats that it doesn't hurt to have the 
extra testing?  Also, I don't see anything preventing this from working 
with non-file protocol.

But whether we widen the test scope or not, we at least test that we 
don't regress.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org