| Message ID | 1441742995-11794-2-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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(-)
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>
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
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
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
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; }
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(-)