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

[Max Reitz <mreitz@redhat.com>]

[-- 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 --]