From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59961) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WOxoS-0003Vh-MD for qemu-devel@nongnu.org; Sat, 15 Mar 2014 19:16:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WOxoF-0005C9-He for qemu-devel@nongnu.org; Sat, 15 Mar 2014 19:16:20 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:56214 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WOxoF-0005Az-Bg for qemu-devel@nongnu.org; Sat, 15 Mar 2014 19:16:07 -0400 Date: Sun, 16 Mar 2014 00:16:05 +0100 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140315231604.GA3067@irqsave.net> References: <1394916954-21774-1-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1394916954-21774-1-git-send-email-mreitz@redhat.com> 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: Max Reitz Cc: Kevin Wolf , Laszlo Ersek , qemu-devel@nongnu.org, Stefan Hajnoczi 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_offse= t > 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 I would say "actually should never fail". > (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(BlockDriv= erState *bs, int reftable_index, > } > > fail: I don't like the label name because its real meaning is exit: it handle b= oth fail and regular exits. > - 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_blo= ck); > - } else { > - ret =3D qcow2_cache_put(bs, s->refcount_block_cache, &refc= ount_block); > - } > + /* This should never fail, as it would only do so if the given= refcount > + * block cannot be found in the cache. As this is impossible a= s long as > + * there are no bugs, assert the success. */ > + int tmp =3D qcow2_cache_put(bs, s->refcount_block_cache, &refc= ount_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; The function documentation says: /* * Allocates a new cluster for the given refcount block (represented by i= ts * offset in the image file) and copies the current content there. This f= unction * does _not_ decrement the reference count for the currently occupied cl= uster. * * This function prints an informative message to stderr on error (and re= turns * -errno); on success, 0 is returned. */ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_= index, uint64_t offset) So why are we returning new_offset on success ?=20 > } > > -- > 1.9.0 > >