From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48893) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUZhv-0008Ql-Aw for qemu-devel@nongnu.org; Tue, 02 Aug 2016 09:26:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bUZhq-0003xQ-4Z for qemu-devel@nongnu.org; Tue, 02 Aug 2016 09:26:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39220) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUZhp-0003x9-Ub for qemu-devel@nongnu.org; Tue, 02 Aug 2016 09:26:02 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 63EAF7CDE1 for ; Tue, 2 Aug 2016 13:26:01 +0000 (UTC) Date: Tue, 2 Aug 2016 15:25:58 +0200 From: Igor Mammedov Message-ID: <20160802152558.76e5ccc8@nial.brq.redhat.com> In-Reply-To: References: <1470109301-12966-1-git-send-email-famz@redhat.com> <20160802095507.6055409b@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qdev: Fix use after free in qdev_init_nofail error path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Fam Zheng , jsnow@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com On Tue, 2 Aug 2016 15:05:28 +0200 Paolo Bonzini wrote: > On 02/08/2016 09:55, Igor Mammedov wrote: > > On Tue, 2 Aug 2016 11:41:41 +0800 > > Fam Zheng wrote: > > > >> Since 69382d8b (qdev: Fix object reference leak in case device.realize() > >> fails), object_property_set_bool could release the object. The error > >> path wants the type name, so hold an reference before realizing it. > >> > >> Cc: Igor Mammedov > >> Signed-off-by: Fam Zheng > >> --- > >> hw/core/qdev.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c > >> index ee4a083..5783442 100644 > >> --- a/hw/core/qdev.c > >> +++ b/hw/core/qdev.c > >> @@ -354,12 +354,14 @@ void qdev_init_nofail(DeviceState *dev) > >> > >> assert(!dev->realized); > >> > >> + object_ref(OBJECT(dev)); > >> object_property_set_bool(OBJECT(dev), true, "realized", &err); > >> if (err) { > >> error_reportf_err(err, "Initialization of device %s failed: ", > >> object_get_typename(OBJECT(dev))); > >> exit(1); > >> } > >> + object_unref(OBJECT(dev)); > >> } > >> > >> void qdev_machine_creation_done(void) > > > > I'm not sure that this is the right fix, commit 69382d8b only affects > > reference created by realize() itself. > > Probably reference counting wrong somewhere else, > > for typical device call sequence is following: > > > > qdev_create() { > > object_new() -> ref == 1 > > qdev_set_parent_bus() -> ref == 2 > > object_unref() -> ref == 1 > > } -> ref == 1 > > > > do property settings and other stuff ... > > > > > > qdev_init_nofail() { called with ref == 1 > > object_property_set_bool(true, "realized") > > if error: > > ref == 1 > > If there is an error and the device was unattached, you get here: > > if (unattached_parent) { > object_unparent(OBJECT(dev)); > unattached_count--; > } > > and object_unparent undoes qdev_set_parent_bus so that the refcount > drops to 0. I've sent v2 of patch that fixes issue with error handling in slightly another way (A little bit more generic and consistent than just wrapping realize with ref/unref) > > Paolo > > > else: > > ref == 2 (+1 for implicitly assigned parent) > > } > > > >