[2/2] qom: Assert that objects being destroyed have no parent

QOM reference counting bugs are often hard to detect, but there's
one kind of bug that's easier: if we are freeing an object but is
still attached to a parent, it means the reference count is wrong
(because the parent always hold a reference to their children).

Add an assertion to make sure we detect those cases.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qom/object.c | 1 +
 1 file changed, 1 insertion(+)

Hi

On Wed, Dec 16, 2020 at 2:41 AM Eduardo Habkost <ehabkost@redhat.com> wrote:

> QOM reference counting bugs are often hard to detect, but there's
> one kind of bug that's easier: if we are freeing an object but is
> still attached to a parent, it means the reference count is wrong
> (because the parent always hold a reference to their children).
>
> Add an assertion to make sure we detect those cases.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>

On the principle, I fully agree. But the risk is high to introduce
regression if objects are manipulated in strange ways.

I remember I wanted object_unref() to automatically remove itself from the
parent when the last ref is dropped. I think there were similar concerns.

Maybe with --enable-qom-debug ? (removing the -cast)

---
>  qom/object.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/qom/object.c b/qom/object.c
> index f2ae6e6b2a..5cfed6d7c6 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -685,6 +685,7 @@ static void object_finalize(void *data)
>      object_deinit(obj, ti);
>
>      g_assert(obj->ref == 0);
> +    g_assert(obj->parent == NULL);
>      if (obj->free) {
>          obj->free(obj);
>      }
> --
> 2.28.0
>
>

On Wed, Dec 16, 2020 at 11:53:06AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Dec 16, 2020 at 2:41 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > QOM reference counting bugs are often hard to detect, but there's
> > one kind of bug that's easier: if we are freeing an object but is
> > still attached to a parent, it means the reference count is wrong
> > (because the parent always hold a reference to their children).
> >
> > Add an assertion to make sure we detect those cases.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >
> 
> On the principle, I fully agree. But the risk is high to introduce
> regression if objects are manipulated in strange ways.

Isn't the point that we're broken already. We have a QOM instance
in the tree which has a zero reference count and has been freed.
As soon as something touches that object in the tree, we're liable
to crash & burn touching free'd memory. So it seems the choices are
between crash fast where we see the problem, or crash eventually
at a place where we can't easily trace back to the root cause.

> I remember I wanted object_unref() to automatically remove itself from the
> parent when the last ref is dropped. I think there were similar concerns.

Automatically removing itself would be hiding the bug in whatever
code has mistakenly removed a reference it didn't own.

> 
> Maybe with --enable-qom-debug ? (removing the -cast)
> 
> ---
> >  qom/object.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/qom/object.c b/qom/object.c
> > index f2ae6e6b2a..5cfed6d7c6 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -685,6 +685,7 @@ static void object_finalize(void *data)
> >      object_deinit(obj, ti);
> >
> >      g_assert(obj->ref == 0);
> > +    g_assert(obj->parent == NULL);
> >      if (obj->free) {
> >          obj->free(obj);
> >      }
> > --
> > 2.28.0
> >
> >

Regards,
Daniel

On 16/12/20 08:53, Marc-André Lureau wrote:
> 
> On the principle, I fully agree. But the risk is high to introduce 
> regression if objects are manipulated in strange ways.
> 
> I remember I wanted object_unref() to automatically remove itself from 
> the parent when the last ref is dropped. I think there were similar 
> concerns.

unref and unparent are two very different operations; the former means 
*I* am done with this object, the latter means *QEMU* is done with this 
object (even though there may be a few reference held, e.g. on the call 
stack or by RCU).  Since object_unparent operates on global state, you 
can even call object_unparent if you don't own yourself a reference to 
the object and just got the pointer from the caller.

While unref is a "mechanical" operation of dropping a reference and 
possibly freeing the object, unparent is an active operation that 
includes for example dropping reference cycles or in general detaching 
from other places that are known to hold references to this object.

This is not a concept that is specific to QEMU, I think I read somewhere 
that LibreOffice's UI library does something similar, calling it "dispose".

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 16/12/20 08:53, Marc-André Lureau wrote:
>> 
>> On the principle, I fully agree. But the risk is high to introduce 
>> regression if objects are manipulated in strange ways.
>> 
>> I remember I wanted object_unref() to automatically remove itself from 
>> the parent when the last ref is dropped. I think there were similar 
>> concerns.
>
> unref and unparent are two very different operations; the former means 
> *I* am done with this object, the latter means *QEMU* is done with this 
> object (even though there may be a few reference held, e.g. on the call 
> stack or by RCU).  Since object_unparent operates on global state, you 
> can even call object_unparent if you don't own yourself a reference to 
> the object and just got the pointer from the caller.
>
> While unref is a "mechanical" operation of dropping a reference and 
> possibly freeing the object, unparent is an active operation that 
> includes for example dropping reference cycles or in general detaching 
> from other places that are known to hold references to this object.

This all sounds like good material for a QOM object lifetime section of
docs/devel/qom.rst

>
> This is not a concept that is specific to QEMU, I think I read somewhere 
> that LibreOffice's UI library does something similar, calling it "dispose".
>
> Paolo

Eduardo Habkost <ehabkost@redhat.com> writes:

> QOM reference counting bugs are often hard to detect, but there's
> one kind of bug that's easier: if we are freeing an object but is
> still attached to a parent, it means the reference count is wrong
> (because the parent always hold a reference to their children).
>
> Add an assertion to make sure we detect those cases.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

I'm happy with an assert and crash early approach if the alternative it
scratching your head when things crash in weird ways. I'll of course
defer to the maintainer but from me:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  qom/object.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/qom/object.c b/qom/object.c
> index f2ae6e6b2a..5cfed6d7c6 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -685,6 +685,7 @@ static void object_finalize(void *data)
>      object_deinit(obj, ti);
>  
>      g_assert(obj->ref == 0);
> +    g_assert(obj->parent == NULL);
>      if (obj->free) {
>          obj->free(obj);
>      }