[libvirt] [PATCHv5 1/3] API: add VIR_TYPED_PARAM_STRING

Stefan Berger stefanb at linux.vnet.ibm.com
Wed Nov 2 10:49:47 UTC 2011


On 11/01/2011 07:47 PM, Eric Blake wrote:
> This allows strings to be transported between client and server
> in the context of name-type-value virTypedParameter functions.
> For compatibility,
>
>      o new clients will not send strings to old servers, based on
>        a feature check
>      o new servers will not send strings to old clients without the
>        flag VIR_TYPED_PARAM_STRING_OKAY; this is enforced by
>        libvirt.c, so that drivers need not worry about it
>      o the flag VIR_TYPED_PARAM_STRING_OKAY is set automatically,
>        based on a feature check (so far, no driver implements it),
>        so clients do not have to worry about it
>
> Future patches can then enable the feature on a per-driver basis.
>
> This patch also ensures that drivers can blindly strdup() field
> names (previously, a malicious client could stuff 80 non-NUL bytes
> into field and cause a read overrun).
>
> * src/libvirt_internal.h (VIR_DRV_FEATURE_TYPED_PARAM_STRING): New
> driver feature.
> * src/libvirt.c (virTypedParameterValidateSet)
> (virTypedParameterSanitizeGet): New helper functions.
> (virDomainSetMemoryParameters, virDomainSetBlkioParameters)
> (virDomainSetSchedulerParameters)
> (virDomainSetSchedulerParametersFlags)
> (virDomainGetMemoryParameters, virDomainGetBlkioParameters)
> (virDomainGetSchedulerParameters)
> (virDomainGetSchedulerParametersFlags, virDomainBlockStatsFlags):
> Use them.
> * src/util/util.h (virTypedParameterArrayClear): New helper
> function.
> * src/util/util.c (virTypedParameterArrayClear): Implement it.
> * src/libvirt_private.syms (util.h): Export it.
> Based on an initial patch by Hu Tao, with feedback from
> Daniel P. Berrange.
>
> Signed-off-by: Eric Blake<eblake at redhat.com>
> ---
>   include/libvirt/libvirt.h.in |   28 ++++++++++-
>   src/libvirt.c                |  119 +++++++++++++++++++++++++++++++++++++-----
>   src/libvirt_internal.h       |    9 +++-
>   src/libvirt_private.syms     |    1 +
>   src/util/util.c              |   14 +++++
>   src/util/util.h              |    2 +
>   6 files changed, 156 insertions(+), 17 deletions(-)
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 0840d46..c737e72 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -200,6 +200,8 @@ typedef virDomainControlInfo *virDomainControlInfoPtr;
>    * current domain state. VIR_DOMAIN_AFFECT_LIVE requires a running
>    * domain, and VIR_DOMAIN_AFFECT_CONFIG requires a persistent domain
>    * (whether or not it is running).
> + *
> + * These enums should not conflict with those of virTypedParameterFlags.
>    */
>   typedef enum {
>       VIR_DOMAIN_AFFECT_CURRENT = 0,      /* Affect current domain state.  */
> @@ -489,10 +491,33 @@ typedef enum {
>       VIR_TYPED_PARAM_LLONG   = 3, /* long long case */
>       VIR_TYPED_PARAM_ULLONG  = 4, /* unsigned long long case */
>       VIR_TYPED_PARAM_DOUBLE  = 5, /* double case */
> -    VIR_TYPED_PARAM_BOOLEAN = 6  /* boolean(character) case */
> +    VIR_TYPED_PARAM_BOOLEAN = 6, /* boolean(character) case */
> +    VIR_TYPED_PARAM_STRING  = 7, /* string case */
>   } virTypedParameterType;
>
>   /**
> + * virTypedParameterFlags:
> + *
> + * Flags related to libvirt APIs that use virTypedParameter.
> + *
> + * These enums should not conflict with those of virDomainModificationImpact.
> + */
> +typedef enum {
> +    /* Older servers lacked the ability to handle string typed
> +     * parameters.  Attempts to set a string parameter with an older
> +     * server will fail at the client, but attempts to retrieve
> +     * parameters must not return strings from a new server to an
> +     * older client, so this flag exists to identify newer clients to
> +     * newer servers.  This flag is automatically set when needed, so
> +     * the user does not have to worry about it; however, manually
> +     * setting the flag can be used to reject servers that cannot
> +     * return typed strings, even if no strings would be returned.
> +     */
> +    VIR_TYPED_PARAM_STRING_OKAY = 1<<  2,
> +
> +} virTypedParameterFlags;
> +
> +/**
>    * VIR_TYPED_PARAM_FIELD_LENGTH:
>    *
>    * Macro providing the field length of virTypedParameter name
> @@ -520,6 +545,7 @@ struct _virTypedParameter {
>           unsigned long long int ul;  /* type is ULLONG */
>           double d;                   /* type is DOUBLE */
>           char b;                     /* type is BOOLEAN */
> +        char *s;                    /* type is STRING, may not be NULL */
>       } value; /* parameter value */
>   };
>
> diff --git a/src/libvirt.c b/src/libvirt.c
> index b0d1e01..5041d88 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -3583,6 +3583,71 @@ error:
>       return -1;
>   }
>
> +/* Helper function called to validate incoming client array on any
> + * interface that sets typed parameters in the hypervisor.  */
> +static int
> +virTypedParameterValidateSet(virDomainPtr domain,
> +                             virTypedParameterPtr params,
> +                             int nparams)
> +{
> +    bool string_okay;
> +    int i;
> +
> +    string_okay = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver,
> +                                           domain->conn,
> +                                           VIR_DRV_FEATURE_TYPED_PARAM_STRING);
> +    for (i = 0; i<  nparams; i++) {
> +        if (strnlen(params[i].field, VIR_TYPED_PARAM_FIELD_LENGTH) ==
> +            VIR_TYPED_PARAM_FIELD_LENGTH) {
> +            virLibDomainError(VIR_ERR_INVALID_ARG,
> +                              _("string parameter name '%.*s' too long"),
> +                              VIR_TYPED_PARAM_FIELD_LENGTH,
> +                              params[i].field);
> +            virDispatchError(NULL);
> +            return -1;
> +        }
> +        if (params[i].type == VIR_TYPED_PARAM_STRING) {
> +            if (string_okay) {
> +                if (!params[i].value.s) {
> +                    virLibDomainError(VIR_ERR_INVALID_ARG,
> +                                      _("NULL string parameter '%s'"),
> +                                      params[i].field);
> +                    virDispatchError(NULL);
> +                    return -1;
> +                }
> +            } else {
> +                virLibDomainError(VIR_ERR_INVALID_ARG,
> +                                  _("string parameter '%s' unsupported"),
> +                                  params[i].field);
> +                virDispatchError(NULL);
> +                return -1;
> +            }
> +        }
> +    }
> +    return 0;
> +}
> +
> +/* Helper function called to sanitize outgoing hypervisor array on any
> + * interface that gets typed parameters for the client.  */
> +static void
> +virTypedParameterSanitizeGet(virTypedParameterPtr params,
> +                             int *nparams,
> +                             unsigned int flags)
> +{
> +    if (params&&  !(flags&  VIR_TYPED_PARAM_STRING_OKAY)) {
> +        int i;
> +        for (i = 0; i<  *nparams; i++) {
> +            if (params[i].type == VIR_TYPED_PARAM_STRING) {
> +                VIR_FREE(params[i].value.s);
> +                memmove(&params[i],&params[i + 1], *nparams - i + 1);
*nparams = 5
i = 0

-> 5-0+1 = 6 -> you would move too many elements (but not enough bytes)

It should be '(*nparams - i - 1) * sizeof(*params)' !

> +                --i;
> +                --*nparams;
> +                memset(&params[*nparams], 0, sizeof(*params));
> +            }
> +        }
> +    }
> +}
> +
>   /**
>    * virDomainSetMemoryParameters:
>    * @domain: pointer to domain object
> @@ -3621,6 +3686,9 @@ virDomainSetMemoryParameters(virDomainPtr domain,
>           virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>           goto error;
>       }
> +    if (virTypedParameterValidateSet(domain, params, nparams)<  0)
> +        return -1;
> +
>       conn = domain->conn;
>
>       if (conn->driver->domainSetMemoryParameters) {
> @@ -3644,7 +3712,7 @@ error:
>    * @params: pointer to memory parameter object
>    *          (return value, allocated by the caller)
>    * @nparams: pointer to number of memory parameters; input and output
> - * @flags: one of virDomainModificationImpact
> + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags
>    *
>    * Get all memory parameters.  On input, @nparams gives the size of the
>    * @params array; on output, @nparams gives how many slots were filled
> @@ -3695,6 +3763,9 @@ virDomainGetMemoryParameters(virDomainPtr domain,
>           virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>           goto error;
>       }
> +    if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> +                                 VIR_DRV_FEATURE_TYPED_PARAM_STRING))
> +        flags |= VIR_TYPED_PARAM_STRING_OKAY;
>       conn = domain->conn;
>
>       if (conn->driver->domainGetMemoryParameters) {
> @@ -3702,6 +3773,7 @@ virDomainGetMemoryParameters(virDomainPtr domain,
>           ret = conn->driver->domainGetMemoryParameters (domain, params, nparams, flags);
>           if (ret<  0)
>               goto error;
> +        virTypedParameterSanitizeGet(params, nparams, flags);
>           return ret;
>       }
>       virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> @@ -3717,7 +3789,7 @@ error:
>    * @params: pointer to blkio parameter objects
>    * @nparams: number of blkio parameters (this value can be the same or
>    *          less than the number of parameters supported)
> - * @flags: an OR'ed set of virDomainModificationImpact
> + * @flags: bitwise-OR of virDomainModificationImpact
>    *
>    * Change all or a subset of the blkio tunables.
>    * This function may require privileged access to the hypervisor.
> @@ -3749,6 +3821,9 @@ virDomainSetBlkioParameters(virDomainPtr domain,
>           virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>           goto error;
>       }
> +    if (virTypedParameterValidateSet(domain, params, nparams)<  0)
> +        return -1;
> +
>       conn = domain->conn;
>
>       if (conn->driver->domainSetBlkioParameters) {
> @@ -3772,7 +3847,7 @@ error:
>    * @params: pointer to blkio parameter object
>    *          (return value, allocated by the caller)
>    * @nparams: pointer to number of blkio parameters; input and output
> - * @flags: an OR'ed set of virDomainModificationImpact
> + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags
>    *
>    * Get all blkio parameters.  On input, @nparams gives the size of the
>    * @params array; on output, @nparams gives how many slots were filled
> @@ -3814,6 +3889,9 @@ virDomainGetBlkioParameters(virDomainPtr domain,
>           virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>           goto error;
>       }
> +    if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> +                                 VIR_DRV_FEATURE_TYPED_PARAM_STRING))
> +        flags |= VIR_TYPED_PARAM_STRING_OKAY;
>       conn = domain->conn;
>
>       if (conn->driver->domainGetBlkioParameters) {
> @@ -3821,6 +3899,7 @@ virDomainGetBlkioParameters(virDomainPtr domain,
>           ret = conn->driver->domainGetBlkioParameters (domain, params, nparams, flags);
>           if (ret<  0)
>               goto error;
> +        virTypedParameterSanitizeGet(params, nparams, flags);
>           return ret;
>       }
>       virLibConnError (VIR_ERR_NO_SUPPORT, __FUNCTION__);
> @@ -6384,7 +6463,6 @@ virDomainGetSchedulerParameters(virDomainPtr domain,
>           virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>           goto error;
>       }
> -
>       conn = domain->conn;
>
>       if (conn->driver->domainGetSchedulerParameters) {
> @@ -6392,6 +6470,7 @@ virDomainGetSchedulerParameters(virDomainPtr domain,
>           ret = conn->driver->domainGetSchedulerParameters (domain, params, nparams);
>           if (ret<  0)
>               goto error;
> +        virTypedParameterSanitizeGet(params, nparams, 0);
>           return ret;
>       }
>
> @@ -6410,7 +6489,7 @@ error:
>    * @nparams: pointer to number of scheduler parameter
>    *          (this value should be same than the returned value
>    *           nparams of virDomainGetSchedulerType()); input and output
> - * @flags: one of virDomainModificationImpact
> + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags
>    *
>    * Get all scheduler parameters.  On input, @nparams gives the size of the
>    * @params array; on output, @nparams gives how many slots were filled
> @@ -6456,6 +6535,9 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain,
>           goto error;
>       }
>
> +    if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> +                                 VIR_DRV_FEATURE_TYPED_PARAM_STRING))
> +        flags |= VIR_TYPED_PARAM_STRING_OKAY;
>       conn = domain->conn;
>
>       if (conn->driver->domainGetSchedulerParametersFlags) {
> @@ -6464,6 +6546,7 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain,
>                                                                  nparams, flags);
>           if (ret<  0)
>               goto error;
> +        virTypedParameterSanitizeGet(params, nparams, flags);
>           return ret;
>       }
>
> @@ -6505,15 +6588,17 @@ virDomainSetSchedulerParameters(virDomainPtr domain,
>           return -1;
>       }
>
> +    if (domain->conn->flags&  VIR_CONNECT_RO) {
> +        virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> +        goto error;
> +    }
>       if (params == NULL || nparams<  0) {
>           virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>           goto error;
>       }
> +    if (virTypedParameterValidateSet(domain, params, nparams)<  0)
> +        return -1;
>
> -    if (domain->conn->flags&  VIR_CONNECT_RO) {
> -        virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> -        goto error;
> -    }
>       conn = domain->conn;
>
>       if (conn->driver->domainSetSchedulerParameters) {
> @@ -6568,15 +6653,17 @@ virDomainSetSchedulerParametersFlags(virDomainPtr domain,
>           return -1;
>       }
>
> +    if (domain->conn->flags&  VIR_CONNECT_RO) {
> +        virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> +        goto error;
> +    }
>       if (params == NULL || nparams<  0) {
>           virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>           goto error;
>       }
> +    if (virTypedParameterValidateSet(domain, params, nparams)<  0)
> +        return -1;
>
> -    if (domain->conn->flags&  VIR_CONNECT_RO) {
> -        virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> -        goto error;
> -    }
>       conn = domain->conn;
>
>       if (conn->driver->domainSetSchedulerParametersFlags) {
> @@ -6665,7 +6752,7 @@ error:
>    * @params: pointer to block stats parameter object
>    *          (return value)
>    * @nparams: pointer to number of block stats; input and output
> - * @flags: unused, always pass 0
> + * @flags: bitwise-OR of virTypedParameterFlags
>    *
>    * This function is to get block stats parameters for block
>    * devices attached to the domain.
> @@ -6715,6 +6802,9 @@ int virDomainBlockStatsFlags(virDomainPtr dom,
>           virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>           goto error;
>       }
> +    if (VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn,
> +                                 VIR_DRV_FEATURE_TYPED_PARAM_STRING))
> +        flags |= VIR_TYPED_PARAM_STRING_OKAY;
>       conn = dom->conn;
>
>       if (conn->driver->domainBlockStatsFlags) {
> @@ -6722,6 +6812,7 @@ int virDomainBlockStatsFlags(virDomainPtr dom,
>           ret = conn->driver->domainBlockStatsFlags(dom, path, params, nparams, flags);
>           if (ret<  0)
>               goto error;
> +        virTypedParameterSanitizeGet(params, nparams, flags);
>           return ret;
>       }
>       virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h
> index 0117c5b..2550d76 100644
> --- a/src/libvirt_internal.h
> +++ b/src/libvirt_internal.h
> @@ -1,7 +1,7 @@
>   /*
>    * libvirt.h: publically exported APIs, not for public use
>    *
> - * Copyright (C) 2006-2008 Red Hat, Inc.
> + * Copyright (C) 2006-2008, 2011 Red Hat, Inc.
>    *
>    * This library is free software; you can redistribute it and/or
>    * modify it under the terms of the GNU Lesser General Public
> @@ -83,7 +83,12 @@ enum {
>       /*
>        * Support for file descriptor passing
>        */
> -    VIR_DRV_FEATURE_FD_PASSING = 8
> +    VIR_DRV_FEATURE_FD_PASSING = 8,
> +
> +    /*
> +     * Support for VIR_TYPED_PARAM_STRING
> +     */
> +    VIR_DRV_FEATURE_TYPED_PARAM_STRING = 9,
>   };
>
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 6a1562e..2185294 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1162,6 +1162,7 @@ virStrncpy;
>   virTimeMs;
>   virTimestamp;
>   virTrimSpaces;
> +virTypedParameterArrayClear;
>   virVasprintf;
>
>
> diff --git a/src/util/util.c b/src/util/util.c
> index bd52b21..b25f8db 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -2572,3 +2572,17 @@ or other application using the libvirt API.\n\
>
>       return 0;
>   }
> +
> +void
> +virTypedParameterArrayClear(virTypedParameterPtr params, int nparams)
> +{
> +    int i;
> +
> +    if (!params)
> +        return;
> +
> +    for (i = 0; i<  nparams; i++) {
> +        if (params[i].type == VIR_TYPED_PARAM_STRING)
> +            VIR_FREE(params[i].value.s);
> +    }
> +}
> diff --git a/src/util/util.h b/src/util/util.h
> index e869f1b..e5594cb 100644
> --- a/src/util/util.h
> +++ b/src/util/util.h
> @@ -263,4 +263,6 @@ bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1);
>   int virEmitXMLWarning(int fd,
>                         const char *name,
>                         const char *cmd) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> +
> +void virTypedParameterArrayClear(virTypedParameterPtr params, int nparams);
>   #endif /* __VIR_UTIL_H__ */
Rest looks good. With nit fixed ACK.




More information about the libvir-list mailing list