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

[Michal Privoznik]

On 12/16/20 8:17 PM, Laine Stump wrote:
> 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 :-/)

Honestly, I did not bother exactly because of the reason you're 
mentioning: we have to support both.

> 
> 
>>   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 at redhat.com>
>> ---
>>   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 :-)

Thank you sir!

> 
> 
> Reviewed-by: Laine Stump <laine at redhat.com>


Michal