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

On 05/30/2017 09:00 AM, Daniel P. Berrange wrote: > 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) > Oh right - just mask away the pesky counter portion... This works me too. I can adjust.