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

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.

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

+ + 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; }