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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
