From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50022) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPUhc-00032K-4O for qemu-devel@nongnu.org; Mon, 17 Mar 2014 06:23:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPUhW-0004Tb-4t for qemu-devel@nongnu.org; Mon, 17 Mar 2014 06:23:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59628) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPUhV-0004Sk-SZ for qemu-devel@nongnu.org; Mon, 17 Mar 2014 06:23:22 -0400 Message-ID: <5326CD13.7020006@redhat.com> Date: Mon, 17 Mar 2014 11:23:15 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1394916954-21774-1-git-send-email-mreitz@redhat.com> <20140315232624.GB3067@irqsave.net> In-Reply-To: <20140315232624.GB3067@irqsave.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] qcow2: Fix fail path in realloc_refcount_block() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-1?Q?Beno=EEt_Canet?= , Max Reitz Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 03/16/14 00:26, Beno=EEt Canet wrote: > The Saturday 15 Mar 2014 =E0 21:55:54 (+0100), Max Reitz wrote : >> If qcow2_alloc_clusters() fails, new_offset and ret will both be >> negative after the fail label, thus passing the first if condition and >> subsequently resulting in a call of qcow2_free_clusters() with an >> invalid (negative) offset parameter. Fix this by checking for new_offs= et >> being positive instead. >> >> While we're at it, clean up the whole fail path. qcow2_cache_put() >> actually can never fail, hence the return value can safely be ignored >> (aside from asserting that it indeed did not fail). >> >> Furthermore, there is no reason to give QCOW2_DISCARD_ALWAYS to >> qcow2_free_clusters(), a mere QCOW2_DISCARD_OTHER will suffice. >> >> Signed-off-by: Max Reitz >> Suggested-by: Laszlo Ersek >> --- >> block/qcow2-refcount.c | 20 +++++++++++--------- >> 1 file changed, 11 insertions(+), 9 deletions(-) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 6151148..b111319 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -1440,20 +1440,22 @@ static int64_t realloc_refcount_block(BlockDri= verState *bs, int reftable_index, >> } >> =20 >> fail: >> - if (new_offset && (ret < 0)) { >> - qcow2_free_clusters(bs, new_offset, s->cluster_size, >> - QCOW2_DISCARD_ALWAYS); >> - } >> if (refcount_block) { >> - if (ret < 0) { >> - qcow2_cache_put(bs, s->refcount_block_cache, &refcount_bl= ock); >> - } else { >> - ret =3D qcow2_cache_put(bs, s->refcount_block_cache, &ref= count_block); >> - } >> + /* This should never fail, as it would only do so if the give= n refcount >> + * block cannot be found in the cache. As this is impossible = as long as >> + * there are no bugs, assert the success. */ >> + int tmp =3D qcow2_cache_put(bs, s->refcount_block_cache, &ref= count_block); >> + assert(tmp =3D=3D 0); >> } >> + >> if (ret < 0) { >> + if (new_offset > 0) { >> + qcow2_free_clusters(bs, new_offset, s->cluster_size, >> + QCOW2_DISCARD_OTHER); >> + } >> return ret; >> } >> + >> return new_offset; >> } >> =20 >> --=20 >> 1.9.0 >> >> >=20 > In fact in wonder if these if after the fail label could be replaced by= a proper > goto chain. That was my first idea too. I didn't propose it to Max because it's complicated by the fact that while you need to release the cache entry in any case (both success & failure), you roll back the cluster alloc only in case of failure. Using a proper goto chain (without any "if"s) means that you'd have to keep it fully separate from the success path, hence duplicate the cache entry release on the success path (including the assert()). The ifs above allow you to share the action. It's a matter of taste; I usually like to code actions only once. But I don't insist of course. Laszlo