| Message ID | bfc390e142739e08943fad1014109869f4f9a7e8.1390445921.git.hutao@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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. > > The allocation each time is stopped at L2 table boundary "The allocation is stopped each time at" > (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. It makes sense. > > so int is safe for cluster_bits<=17, unsafe otherwise. > > Reviewed-by: Max Reitz <mreitz@redhat.com> > Signed-off-by: Hu Tao <hutao@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. Best regards Benoît > > /* Check how many clusters there are free */ > cluster_index = offset >> s->cluster_bits; > -- > 1.8.5.2.229.g4448466 > >
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? > It makes sense. > > > > > so int is safe for cluster_bits<=17, unsafe otherwise. > > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > Signed-off-by: Hu Tao <hutao@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 ? > > Best regards > > Benoît > > > > > /* Check how many clusters there are free */ > > cluster_index = offset >> s->cluster_bits; > > -- > > 1.8.5.2.229.g4448466 > > > >
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 <mreitz@redhat.com> > > > Signed-off-by: Hu Tao <hutao@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 > > > > > > >
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; + } /* Check how many clusters there are free */ cluster_index = offset >> s->cluster_bits;