qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] qdev: Fix use after free in qdev_init_nofail er


From: Igor Mammedov
Subject: Re: [Qemu-devel] [PATCH] qdev: Fix use after free in qdev_init_nofail error path
Date: Tue, 2 Aug 2016 15:25:58 +0200

On Tue, 2 Aug 2016 15:05:28 +0200
Paolo Bonzini <address@hidden> wrote:

> On 02/08/2016 09:55, Igor Mammedov wrote:
> > On Tue,  2 Aug 2016 11:41:41 +0800
> > Fam Zheng <address@hidden> 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 <address@hidden>
> >> Signed-off-by: Fam Zheng <address@hidden>
> >> ---
> >>  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)
> >  }
> > 
> >   




reply via email to

[Prev in Thread] Current Thread [Next in Thread]