[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]