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@redhat.com>
---
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;
};
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
I don't think that we need to guard this kind of programming error.
We've been able to cope with the current virObject code without hitting
this issue.
Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/30/2017 08:50 AM, Pavel Hrdina wrote:
> 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.
>
> I don't think that we need to guard this kind of programming error.
> We've been able to cope with the current virObject code without hitting
> this issue.
>
I don't disagree that we've been able to cope, but adding more objects
makes things more interesting and prone to having code trip across a bad
usage which we could have prevented. The base object classes make a lot
of assumptions and use opaque params, so having some sort of check
prevents those possibly very bad things happening. Not everyone is well
behaved and at times we do make assumptions. IIRC it took me a bit of
combing through what I'd changed to figure out my erroneous usage which
resulted in me adding this to make it very much so more obvious.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com>
> ---
> 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@redhat.com>
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/30/2017 09:00 AM, Daniel P. Berrange wrote:
> 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@redhat.com>
>> ---
>> 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@redhat.com>
> 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)
>
Oh right - just mask away the pesky counter portion... This works me
too. I can adjust.
Tks -
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 30, 2017 at 10:43:35AM -0400, John Ferlan wrote:
>
>
> On 05/30/2017 09:00 AM, Daniel P. Berrange wrote:
> > 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@redhat.com>
> >> ---
> >> 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@redhat.com>
> > 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)
> >
>
> Oh right - just mask away the pesky counter portion... This works me
> too. I can adjust.
Should also put an assert in the virClassNew method that the new magic is
<= 0xCAFEFFFF as a safety net for 20 years time when we finally have
65536 classes :-)
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
© 2016 - 2025 Red Hat, Inc.