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.
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@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))
+
#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;
+ }
if (VIR_STRDUP(klass->name, name) < 0)
goto error;
- klass->magic = virAtomicIntInc(&magicCounter);
klass->objectSize = objectSize;
klass->dispose = dispose;
@@ -331,7 +343,7 @@ virObjectUnref(void *anyobj)
{
virObjectPtr obj = anyobj;
- if (!obj)
+ if (VIR_OBJECT_NOTVALID(obj))
return false;
bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs);
@@ -370,7 +382,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);
@@ -532,7 +544,7 @@ virObjectIsClass(void *anyobj,
virClassPtr klass)
{
virObjectPtr obj = anyobj;
- if (!obj)
+ if (VIR_OBJECT_NOTVALID(obj))
return false;
return virClassIsDerivedFrom(obj->klass, klass);
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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().
>
> 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@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)
> +
> #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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
>
> 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@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))
> +
> #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;
> + }
> if (VIR_STRDUP(klass->name, name) < 0)
> goto error;
> - klass->magic = virAtomicIntInc(&magicCounter);
> klass->objectSize = objectSize;
> klass->dispose = dispose;
>
> @@ -331,7 +343,7 @@ virObjectUnref(void *anyobj)
> {
> virObjectPtr obj = anyobj;
>
> - if (!obj)
> + if (VIR_OBJECT_NOTVALID(obj))
> return false;
>
> bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs);
> @@ -370,7 +382,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);
> @@ -532,7 +544,7 @@ virObjectIsClass(void *anyobj,
> virClassPtr klass)
> {
> virObjectPtr obj = anyobj;
> - if (!obj)
> + if (VIR_OBJECT_NOTVALID(obj))
> return false;
I really don't think these changes are a positive move.
If you have code that is passing in something that is not a valid object,
then silently doing nothing in virObjectRef / virObjectIsClass is not
going to make the code any more correct. In fact you're turning something
that could be an immediate crash (and thus easy to diagnose) into
something that could be silent bad behaviour, which is much harder to
diagnose, or cause a crash much further away from the original root bug.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[...] > > I really don't think these changes are a positive move. > > If you have code that is passing in something that is not a valid object, > then silently doing nothing in virObjectRef / virObjectIsClass is not > going to make the code any more correct. In fact you're turning something > that could be an immediate crash (and thus easy to diagnose) into > something that could be silent bad behaviour, which is much harder to > diagnose, or cause a crash much further away from the original root bug. > > Consider the longevity of the patch being on list - I cannot remember all the details, although the commit message does help a bit. I do recall looking at the code and thinking incorrect usage could lead down the road of bad things. For Ref/Unref all that has been checked is !obj and we Incr/Decr a location. For Lock/Unlock as described in my 1/7 response class checking is/was added. Adding in object validity for Ref/Unref at least avoids rogue corruption which is really hard to diagnose in favor of leaving an entrail. Even without the additional check, the @obj someone may have thought they were incr/decr the refcnt isn't going to happen. Still, it just seems better in the event that we don't have a real @obj to at least message that in the hopes that someone trips across it. Similarly for Lock/Unlock same thing. IMO: The patches aren't making it harder to diagnose a problem - they're helping and they're not altering the value of some field for a valid @obj address. But if that's not desired, then fine - at least attempt was made and the feedback has been provided. Tks - John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.