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

[John Ferlan]

On 06/05/2017 03:35 AM, Peter Krempa wrote:
> On Fri, Jun 02, 2017 at 06:17:18 -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 objects.
>>
>> 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 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/util/virobject.c b/src/util/virobject.c
>> index 9f5f187..e0465b1 100644
>> --- a/src/util/virobject.c
>> +++ b/src/util/virobject.c
> 
> [..]
> 
>> @@ -156,6 +159,7 @@ virClassNew(virClassPtr parent,
>>      if (VIR_STRDUP(klass->name, name) < 0)
>>          goto error;
>>      klass->magic = virAtomicIntInc(&magicCounter);
>> +    assert(klass->magic <= 0xCAFEFFFF);
> 
> Library code should not use assert. This makes the client application
> crash on library usage. This also does not deterministically make this
> crash all the time. Only if you initialize the class that exceeds the
> marker, which depends on serialization of the initialization.
> 

Idea came from Dan - perhaps I took the words too literally, see

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

I can go with virReportError if that's preferable.

John