[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH for-5.1?] qcow2-cluster: Fix integer left shift error in qcow

From: Eric Blake
Subject: Re: [PATCH for-5.1?] qcow2-cluster: Fix integer left shift error in qcow2_alloc_cluster_link_l2()
Date: Wed, 5 Aug 2020 08:33:20 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0

On 8/5/20 4:22 AM, Tuguoyi wrote:
When calculating the offset, the result of left shift operation will be promoted
to type int64 automatically because the left operand of + operator is uint64_t.
but the result after integer promotion may be produce an error value for us and
trigger the following asserting error.

For example, consider i=0x2000, cluster_bits=18, the result of left shift
operation will be 0x80000000. Cause argument i is of signed integer type,
the result is automatically promoted to 0xffffffff80000000 which is not
we expected

The way to trigger the assertion error:
   qemu-img create -f qcow2 -o preallocation=full,cluster_size=256k tmpdisk 10G

I wonder if it is worth an iotest addition to cover this.

This patch fix it by casting @i to uint64_t before doing left shift operation

Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com>
  block/qcow2-cluster.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

While this appears to be long-standing, rather than a regression in 5.1, this would be worth getting into -rc3 today if we still have time (if not, slipping to 5.2 is okay).

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index a677ba9..550850b 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -980,7 +980,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
QCowL2Meta *m)
assert(l2_index + m->nb_clusters <= s->l2_slice_size);
      for (i = 0; i < m->nb_clusters; i++) {
-        uint64_t offset = cluster_offset + (i << s->cluster_bits);
+        uint64_t offset = cluster_offset + ((uint64_t)i << s->cluster_bits);

We have:
offset = uint64_t + (int << int)

which is indeed a cause of unwanted sign extension.

If I'm not mistaken, it would also be feasible to fix this by changing qcow2.h to fix typedef struct BDRVQcow2State to use an unsigned type for cluster_bits (the way we already do for struct QCowHeader), avoiding the need for a cast here.

But if we're trying to get this in today, rather than auditing all other uses of BDRVQcow2State for incorrect typing,

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

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]