From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50331) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZfdd-0001xG-QH for qemu-devel@nongnu.org; Wed, 09 Sep 2015 09:42:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZZfdZ-0007UL-LY for qemu-devel@nongnu.org; Wed, 09 Sep 2015 09:42:13 -0400 References: <1441742995-11794-1-git-send-email-mreitz@redhat.com> <1441742995-11794-2-git-send-email-mreitz@redhat.com> <20150909084519.GB4860@noname.redhat.com> From: Max Reitz Message-ID: <55F03722.2000301@redhat.com> Date: Wed, 9 Sep 2015 15:41:54 +0200 MIME-Version: 1.0 In-Reply-To: <20150909084519.GB4860@noname.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="AX3esCRjNPejWHTQtI50XFCp5j1GlOsGb" Subject: Re: [Qemu-devel] [PATCH 1/2] qcow2: Make size_to_clusters() return int64_t List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --AX3esCRjNPejWHTQtI50XFCp5j1GlOsGb Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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, bu= t >> since size_to_clusters() truncated the returned value, that check neve= r >> did anything useful). >> >> Signed-off-by: Max Reitz >=20 > You seem to fix a few of the callers as well, which is a good thing. >=20 > 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; >> =20 >> index_in_cluster =3D (offset >> 9) & (s->cluster_sectors - 1); >=20 > 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. >=20 > 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 unalloca= ted 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; >> =20 >> for (i =3D 0; i < nb_clusters; i++) { >> uint64_t l2_entry =3D be64_to_cpu(l2_table[l2_index + i]); >=20 > 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, uin= t64_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; >=20 > It looks like size isn't limited to a single L2 table there yet, so thi= s > 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 *b= s, 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_dis= card) >> { >> BDRVQcow2State *s =3D bs->opaque; >> uint64_t *l2_table; >> @@ -1441,6 +1441,7 @@ static int discard_single_l2(BlockDriverState *b= s, uint64_t offset, >> =20 >> /* Limit nb_clusters to one L2 table */ >> nb_clusters =3D MIN(nb_clusters, s->l2_size - l2_index); >> + assert(nb_clusters <=3D INT_MAX); >> =20 >> for (i =3D 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 =3D bs->opaque; >> uint64_t end_offset; >> - unsigned int nb_clusters; >> + uint64_t nb_clusters; >> int ret; >> =20 >> end_offset =3D offset + (nb_sectors << BDRV_SECTOR_BITS); >=20 > We can actually assert nb_clusters <=3D INT_MAX directly after assignin= g > 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. >=20 >> @@ -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 =3D bs->opaque; >> uint64_t *l2_table; >> @@ -1560,6 +1561,7 @@ static int zero_single_l2(BlockDriverState *bs, = uint64_t offset, >> =20 >> /* Limit nb_clusters to one L2 table */ >> nb_clusters =3D MIN(nb_clusters, s->l2_size - l2_index); >> + assert(nb_clusters <=3D INT_MAX); >> =20 >> for (i =3D 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 =3D bs->opaque; >> - unsigned int nb_clusters; >> + uint64_t nb_clusters; >> int ret; >> =20 >> /* The zero flag is only supported by version 3 and newer */ >=20 > 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(BDRVQcow= 2State *s, int64_t offset) >> return offset & (s->cluster_size - 1); >> } >> =20 >> -static inline int size_to_clusters(BDRVQcow2State *s, int64_t size) >> +static inline int64_t size_to_clusters(BDRVQcow2State *s, int64_t siz= e) >> { >> return (size + (s->cluster_size - 1)) >> s->cluster_bits; >> } >=20 > Kevin >=20 Thanks for reviewing! Max --AX3esCRjNPejWHTQtI50XFCp5j1GlOsGb Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJV8DciAAoJEDuxQgLoOKytFfEH/0nghH1CwasXtdfmPwkRxKfm XN+o1euqi8DITOpedxwCSqH7qEXWYHT1ISzNpp2aGBxiW0Kt1lO+rePIJ8O1D+XQ nwVQk3/mFh5b+M53/cHHwLq9bAjtjVhjmansNRe2R7yxO+vK+IHburSajwB99CWV 0gXx0fY6MqC/gB74+QH8IMuRFsHLElzKv5IKf29y0seuOwDH310BCBLUaEui39Eb 7t4bpkj97lre8SjZaTR75Z4+6kgLscA8l4kk5tfgVrJNQalaBr4G1HVp1qrE6qCN xify5u1nQcklP99WszQ5ongdA/GneCcMCqWwf0etCOxub4IWMNhlUPPPA0cMvZg= =ziao -----END PGP SIGNATURE----- --AX3esCRjNPejWHTQtI50XFCp5j1GlOsGb--