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 <mre...@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
signature.asc
Description: OpenPGP digital signature
