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

[Pavel Hrdina]

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 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))

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170728/ca20ffe3/attachment-0001.sig>