[libvirt] [PATCH 7/7] util: Add safety net of checks to ensure valid object

[John Ferlan]

On 07/28/2017 01:19 PM, Pavel Hrdina wrote:
> On Fri, Jul 28, 2017 at 12:39:01PM -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, this patch adds two new checks - one
>> to validate that the object has the 0xCAFExxxx value in obj->->u.s.magic
>> and the other to ensure obj->u.s.magic doesn't "wrap" some day to
>> 0xCAFF0000 if we ever get that many object classes. The check is also
>> moved before the name VIR_STRDUP to avoid the extra VIR_FREE that would
>> be required on the failure path.
> 
> This doesn't add any safeguard and for most virObject(Ref|Unref) calls
> we don't check the return value.  This is basically a programming error
> as well and we should consider start using abort().
> 

And yes, this is the programming error - it was awfully stupid, but IIRC
it also didn't "jump out" right away what the problem is/was. It was far
worse for *Ref/Unref, but lock was interesting too insomuch as I
wouldn't get the lock so perhaps two threads would make a change at the
same time and we may never know unless we check lock status.

>>
>> 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.

And of course this was my escape clause because of void Lock's.


>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/util/virobject.c | 26 +++++++++++++++++++-------
>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/util/virobject.c b/src/util/virobject.c
>> index 2cf4743..dd4c39a 100644
>> --- a/src/util/virobject.c
>> +++ b/src/util/virobject.c
>> @@ -47,14 +47,21 @@ struct _virClass {
>>      virObjectDisposeCallback dispose;
>>  };
>>  
>> +#define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0xCAFE0000) != 0xCAFE0000))
> 
> This is not correct, it should be:
> 
>     ((obj->u.s.magic & 0xFFFF0000) != 0xCAFE0000)
> 

Oh right and I see it was a straight cut-n-paste from Dan's reply too:

https://www.redhat.com/archives/libvir-list/2017-May/msg01211.html


>> +
>>  #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass)                    \
>>      do {                                                                    \
>>          virObjectPtr obj = anyobj;                                          \
>> -        if (!obj)                                                           \
>> -            VIR_WARN("Object cannot be NULL");                              \
>> -        else                                                                \
>> +        if (VIR_OBJECT_NOTVALID(obj)) {                                     \
>> +            if (!obj)                                                       \
>> +                VIR_WARN("Object cannot be NULL");                          \
>> +            else                                                            \
>> +                VIR_WARN("Object %p has a bad magic number %X",             \
>> +                         obj, obj->u.s.magic);                              \
>> +        } else {                                                            \
>>              VIR_WARN("Object %p (%s) is not a %s instance",                 \
>>                        anyobj, obj->klass->name, #objclass);                 \
>> +        }                                                                   \
>>      } while (0)
>>  
>>  
>> @@ -177,9 +184,14 @@ virClassNew(virClassPtr parent,
>>          goto error;
>>  
>>      klass->parent = parent;
>> +    klass->magic = virAtomicIntInc(&magicCounter);
>> +    if (klass->magic > 0xCAFEFFFF) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("too many object classes defined"));
>> +        goto error;
>> +    }
> 
> This will probably never happen :).
> 
> Pavel
> 

Agreed, but if it does happen then I'd be the last to touch right... So
I'd get the blame.  In any case, another one of those Dan suggestions:

https://www.redhat.com/archives/libvir-list/2017-May/msg01221.html

If this is not desired - that's fine I can drop it. Cannot say I didn't
try though.

Thanks for the quick review! With a freeze on at least this may give
time for others to chime in with thoughts as well.

John