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

[John Ferlan]

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.

Tks -

John