Re: [PATCH 2/2] virnetdevopenvswitch: Try to unescape ovs-vsctl reply in one specific case

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

 



On 12/16/20 1:45 PM, Michal Privoznik wrote:
During testing of my patch v6.10.0-rc1~221 it was found that

   'ovs-vsctl get Interface $name name' or
   'ovs-vsctl find Interface options:vhost-server-path=$path'

may return a string in double quotes, e.g. "vhost-user1". Later
investigation of openvswitch code showed, that early versions
(like 1.3.0) have somewhat restrictive set of safe characters
(isalpha() || '_' || '-' || '.'), which is then refined with
increasing version. For instance, version 2.11.4 has: isalnum()
|| '_' || '-' || '.'. If the string that ovs-vsctl wants to
output contains any other character it is escaped. You want to be
looking at ovsdb_atom_to_string() which handles outputting of a
single string and calls string_needs_quotes() and possibly
json_serialize_string() in openvswitch code base.

Since the interfaces are usually named "vhost-userN" we are
facing a problem where with one version we get the name in double
quotes and with another we get plain name without funny business.

Because of json involved I thought, let's make ovs-vsctl output
into JSON format and then use our JSON parser, but guess what -
ovs-vsctl ignores --format=json.


Yuck. Is this a known/reported problem? Is it true with current ovs-vsctl, or just with old versions? (I guess in the end it doesn't really matter, because we have to support old and new anyway :-/)


  But with a little help of
g_strdup_printf() it can be turned into JSON.

Fixes: e4c29e2904197472919d050c67acfd59f0144bbc
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1767013
Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
  src/libvirt_private.syms         |  1 +
  src/util/virnetdevopenvswitch.c  | 47 +++++++++++++++++++++++++++++
  src/util/virnetdevopenvswitch.h  |  4 +++
  tests/virnetdevopenvswitchtest.c | 52 ++++++++++++++++++++++++++++++++
  4 files changed, 104 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c7c37d9689..583fc5800e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2676,6 +2676,7 @@ virNetDevOpenvswitchGetVhostuserIfname;
  virNetDevOpenvswitchInterfaceGetMaster;
  virNetDevOpenvswitchInterfaceParseStats;
  virNetDevOpenvswitchInterfaceStats;
+virNetDevOpenvswitchMaybeUnescapeReply;
  virNetDevOpenvswitchRemovePort;
  virNetDevOpenvswitchSetMigrateData;
  virNetDevOpenvswitchSetTimeout;
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index 7eabaa763d..14fa294ae1 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -460,6 +460,48 @@ virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
  }
+/**
+ * virNetDevOpenvswitchMaybeUnescapeReply:
+ * @reply: a string to unescape
+ *
+ * Depending on ovs-vsctl version a string might be escaped. For instance:
+ *  -version 2.11.4 allows only is_alpha(), an underscore, a dash or a dot,
+ *  -version 2.14.0 allows only is_alnum(), an underscore, a dash or a dot,
+ * any other character causes the string to be escaped.
+ *
+ * What this function does, is it checks whether @reply string consists solely
+ * from safe, not escaped characters (as defined by version 2.14.0) and if not
+ * an error is reported. If @reply is a string enclosed in double quotes, but
+ * otherwise safe those double quotes are removed.
+ *
+ * Returns: 0 on success,
+ *         -1 otherwise (with error reported).
+ */
+int
+virNetDevOpenvswitchMaybeUnescapeReply(char *reply)
+{
+    g_autoptr(virJSONValue) json = NULL;
+    g_autofree char *jsonStr = NULL;
+    const char *tmp = NULL;
+    size_t replyLen = strlen(reply);
+
+    if (*reply != '"')
+        return 0;
+
+    jsonStr = g_strdup_printf("{\"name\": %s}", reply);
+    if (!(json = virJSONValueFromString(jsonStr)))
+        return -1;
+
+    if (!(tmp = virJSONValueObjectGetString(json, "name"))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Malformed ovs-vsctl output"));
+        return -1;
+    }


Tricky!

There are other ways to unescape a string, but this one gets a vote for novelty :-)


Reviewed-by: Laine Stump <laine@xxxxxxxxxx>


+
+    return virStrcpy(reply, tmp, replyLen);
+}
+
+
  /**
   * virNetDevOpenvswitchGetVhostuserIfname:
   * @path: the path of the unix socket
@@ -522,6 +564,11 @@ virNetDevOpenvswitchGetVhostuserIfname(const char *path,
          return 0;
      }
+ if (virNetDevOpenvswitchMaybeUnescapeReply(*ifname) < 0) {
+        VIR_FREE(*ifname);
+        return -1;
+    }
+
      return 1;
  }
diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h
index 8409aa92ac..3571708582 100644
--- a/src/util/virnetdevopenvswitch.h
+++ b/src/util/virnetdevopenvswitch.h
@@ -60,6 +60,10 @@ int virNetDevOpenvswitchInterfaceStats(const char *ifname,
  int virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
      ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT;
+int
+virNetDevOpenvswitchMaybeUnescapeReply(char *reply)
+    ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT;
+
  int virNetDevOpenvswitchGetVhostuserIfname(const char *path,
                                             bool server,
                                             char **ifname)
diff --git a/tests/virnetdevopenvswitchtest.c b/tests/virnetdevopenvswitchtest.c
index fd47e927ea..46172dae90 100644
--- a/tests/virnetdevopenvswitchtest.c
+++ b/tests/virnetdevopenvswitchtest.c
@@ -75,6 +75,42 @@ testInterfaceParseStats(const void *opaque)
  }
+typedef struct _escapeData escapeData;
+struct _escapeData {
+    const char *input;
+    const char *expect;
+};
+
+
+static int
+testNameEscape(const void *opaque)
+{
+    const escapeData *data = opaque;
+    g_autofree char *reply = g_strdup(data->input);
+    int rv;
+
+    rv = virNetDevOpenvswitchMaybeUnescapeReply(reply);
+
+    if (data->expect) {
+        if (rv < 0 || STRNEQ(reply, data->expect)) {
+            fprintf(stderr,
+                    "Unexpected failure, expected: %s for input %s got %s\n",
+                    data->expect, data->input, reply);
+            return -1;
+        }
+    } else {
+        if (rv >= 0) {
+            fprintf(stderr,
+                    "Unexpected success, input %s got %s\n",
+                    data->input, reply);
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+
  static int
  mymain(void)
  {
@@ -94,6 +130,22 @@ mymain(void)
      TEST_INTERFACE_STATS("stats1.json", 9, 12, 11, 10, 2, 8, 5, 4);
      TEST_INTERFACE_STATS("stats2.json", 12406, 173, 0, 0, 0, 0, 0, 0);
+#define TEST_NAME_ESCAPE(str, fail) \
+    do { \
+        const escapeData data = {str, fail};\
+        if (virTestRun("Name escape " str, testNameEscape, &data) < 0) \
+            ret = -1; \
+    } while (0)
+
+    TEST_NAME_ESCAPE("", "");
+    TEST_NAME_ESCAPE("\"\"", "");
+    TEST_NAME_ESCAPE("vhost-user1", "vhost-user1");
+    TEST_NAME_ESCAPE("\"vhost-user1\"", "vhost-user1");
+    TEST_NAME_ESCAPE("\"vhost_user-name.to.escape1", NULL);
+    TEST_NAME_ESCAPE("\"vhost_user-name.to\\\"escape1\"", "vhost_user-name.to\"escape1");
+    TEST_NAME_ESCAPE("\"vhost\"user1\"", NULL);
+    TEST_NAME_ESCAPE("\"\\\\", NULL);
+
      return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
  }





[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]

  Powered by Linux