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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
