Le Friday 24 Jan 2014 à 18:01:20 (+0800), Hu Tao a écrit : > On Thu, Jan 23, 2014 at 06:13:48PM +0100, Benoît Canet wrote: > > Le Thursday 23 Jan 2014 à 11:04:06 (+0800), Hu Tao a écrit : > > > When cluster size is big enough it can lead offset overflow > > > > Maybe "it can lead to an offset overflow in" > > > in qcow2_alloc_clusters_at(). This patch fixes it. > > Sure. > > > > > > > The allocation each time is stopped at L2 table boundary > > "The allocation is stopped each time at" > > Sure. > > > > > > (see handle_alloc()), so the possible maximum bytes could be > > > > > > 2^(cluster_bits - 3 + cluster_bits) > > > > So if understand cluster_bits - 3 is used to compute the number of entry by > > L2 > > and the additional cluster_bits is to take into account each clusters > > referenced > > by the L2 entries. > > Exactly. This is clearer than just one calculation. Do you mind I > put the sentence in commit message? yes It would be fine for future reference.
> > > It makes sense. > > > > > > > > so int is safe for cluster_bits<=17, unsafe otherwise. > > > > > > Reviewed-by: Max Reitz <mre...@redhat.com> > > > Signed-off-by: Hu Tao <hu...@cn.fujitsu.com> > > > --- > > > block/qcow2-refcount.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > > > index c974abe..8712d8b 100644 > > > --- a/block/qcow2-refcount.c > > > +++ b/block/qcow2-refcount.c > > > @@ -676,7 +676,13 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, > > > uint64_t offset, > > > BDRVQcowState *s = bs->opaque; > > > uint64_t cluster_index; > > > uint64_t old_free_cluster_index; > > > - int i, refcount, ret; > > > + uint64_t i; > > > + int refcount, ret; > > > + > > > > > > > + assert(nb_clusters >= 0); > > > + if (nb_clusters == 0) { > > > + return 0; > > > + } > > ^ > > Adding a a line on the commit message about this assertion and chunk of code > > would be helpful. > > How about > > Add an assert to guard the comparation between signed and unsigned yes > > ? > > > > > Best regards > > > > Benoît > > > > > > > > /* Check how many clusters there are free */ > > > cluster_index = offset >> s->cluster_bits; > > > -- > > > 1.8.5.2.229.g4448466 > > > > > > >