Re: [Qemu-devel] [PATCH v4 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at()

[Benoît Canet]

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 <address@hidden>
> > > Signed-off-by: Hu Tao <address@hidden>
> > > ---
> > >  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
> > > 
> > > 
>