[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