Re: [libvirt] [PATCH 4/9] util: Add magic_marker for all virObjects

On Tue, May 30, 2017 at 07:38:16AM -0400, John Ferlan wrote: > The virObject logic "assumes" that whatever is passed to its API's > would be some sort of virObjectPtr; however, if it is not then some > really bad things can happen. > > So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock}, > and virObjectIsClass and the virObject and virObjectLockable class > consumers have been well behaved and code well tested. Soon there will > be more consumers and one such consumer tripped over this during testing > by passing a virHashTablePtr to virObjectIsClass which ends up calling > virClassIsDerivedFrom using "obj->klass", which wasn't really a klass > object causing one of those bad things to happen. > > To avoid the future possibility that a non virObject class memory was > passed to some virObject* API, let's add a "magic_marker" to the base > virObject class that contains a value "0xFEEDBEEF", then any place in > the code which would accept or process the base opaque virObjectPtr, > compare the magic_marker against 0xFEEDBEEF to make sure this is an > object allocated by this code. > > It is still left up to the caller to handle the failed API calls just > as it would be if it passed a NULL opaque pointer anyobj. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/util/virobject.c | 12 ++++++++---- > src/util/virobject.h | 1 + > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/src/util/virobject.c b/src/util/virobject.c > index 9f5f187..a1934941 100644 > --- a/src/util/virobject.c > +++ b/src/util/virobject.c > @@ -47,10 +47,12 @@ struct _virClass { > virObjectDisposeCallback dispose; > }; > > +#define VIR_OBJECT_NOTVALID(obj) (!obj || obj->magic_marker != 0xFEEDBEEF) > + > #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ > do { \ > virObjectPtr obj = anyobj; \ > - if (!obj) \ > + if (VIR_OBJECT_NOTVALID(obj)) \ > VIR_WARN("Object %p is not a virObject class instance", anyobj);\ > else \ > VIR_WARN("Object %p (%s) is not a %s instance", \ > @@ -212,6 +214,7 @@ virObjectNew(virClassPtr klass) > return NULL; > > obj->u.s.magic = klass->magic; > + obj->magic_marker = 0xFEEDBEEF; > obj->klass = klass; > virAtomicIntSet(&obj->u.s.refs, 1); > > @@ -272,7 +275,7 @@ virObjectUnref(void *anyobj) > { > virObjectPtr obj = anyobj; > > - if (!obj) > + if (VIR_OBJECT_NOTVALID(obj)) > return false; > > bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs); > @@ -289,6 +292,7 @@ virObjectUnref(void *anyobj) > /* Clear & poison object */ > memset(obj, 0, obj->klass->objectSize); > obj->u.s.magic = 0xDEADBEEF; > + obj->magic_marker = 0xDEADBEEF; > obj->klass = (void*)0xDEADBEEF; > VIR_FREE(obj); > } > @@ -311,7 +315,7 @@ virObjectRef(void *anyobj) > { > virObjectPtr obj = anyobj; > > - if (!obj) > + if (VIR_OBJECT_NOTVALID(obj)) > return NULL; > virAtomicIntInc(&obj->u.s.refs); > PROBE(OBJECT_REF, "obj=%p", obj); > @@ -389,7 +393,7 @@ virObjectIsClass(void *anyobj, > virClassPtr klass) > { > virObjectPtr obj = anyobj; > - if (!obj) > + if (VIR_OBJECT_NOTVALID(obj)) > return false; > > return virClassIsDerivedFrom(obj->klass, klass); > diff --git a/src/util/virobject.h b/src/util/virobject.h > index f4c292b..89f8050 100644 > --- a/src/util/virobject.h > +++ b/src/util/virobject.h > @@ -51,6 +51,7 @@ struct _virObject { > int refs; > } s; > } u; > + unsigned int magic_marker; > virClassPtr klass; > }; I'm wondering whether this will risk re-introducing the bug fixed in commit fca4f2334072d87f7faeb2948e6f83201309e1b9 Author: Eric Blake <eblake(a)redhat.com&gt; Date: Thu Dec 12 16:01:15 2013 -0700 object: require maximal alignment in base class I'm also thinking we don't really need to have 2 magic fields in the same struct - we already have a 'magic' field that is initialized from the class object magic value. Now, this existing magic is different for each object subclass - we allocate class magic starting with static unsigned int magicCounter = 0xCAFE0000; I'm thinking though, that we're never going to have > 65556 different sub-classes (well at least not for a long time). So instead of adding this new field you could just check ((object->u.s.magic & 0xCAFE0000) == 0xCAFE0000)