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

[Daniel P. Berrange]

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 at 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 :|