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