From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53513) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W6iau-0003Tn-Rz for qemu-devel@nongnu.org; Fri, 24 Jan 2014 10:23:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W6iap-00034i-T2 for qemu-devel@nongnu.org; Fri, 24 Jan 2014 10:22:56 -0500 Received: from paradis.irqsave.net ([62.212.105.220]:48250) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W6iap-00034b-IO for qemu-devel@nongnu.org; Fri, 24 Jan 2014 10:22:51 -0500 Date: Fri, 24 Jan 2014 16:22:49 +0100 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140124152249.GA3087@irqsave.net> References: <20140123171348.GD3519@irqsave.net> <20140124100120.GD23175@G08FNSTD100614.fnst.cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140124100120.GD23175@G08FNSTD100614.fnst.cn.fujitsu.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hu Tao Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , Kevin Wolf , qemu-devel@nongnu.org Le Friday 24 Jan 2014 =E0 18:01:20 (+0800), Hu Tao a =E9crit : > On Thu, Jan 23, 2014 at 06:13:48PM +0100, Beno=EEt Canet wrote: > > Le Thursday 23 Jan 2014 =E0 11:04:06 (+0800), Hu Tao a =E9crit : > > > When cluster size is big enough it can lead offset overflow > >=20 > > Maybe "it can lead to an offset overflow in" > > > in qcow2_alloc_clusters_at(). This patch fixes it. >=20 > Sure. >=20 > > >=20 > > > The allocation each time is stopped at L2 table boundary > > "The allocation is stopped each time at" >=20 > Sure. >=20 > >=20 > > > (see handle_alloc()), so the possible maximum bytes could be > > >=20 > > > 2^(cluster_bits - 3 + cluster_bits) > >=20 > > So if understand cluster_bits - 3 is used to compute the number of en= try by L2 > > and the additional cluster_bits is to take into account each clusters= referenced > > by the L2 entries. >=20 > 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. >=20 > > It makes sense. > >=20 > > >=20 > > > so int is safe for cluster_bits<=3D17, unsafe otherwise. > > >=20 > > > Reviewed-by: Max Reitz > > > Signed-off-by: Hu Tao > > > --- > > > block/qcow2-refcount.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > >=20 > > > 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 =3D bs->opaque; > > > uint64_t cluster_index; > > > uint64_t old_free_cluster_index; > > > - int i, refcount, ret; > > > + uint64_t i; > > > + int refcount, ret; > > > + > >=20 > >=20 > > > + assert(nb_clusters >=3D 0); > > > + if (nb_clusters =3D=3D 0) { > > > + return 0; > > > + } > > ^ > > Adding a a line on the commit message about this assertion and chunk = of code > > would be helpful. >=20 > How about >=20 > Add an assert to guard the comparation between signed and unsigned yes >=20 > ? >=20 > >=20 > > Best regards > >=20 > > Beno=EEt > >=20 > > > =20 > > > /* Check how many clusters there are free */ > > > cluster_index =3D offset >> s->cluster_bits; > > > --=20 > > > 1.8.5.2.229.g4448466 > > >=20 > > >=20 >=20