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