From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51456) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XGjDX-0000C1-1k for qemu-devel@nongnu.org; Mon, 11 Aug 2014 02:36:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XGjDQ-0001vA-Bt for qemu-devel@nongnu.org; Mon, 11 Aug 2014 02:36:26 -0400 Received: from [58.251.49.30] (port=60178 helo=mail.sangfor.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XGjDQ-0001t2-0y for qemu-devel@nongnu.org; Mon, 11 Aug 2014 02:36:20 -0400 Date: Mon, 11 Aug 2014 14:34:20 +0800 From: "Zhang Haoyu" References: <201408091333190205168@sangfor.com>, <33183CC9F5247A488A2544077AF1902086C29A22@SZXEMA503-MBS.china.huawei.com> Message-ID: <201408111434143940940@sangfor.com> Mime-Version: 1.0 Content-Type: text/plain; charset="gb2312" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [patch] qcow2: double free snapshots List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Gonglei (Arei)" , qemu-devel >Hi, > >> Subject: [Qemu-devel] [patch] qcow2: double free snapshots >> >> In qcow2_open(), if qcow2_read_snapshots() failed, qcow2_open() -> >> qcow2_free_snapshots() will be called, NULL snapshots dereference happened. >> because qcow2_free_snapshots has been performed before in the fail case of >> qcow2_read_snapshots(). >> shown as below callstack, >> qcow2_open >> |- qcow2_read_snapshots >> |-- goto fail; >> |-- qcow2_free_snapshots >> |- goto fail; >> |- qcow2_free_snapshots /* on this case, NULL snapshots dereference >> happened. */ >> >> Signed-off-by: Zhang Haoyu >> --- >> block/qcow2-snapshot.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c >> index 0aa9def..15c0513 100644 >> --- a/block/qcow2-snapshot.c >> +++ b/block/qcow2-snapshot.c >> @@ -31,6 +31,10 @@ void qcow2_free_snapshots(BlockDriverState *bs) >> BDRVQcowState *s = bs->opaque; >> int i; >> >> + if (!s || !s->snapshots) { >> + return; >> + } >> + >> for(i = 0; i < s->nb_snapshots; i++) { >> g_free(s->snapshots[i].name); >> g_free(s->snapshots[i].id_str); >> -- >> 1.9.4.msysgit.0 >> >Yes, the code path is right. But I don't think this check is necessary. >The first free has set "s->nb_snapshots = 0", so it is impossibility go into >g_free(s->snapshots[i].name), and g_free(s->snapshots) is OK, though it is NULL. > Yes, you are right, ignore this patch, please. Thanks, Zhang Haoyu >Best regards, >-Gonglei