[Qemu-devel] [PATCH 0/2] qcow2: Make size_to_clusters() return int64_t

[Qemu-devel] [PATCH 0/2] qcow2: Make size_to_clusters() return int64_t

[Max Reitz]

Some callers actually expected that function to return int64_t. As it
turns out, it doesn't. Fix that.


Max Reitz (2):
  qcow2: Make size_to_clusters() return int64_t
  iotests: Add test for checking large image files

 block/qcow2-cluster.c      | 20 +++++++------
 block/qcow2.h              |  2 +-
 tests/qemu-iotests/138     | 73 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/138.out |  9 ++++++
 tests/qemu-iotests/group   |  1 +
 5 files changed, 95 insertions(+), 10 deletions(-)
 create mode 100755 tests/qemu-iotests/138
 create mode 100644 tests/qemu-iotests/138.out

-- 
2.5.1

[Qemu-devel] [PATCH 1/2] qcow2: Make size_to_clusters() return int64_t

[Max Reitz]

Sadly, some images may have more clusters than what can be represented
using a plain int. We should be prepared for that case (in
qcow2_check_refcounts() we actually were trying to catch that case, but
since size_to_clusters() truncated the returned value, that check never
did anything useful).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 20 +++++++++++---------
 block/qcow2.h         |  2 +-
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 2975b83..a34f0b1 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -473,8 +473,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
     unsigned int l2_index;
     uint64_t l1_index, l2_offset, *l2_table;
     int l1_bits, c;
-    unsigned int index_in_cluster, nb_clusters;
-    uint64_t nb_available, nb_needed;
+    unsigned int index_in_cluster;
+    uint64_t nb_available, nb_needed, nb_clusters;
     int ret;
 
     index_in_cluster = (offset >> 9) & (s->cluster_sectors - 1);
@@ -837,10 +837,10 @@ err:
  * write, but require COW to be performed (this includes yet unallocated space,
  * which must copy from the backing file)
  */
-static int count_cow_clusters(BDRVQcow2State *s, int nb_clusters,
+static int count_cow_clusters(BDRVQcow2State *s, uint64_t nb_clusters,
     uint64_t *l2_table, int l2_index)
 {
-    int i;
+    uint64_t i;
 
     for (i = 0; i < nb_clusters; i++) {
         uint64_t l2_entry = be64_to_cpu(l2_table[l2_index + i]);
@@ -960,7 +960,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
     int l2_index;
     uint64_t cluster_offset;
     uint64_t *l2_table;
-    unsigned int nb_clusters;
+    uint64_t nb_clusters;
     unsigned int keep_clusters;
     int ret;
 
@@ -1426,7 +1426,7 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
  * clusters.
  */
 static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
-    unsigned int nb_clusters, enum qcow2_discard_type type, bool full_discard)
+    uint64_t nb_clusters, enum qcow2_discard_type type, bool full_discard)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t *l2_table;
@@ -1441,6 +1441,7 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
 
     /* Limit nb_clusters to one L2 table */
     nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
+    assert(nb_clusters <= INT_MAX);
 
     for (i = 0; i < nb_clusters; i++) {
         uint64_t old_l2_entry;
@@ -1503,7 +1504,7 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t end_offset;
-    unsigned int nb_clusters;
+    uint64_t nb_clusters;
     int ret;
 
     end_offset = offset + (nb_sectors << BDRV_SECTOR_BITS);
@@ -1545,7 +1546,7 @@ fail:
  * clusters.
  */
 static int zero_single_l2(BlockDriverState *bs, uint64_t offset,
-    unsigned int nb_clusters)
+    uint64_t nb_clusters)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t *l2_table;
@@ -1560,6 +1561,7 @@ static int zero_single_l2(BlockDriverState *bs, uint64_t offset,
 
     /* Limit nb_clusters to one L2 table */
     nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
+    assert(nb_clusters <= INT_MAX);
 
     for (i = 0; i < nb_clusters; i++) {
         uint64_t old_offset;
@@ -1584,7 +1586,7 @@ static int zero_single_l2(BlockDriverState *bs, uint64_t offset,
 int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors)
 {
     BDRVQcow2State *s = bs->opaque;
-    unsigned int nb_clusters;
+    uint64_t nb_clusters;
     int ret;
 
     /* The zero flag is only supported by version 3 and newer */
diff --git a/block/qcow2.h b/block/qcow2.h
index 61f1b57..ce292a0 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -415,7 +415,7 @@ static inline int64_t offset_into_cluster(BDRVQcow2State *s, int64_t offset)
     return offset & (s->cluster_size - 1);
 }
 
-static inline int size_to_clusters(BDRVQcow2State *s, int64_t size)
+static inline int64_t size_to_clusters(BDRVQcow2State *s, int64_t size)
 {
     return (size + (s->cluster_size - 1)) >> s->cluster_bits;
 }
-- 
2.5.1

[Qemu-devel] [PATCH 2/2] iotests: Add test for checking large image files

[Max Reitz]

Add a test for checking a qcow2 file with a multiple of 2^32 clusters.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/138     | 73 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/138.out |  9 ++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 83 insertions(+)
 create mode 100755 tests/qemu-iotests/138
 create mode 100644 tests/qemu-iotests/138.out

diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
new file mode 100755
index 0000000..0507ccc
--- /dev/null
+++ b/tests/qemu-iotests/138
@@ -0,0 +1,73 @@
+#!/bin/bash
+#
+# General test case for qcow2's image check
+#
+# Copyright (C) 2015 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=mreitz@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 qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+echo
+echo '=== Check on an image with a multiple of 2^32 clusters ==='
+echo
+
+IMGOPTS=$(_optstr_add "$IMGOPTS" "cluster_size=512") \
+    _make_test_img 512
+
+# Allocate L2 table
+$QEMU_IO -c 'write 0 512' "$TEST_IMG" | _filter_qemu_io
+
+# Put the data cluster at a multiple of 2 TB, resulting in the image apparently
+# having a multiple of 2^32 clusters
+# (To be more specific: It is at 32 PB)
+poke_file "$TEST_IMG" 2048 "\x80\x80\x00\x00\x00\x00\x00\x00"
+
+# An offset of 32 PB results in qemu-img check having to allocate an in-memory
+# refcount table of 128 TB (16 bit refcounts, 512 byte clusters).
+# This should be generally too much for any system and thus fail.
+# What this test is checking is that the qcow2 driver actually tries to allocate
+# such a large amount of memory (and is consequently aborting) instead of having
+# truncated the cluster count somewhere (which would result in much less memory
+# being allocated and then a segfault occuring).
+_check_test_img
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/138.out b/tests/qemu-iotests/138.out
new file mode 100644
index 0000000..3fe911f
--- /dev/null
+++ b/tests/qemu-iotests/138.out
@@ -0,0 +1,9 @@
+QA output created by 138
+
+=== Check on an image with a multiple of 2^32 clusters ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=512
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qemu-img: Check failed: Cannot allocate memory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 3a6a8f0..439b1d2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -135,3 +135,4 @@
 134 rw auto quick
 135 rw auto
 137 rw auto
+138 rw auto quick
-- 
2.5.1

Re: [Qemu-devel] [PATCH 1/2] qcow2: Make size_to_clusters() return int64_t

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

On 08.09.2015 22:09, Max Reitz wrote:
> Sadly, some images may have more clusters than what can be represented
> using a plain int. We should be prepared for that case (in
> qcow2_check_refcounts() we actually were trying to catch that case, but
> since size_to_clusters() truncated the returned value, that check never
> did anything useful).
> 

Cc: qemu-stable <qemu-stable@nongnu.org>

> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c | 20 +++++++++++---------
>  block/qcow2.h         |  2 +-
>  2 files changed, 12 insertions(+), 10 deletions(-)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 2/2] iotests: Add test for checking large image files

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]

On 09/08/2015 02:09 PM, Max Reitz wrote:
> Add a test for checking a qcow2 file with a multiple of 2^32 clusters.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/138     | 73 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/138.out |  9 ++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 83 insertions(+)
>  create mode 100755 tests/qemu-iotests/138
>  create mode 100644 tests/qemu-iotests/138.out
> 

> +# Put the data cluster at a multiple of 2 TB, resulting in the image apparently
> +# having a multiple of 2^32 clusters
> +# (To be more specific: It is at 32 PB)
> +poke_file "$TEST_IMG" 2048 "\x80\x80\x00\x00\x00\x00\x00\x00"
> +
> +# An offset of 32 PB results in qemu-img check having to allocate an in-memory
> +# refcount table of 128 TB (16 bit refcounts, 512 byte clusters).
> +# This should be generally too much for any system and thus fail.
> +# What this test is checking is that the qcow2 driver actually tries to allocate
> +# such a large amount of memory (and is consequently aborting) instead of having
> +# truncated the cluster count somewhere (which would result in much less memory
> +# being allocated and then a segfault occuring).

s/occuring/occurring/

With the typo fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/2] qcow2: Make size_to_clusters() return int64_t

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1783 bytes --]

On 09/08/2015 02:09 PM, Max Reitz wrote:
> Sadly, some images may have more clusters than what can be represented
> using a plain int. We should be prepared for that case (in
> qcow2_check_refcounts() we actually were trying to catch that case, but
> since size_to_clusters() truncated the returned value, that check never
> did anything useful).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c | 20 +++++++++++---------
>  block/qcow2.h         |  2 +-
>  2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 2975b83..a34f0b1 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -473,8 +473,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>      unsigned int l2_index;
>      uint64_t l1_index, l2_offset, *l2_table;
>      int l1_bits, c;
> -    unsigned int index_in_cluster, nb_clusters;
> -    uint64_t nb_available, nb_needed;
> +    unsigned int index_in_cluster;
> +    uint64_t nb_available, nb_needed, nb_clusters;

Most uses are storing the results unsigned...

>  
> -static inline int size_to_clusters(BDRVQcow2State *s, int64_t size)
> +static inline int64_t size_to_clusters(BDRVQcow2State *s, int64_t size)
>  {
>      return (size + (s->cluster_size - 1)) >> s->cluster_bits;
>  }

...and the function itself doesn't appear to intentionally return
negative (unless size was passed in as negative, but then that may be
accidental).  Should it just return uint64_t instead?

At any rate, I agree that 'int' is too small, so:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/2] qcow2: Make size_to_clusters() return int64_t

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2089 bytes --]

On 08.09.2015 22:22, Eric Blake wrote:
> On 09/08/2015 02:09 PM, Max Reitz wrote:
>> Sadly, some images may have more clusters than what can be represented
>> using a plain int. We should be prepared for that case (in
>> qcow2_check_refcounts() we actually were trying to catch that case, but
>> since size_to_clusters() truncated the returned value, that check never
>> did anything useful).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/qcow2-cluster.c | 20 +++++++++++---------
>>  block/qcow2.h         |  2 +-
>>  2 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 2975b83..a34f0b1 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -473,8 +473,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>>      unsigned int l2_index;
>>      uint64_t l1_index, l2_offset, *l2_table;
>>      int l1_bits, c;
>> -    unsigned int index_in_cluster, nb_clusters;
>> -    uint64_t nb_available, nb_needed;
>> +    unsigned int index_in_cluster;
>> +    uint64_t nb_available, nb_needed, nb_clusters;
> 
> Most uses are storing the results unsigned...
> 
>>  
>> -static inline int size_to_clusters(BDRVQcow2State *s, int64_t size)
>> +static inline int64_t size_to_clusters(BDRVQcow2State *s, int64_t size)
>>  {
>>      return (size + (s->cluster_size - 1)) >> s->cluster_bits;
>>  }
> 
> ...and the function itself doesn't appear to intentionally return
> negative (unless size was passed in as negative, but then that may be
> accidental).  Should it just return uint64_t instead?

It won't matter in practice because we generally don't support any
offsets bigger than INT64_MAX anyway; the @size parameter has been an
int64_t all along, too.

If I have to respin for some reason (i.e. maintainer not willing to fix
up the comment in patch 2), I'll probably change the type, though.

> At any rate, I agree that 'int' is too small, so:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 1/2] qcow2: Make size_to_clusters() return int64_t

[Kevin Wolf]

Am 08.09.2015 um 22:09 hat Max Reitz geschrieben:
> Sadly, some images may have more clusters than what can be represented
> using a plain int. We should be prepared for that case (in
> qcow2_check_refcounts() we actually were trying to catch that case, but
> since size_to_clusters() truncated the returned value, that check never
> did anything useful).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

You seem to fix a few of the callers as well, which is a good thing.

However, what about realloc_refcount_array()? It uses size_t, which can
be 32 bits, whereas the comment in refcount_array_byte_size() suggests
that we could get as much as 2^55.

> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 2975b83..a34f0b1 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -473,8 +473,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>      unsigned int l2_index;
>      uint64_t l1_index, l2_offset, *l2_table;
>      int l1_bits, c;
> -    unsigned int index_in_cluster, nb_clusters;
> -    uint64_t nb_available, nb_needed;
> +    unsigned int index_in_cluster;
> +    uint64_t nb_available, nb_needed, nb_clusters;
>      int ret;
>  
>      index_in_cluster = (offset >> 9) & (s->cluster_sectors - 1);

We're probably better off adding an assertion here. The type change is
useless because nb_clusters is only used as a parameter for calling
count_contiguous_(free_)clusters, which is a function that takes int64_t
and returns int (which totally makes sense). In the overflow case it
seems to have an endless loop.

Of course, all of that doesn't really matter because nb_needed never
exceeds a single L2 table.

> @@ -837,10 +837,10 @@ err:
>   * write, but require COW to be performed (this includes yet unallocated space,
>   * which must copy from the backing file)
>   */
> -static int count_cow_clusters(BDRVQcow2State *s, int nb_clusters,
> +static int count_cow_clusters(BDRVQcow2State *s, uint64_t nb_clusters,
>      uint64_t *l2_table, int l2_index)
>  {
> -    int i;
> +    uint64_t i;
>  
>      for (i = 0; i < nb_clusters; i++) {
>          uint64_t l2_entry = be64_to_cpu(l2_table[l2_index + i]);

The return value is still int, so this changes the behaviour from an
endless loop (same thing as mentioned above) to a truncated return
value. Questionable whether that is an improvement (I'd say no).

> @@ -960,7 +960,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>      int l2_index;
>      uint64_t cluster_offset;
>      uint64_t *l2_table;
> -    unsigned int nb_clusters;
> +    uint64_t nb_clusters;
>      unsigned int keep_clusters;
>      int ret;

It looks like size isn't limited to a single L2 table there yet, so this
is an important fix. However, handle_alloc() needs the same.

> @@ -1426,7 +1426,7 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
>   * clusters.
>   */
>  static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
> -    unsigned int nb_clusters, enum qcow2_discard_type type, bool full_discard)
> +    uint64_t nb_clusters, enum qcow2_discard_type type, bool full_discard)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      uint64_t *l2_table;
> @@ -1441,6 +1441,7 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
>  
>      /* Limit nb_clusters to one L2 table */
>      nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
> +    assert(nb_clusters <= INT_MAX);
>  
>      for (i = 0; i < nb_clusters; i++) {
>          uint64_t old_l2_entry;
> @@ -1503,7 +1504,7 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
>  {
>      BDRVQcow2State *s = bs->opaque;
>      uint64_t end_offset;
> -    unsigned int nb_clusters;
> +    uint64_t nb_clusters;
>      int ret;
>  
>      end_offset = offset + (nb_sectors << BDRV_SECTOR_BITS);

We can actually assert nb_clusters <= INT_MAX directly after assigning
it and before limiting it to a single L2 table. nb_sectors is already
int, so nb_clusters can never be larger.

I'm not objecting to uint64_t and an assertion, though, being explicit
is always nice.

> @@ -1545,7 +1546,7 @@ fail:
>   * clusters.
>   */
>  static int zero_single_l2(BlockDriverState *bs, uint64_t offset,
> -    unsigned int nb_clusters)
> +    uint64_t nb_clusters)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      uint64_t *l2_table;
> @@ -1560,6 +1561,7 @@ static int zero_single_l2(BlockDriverState *bs, uint64_t offset,
>  
>      /* Limit nb_clusters to one L2 table */
>      nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
> +    assert(nb_clusters <= INT_MAX);
>  
>      for (i = 0; i < nb_clusters; i++) {
>          uint64_t old_offset;
> @@ -1584,7 +1586,7 @@ static int zero_single_l2(BlockDriverState *bs, uint64_t offset,
>  int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors)
>  {
>      BDRVQcow2State *s = bs->opaque;
> -    unsigned int nb_clusters;
> +    uint64_t nb_clusters;
>      int ret;
>  
>      /* The zero flag is only supported by version 3 and newer */

Same thing really.

> diff --git a/block/qcow2.h b/block/qcow2.h
> index 61f1b57..ce292a0 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -415,7 +415,7 @@ static inline int64_t offset_into_cluster(BDRVQcow2State *s, int64_t offset)
>      return offset & (s->cluster_size - 1);
>  }
>  
> -static inline int size_to_clusters(BDRVQcow2State *s, int64_t size)
> +static inline int64_t size_to_clusters(BDRVQcow2State *s, int64_t size)
>  {
>      return (size + (s->cluster_size - 1)) >> s->cluster_bits;
>  }

Kevin

Re: [Qemu-devel] [PATCH 1/2] qcow2: Make size_to_clusters() return int64_t

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 7690 bytes --]

On 09.09.2015 10:45, Kevin Wolf wrote:
> Am 08.09.2015 um 22:09 hat Max Reitz geschrieben:
>> Sadly, some images may have more clusters than what can be represented
>> using a plain int. We should be prepared for that case (in
>> qcow2_check_refcounts() we actually were trying to catch that case, but
>> since size_to_clusters() truncated the returned value, that check never
>> did anything useful).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> You seem to fix a few of the callers as well, which is a good thing.
> 
> However, what about realloc_refcount_array()? It uses size_t, which can
> be 32 bits, whereas the comment in refcount_array_byte_size() suggests
> that we could get as much as 2^55.

You're right. It was probably just too late when I wrote this patch. I
looked at that code and assumed that in the past I was intelligent
enough to make sure somewhere that it won't overflow there. Obviously I
wasn't.

>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 2975b83..a34f0b1 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -473,8 +473,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>>      unsigned int l2_index;
>>      uint64_t l1_index, l2_offset, *l2_table;
>>      int l1_bits, c;
>> -    unsigned int index_in_cluster, nb_clusters;
>> -    uint64_t nb_available, nb_needed;
>> +    unsigned int index_in_cluster;
>> +    uint64_t nb_available, nb_needed, nb_clusters;
>>      int ret;
>>  
>>      index_in_cluster = (offset >> 9) & (s->cluster_sectors - 1);
> 
> We're probably better off adding an assertion here. The type change is
> useless because nb_clusters is only used as a parameter for calling
> count_contiguous_(free_)clusters, which is a function that takes int64_t
> and returns int (which totally makes sense). In the overflow case it
> seems to have an endless loop.
> 
> Of course, all of that doesn't really matter because nb_needed never
> exceeds a single L2 table.

Hm, yes. I just looked at count_contiguous_{free_,}clusters() and they
took int64_t as a parameter, so I assumed they were prepared to handle
it. Again, what a fool I was.

Yes, I'll add an assertion. Or maybe I don't, because I feel like the
best place to do so is actually in count_contiguous_{free_,}clusters().
And there isn't even a need for an assertion there, because we can just
limit nb_clusters to the number of L2 table entries in those functions.
So there it's a question of "We could actually easily work with large
@nb_clusters by limiting it to the obvious maximum, but you are not
supposed to call this function with such large values, so by having a
too large value you are violating the function contract".

I'll probably just add an assertion.

>> @@ -837,10 +837,10 @@ err:
>>   * write, but require COW to be performed (this includes yet unallocated space,
>>   * which must copy from the backing file)
>>   */
>> -static int count_cow_clusters(BDRVQcow2State *s, int nb_clusters,
>> +static int count_cow_clusters(BDRVQcow2State *s, uint64_t nb_clusters,
>>      uint64_t *l2_table, int l2_index)
>>  {
>> -    int i;
>> +    uint64_t i;
>>  
>>      for (i = 0; i < nb_clusters; i++) {
>>          uint64_t l2_entry = be64_to_cpu(l2_table[l2_index + i]);
> 
> The return value is still int, so this changes the behaviour from an
> endless loop (same thing as mentioned above) to a truncated return
> value. Questionable whether that is an improvement (I'd say no).

OK. Argh. OK then. I'll keep this function taking an int, and make
count_contiguous_{free_,}clusters() take an int, too, and handle the
assert()s in the functions calling those.

>> @@ -960,7 +960,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>>      int l2_index;
>>      uint64_t cluster_offset;
>>      uint64_t *l2_table;
>> -    unsigned int nb_clusters;
>> +    uint64_t nb_clusters;
>>      unsigned int keep_clusters;
>>      int ret;
> 
> It looks like size isn't limited to a single L2 table there yet, so this
> is an important fix. However, handle_alloc() needs the same.

Oops, I simply missed that size_to_clusters() call.

>> @@ -1426,7 +1426,7 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
>>   * clusters.
>>   */
>>  static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
>> -    unsigned int nb_clusters, enum qcow2_discard_type type, bool full_discard)
>> +    uint64_t nb_clusters, enum qcow2_discard_type type, bool full_discard)
>>  {
>>      BDRVQcow2State *s = bs->opaque;
>>      uint64_t *l2_table;
>> @@ -1441,6 +1441,7 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
>>  
>>      /* Limit nb_clusters to one L2 table */
>>      nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
>> +    assert(nb_clusters <= INT_MAX);
>>  
>>      for (i = 0; i < nb_clusters; i++) {
>>          uint64_t old_l2_entry;
>> @@ -1503,7 +1504,7 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
>>  {
>>      BDRVQcow2State *s = bs->opaque;
>>      uint64_t end_offset;
>> -    unsigned int nb_clusters;
>> +    uint64_t nb_clusters;
>>      int ret;
>>  
>>      end_offset = offset + (nb_sectors << BDRV_SECTOR_BITS);
> 
> We can actually assert nb_clusters <= INT_MAX directly after assigning
> it and before limiting it to a single L2 table. nb_sectors is already
> int, so nb_clusters can never be larger.

Hm, I think I like asserting such a range limitation after the last
assignment, and the fact that that assignment is limiting is obvious,
too, since s->l2_size is an int. So I think I'll keep it as it is (and
do the same elsewhere).

> I'm not objecting to uint64_t and an assertion, though, being explicit
> is always nice.
> 
>> @@ -1545,7 +1546,7 @@ fail:
>>   * clusters.
>>   */
>>  static int zero_single_l2(BlockDriverState *bs, uint64_t offset,
>> -    unsigned int nb_clusters)
>> +    uint64_t nb_clusters)
>>  {
>>      BDRVQcow2State *s = bs->opaque;
>>      uint64_t *l2_table;
>> @@ -1560,6 +1561,7 @@ static int zero_single_l2(BlockDriverState *bs, uint64_t offset,
>>  
>>      /* Limit nb_clusters to one L2 table */
>>      nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
>> +    assert(nb_clusters <= INT_MAX);
>>  
>>      for (i = 0; i < nb_clusters; i++) {
>>          uint64_t old_offset;
>> @@ -1584,7 +1586,7 @@ static int zero_single_l2(BlockDriverState *bs, uint64_t offset,
>>  int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors)
>>  {
>>      BDRVQcow2State *s = bs->opaque;
>> -    unsigned int nb_clusters;
>> +    uint64_t nb_clusters;
>>      int ret;
>>  
>>      /* The zero flag is only supported by version 3 and newer */
> 
> Same thing really.

Not really. The value returned by this function is not related to
nb_clusters (it's 0 in case of success), and zero_single_l2() takes a
uint64_t and makes good use of it. So this should actually be fine.

>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 61f1b57..ce292a0 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -415,7 +415,7 @@ static inline int64_t offset_into_cluster(BDRVQcow2State *s, int64_t offset)
>>      return offset & (s->cluster_size - 1);
>>  }
>>  
>> -static inline int size_to_clusters(BDRVQcow2State *s, int64_t size)
>> +static inline int64_t size_to_clusters(BDRVQcow2State *s, int64_t size)
>>  {
>>      return (size + (s->cluster_size - 1)) >> s->cluster_bits;
>>  }
> 
> Kevin
> 

Thanks for reviewing!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]