From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42451) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUZO3-00037z-Cs for qemu-devel@nongnu.org; Tue, 02 Aug 2016 09:05:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bUZO0-0007O0-44 for qemu-devel@nongnu.org; Tue, 02 Aug 2016 09:05:35 -0400 Received: from mail-wm0-x244.google.com ([2a00:1450:400c:c09::244]:35268) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUZNz-0007Ns-Ts for qemu-devel@nongnu.org; Tue, 02 Aug 2016 09:05:32 -0400 Received: by mail-wm0-x244.google.com with SMTP id i5so30681936wmg.2 for ; Tue, 02 Aug 2016 06:05:31 -0700 (PDT) Sender: Paolo Bonzini References: <1470109301-12966-1-git-send-email-famz@redhat.com> <20160802095507.6055409b@nial.brq.redhat.com> From: Paolo Bonzini Message-ID: Date: Tue, 2 Aug 2016 15:05:28 +0200 MIME-Version: 1.0 In-Reply-To: <20160802095507.6055409b@nial.brq.redhat.com> Content-Type: text/plain; charset=windows-1252 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: Igor Mammedov , Fam Zheng Cc: jsnow@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com 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. Paolo > else: > ref == 2 (+1 for implicitly assigned parent) > } > >