[libvirt] [PATCH 1/5] API: add VIR_TYPED_PARAM_STRING

Stefan Berger stefanb at linux.vnet.ibm.com
Tue Nov 8 12:34:50 UTC 2011


On 11/08/2011 06:00 AM, Hu Tao wrote:

Hi Hu,

   previously I had looked at v6. I suppose this is v7. I'll ACK what I 
had previously ack'ed in v6.
> From: Eric Blake<eblake at redhat.com>
>
> 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 will be enforced at
>        the RPC layer in the next patch, so that drivers need not
>        worry about it in general.  The one exception is that
>        virDomainGetSchedulerParameters lacks a flags argument, so
>        it must not return a string; drivers that forward that
>        function on to virDomainGetSchedulerParametersFlags will
>        have to pay attention to the flag.
>      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 |   32 ++++++++++++++-
>   src/libvirt.c                |   92 ++++++++++++++++++++++++++++++++++++------
>   src/libvirt_internal.h       |    9 +++-
>   src/libvirt_private.syms     |    1 +
>   src/util/util.c              |   14 ++++++
>   src/util/util.h              |    2 +
>   6 files changed, 134 insertions(+), 16 deletions(-)
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index aa320b6..2ab89f5 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -200,11 +200,14 @@ 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.  */
>       VIR_DOMAIN_AFFECT_LIVE    = 1<<  0, /* Affect running domain state.  */
>       VIR_DOMAIN_AFFECT_CONFIG  = 1<<  1, /* Affect persistent domain state.  */
> +    /* 1<<  2 is reserved for virTypedParameterFlags */
>   } virDomainModificationImpact;
>
>   /**
> @@ -489,10 +492,36 @@ 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 {
> +    /* 1<<  0 is reserved for virDomainModificationImpact */
> +    /* 1<<  1 is reserved for virDomainModificationImpact */
> +
> +    /* 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 +549,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..1518ed2 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -3583,6 +3583,50 @@ 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;
> +}
> +
>   /**
>    * virDomainSetMemoryParameters:
>    * @domain: pointer to domain object
> @@ -3621,6 +3665,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 +3691,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 +3742,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) {
> @@ -3717,7 +3767,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 +3799,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 +3825,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 +3867,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) {
> @@ -6410,7 +6466,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 +6512,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) {
> @@ -6505,15 +6564,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 +6629,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 +6728,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 +6778,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) {
> 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 959c224..9ecfa9d 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -2607,3 +2607,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 d8176a8..3295ce8 100644
> --- a/src/util/util.h
> +++ b/src/util/util.h
> @@ -258,4 +258,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__ */
ACK




More information about the libvir-list mailing list