From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48509) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XIvay-0005Ws-B8 for qemu-devel@nongnu.org; Sun, 17 Aug 2014 04:13:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XIvat-0000om-1W for qemu-devel@nongnu.org; Sun, 17 Aug 2014 04:13:44 -0400 Received: from mail-wg0-x22d.google.com ([2a00:1450:400c:c00::22d]:33179) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XIvas-0000oi-RZ for qemu-devel@nongnu.org; Sun, 17 Aug 2014 04:13:38 -0400 Received: by mail-wg0-f45.google.com with SMTP id x12so3729195wgg.28 for ; Sun, 17 Aug 2014 01:13:38 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <53F0642E.7090508@redhat.com> Date: Sun, 17 Aug 2014 10:13:34 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <201408091133130940565@sangfor.com> In-Reply-To: <201408091133130940565@sangfor.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qemu-nbd: NULL nbd export pointer dereference after kill (TERMINATE) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Zhang Haoyu , qemu-devel Il 09/08/2014 05:33, Zhang Haoyu ha scritto: > After receive TERMINATE signal, qemu nbd state is set to TERMINATE, then in the main loop, > nbd_export_close -> nbd_export_put is performed, but sometimes exp->refcount still greater than zero after nbd_export_put, > so the qemu nbd state has not been set to TERMINATED, then in next cycle, NULL exp will be dereference. > do { > main_loop_wait(false); > if (state == TERMINATE) { > state = TERMINATING; > nbd_export_close(exp); > nbd_export_put(exp); > exp = NULL; > } > } while (state != TERMINATED); This patch will cause use-after-frees if the reference count is dropped by the legitimate owner. Where is the NULL exp dereferenced? Perhaps that's where a guard has to be added. Also, blockdev-nbd.c is not part of qemu-nbd. Paolo > Signed-off-by: Zhang Haoyu > --- > blockdev-nbd.c | 5 +++-- > include/block/nbd.h | 2 +- > nbd.c | 5 ++++- > qemu-nbd.c | 6 +++--- > 4 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > index b3a2474..c339081 100644 > --- a/blockdev-nbd.c > +++ b/blockdev-nbd.c > @@ -65,8 +65,9 @@ static void nbd_close_notifier(Notifier *n, void *data) > notifier_remove(&cn->n); > QTAILQ_REMOVE(&close_notifiers, cn, next); > > - nbd_export_close(cn->exp); > - nbd_export_put(cn->exp); > + do { > + cn->exp = nbd_export_put(cn->exp); > + } while (cn->exp); > g_free(cn); > } > > diff --git a/include/block/nbd.h b/include/block/nbd.h > index 9e835d2..1912ef0 100644 > --- a/include/block/nbd.h > +++ b/include/block/nbd.h > @@ -90,7 +90,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, > void (*close)(NBDExport *)); > void nbd_export_close(NBDExport *exp); > void nbd_export_get(NBDExport *exp); > -void nbd_export_put(NBDExport *exp); > +NBDExport *nbd_export_put(NBDExport *exp); > > BlockDriverState *nbd_export_get_blockdev(NBDExport *exp); > > diff --git a/nbd.c b/nbd.c > index e7d1cee..2fccba5 100644 > --- a/nbd.c > +++ b/nbd.c > @@ -991,7 +991,7 @@ void nbd_export_get(NBDExport *exp) > exp->refcount++; > } > > -void nbd_export_put(NBDExport *exp) > +NBDExport *nbd_export_put(NBDExport *exp) > { > assert(exp->refcount > 0); > if (exp->refcount == 1) { > @@ -1006,7 +1006,10 @@ void nbd_export_put(NBDExport *exp) > } > > g_free(exp); > + return NULL; > } > + > + return exp; > } > > BlockDriverState *nbd_export_get_blockdev(NBDExport *exp) > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 626e584..e1f3577 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -735,9 +735,9 @@ int main(int argc, char **argv) > main_loop_wait(false); > if (state == TERMINATE) { > state = TERMINATING; > - nbd_export_close(exp); > - nbd_export_put(exp); > - exp = NULL; > + do { > + exp = nbd_export_put(exp); > + } while (exp); > } > } while (state != TERMINATED); > >