Re: [libvirt] [PATCH 7/7] util: Add safety net of checks to ensure valid object

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(a)redhat.com&gt; > --- > 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