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

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&lt;eblake(a)redhat.com&gt; --- 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);

+ --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__ */