[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(¶ms[i],¶ms[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(¶ms[*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