Re: [PATCHv6 1/5] API: add VIR_TYPED_PARAM_STRING

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

 



On 11/03/2011 02:05 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 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@xxxxxxxxxx>
---

v6: address review comments from Stefan, defer filtering of string
params on Get functions to the rpc side

  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 7181f62..e10a72b 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 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__ */

ACK

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]