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

[Daniel P. Berrange]

On Tue, May 30, 2017 at 10:43:35AM -0400, John Ferlan wrote:
> 
> 
> 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 at redhat.com>
> >> ---
> >>  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 at redhat.com>
> >   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.

Should also put an assert in the virClassNew method that the new magic is
<= 0xCAFEFFFF as a safety net for 20 years time when we finally have
65536 classes :-)

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|