Re: [Qemu-block] [PATCH for-2.9] rbd: Fix regression in legacy key/values containing escaped :

[Jeff Cody]

On Fri, Mar 31, 2017 at 10:27:30AM -0500, Eric Blake wrote:
> Commit c7cacb3 accidentally broke legacy key-value parsing through
> pseudo-filename parsing of -drive file=rbd://..., for any key that
> contains an escaped ':'.  Such a key is surprisingly common, thanks
> to mon_host specifying a 'host:port' string.  The break happens
> because passing things from QDict through QemuOpts back to another
> QDict requires that we pack our parsed key/value pairs into a string,
> and then reparse that string, but the intermediate string that we
> created ("key1=value1:key2=value2") lost the \: escaping that was
> present in the original, so that we could no longer see which : were
> used as separators vs. those used as part of the original input.
> 
> Fix it by collecting the key/value pairs through a QList, and
> sending that list on a round trip through a JSON QString (as in
> '["key1","value1","key2","value2"]') on its way through QemuOpts,
> rather than hand-rolling our own string.  Since the string is only
> handled internally, this was faster than creating a full-blown
> struct of '[{"key1":"value1"},{"key2":"value2"}]', and safer at
> guaranteeing order compared to '{"key1":"value1","key2":"value2"}'.
> 
> It would be nicer if we didn't have to round-trip through QemuOpts
> in the first place, but that's a much bigger task for later.
> 
> Reproducer:
> ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio \
> -drive 'file=rbd:volumes/volume-ea141b5c-cdb3-4765-910d-e7008b209a70'\
> ':id=compute:key=AQAVkvxXAAAAABAA9ZxWFYdRmV+DSwKr7BKKXg=='\
> ':auth_supported=cephx\;none:mon_host=192.168.1.2\:6789'\
> ',format=raw,if=none,id=drive-virtio-disk0,'\
> 'serial=ea141b5c-cdb3-4765-910d-e7008b209a70,cache=writeback'
> 
> Even without an RBD setup, this serves a test of whether we get
> the incorrect parser error of:
> qemu-system-x86_64: -drive file=rbd:...cache=writeback: conf option 6789 has 
> no value
> or the correct behavior of hanging while trying to connect to
> the requested mon_host of 192.168.1.2:6789.
> 
> Reported-by: Alexandru Avadanii <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  block/rbd.c | 83 
> +++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 42 insertions(+), 41 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 498322b..fbdb131 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -20,6 +20,7 @@
>  #include "crypto/secret.h"
>  #include "qemu/cutils.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qapi/qmp/qjson.h"
> 
>  /*
>   * When specifying the image filename use:
> @@ -135,18 +136,16 @@ static void qemu_rbd_parse_filename(const char 
> *filename, QDict *options,
>                                      Error **errp)
>  {
>      const char *start;
> -    char *p, *buf, *keypairs;
> +    char *p, *buf;
> +    QList *keypairs = NULL;
>      char *found_str;
> -    size_t max_keypair_size;
> 
>      if (!strstart(filename, "rbd:", &start)) {
>          error_setg(errp, "File name must start with 'rbd:'");
>          return;
>      }
> 
> -    max_keypair_size = strlen(start) + 1;
>      buf = g_strdup(start);
> -    keypairs = g_malloc0(max_keypair_size);
>      p = buf;
> 
>      found_str = qemu_rbd_next_tok(p, '/', &p);
> @@ -194,33 +193,30 @@ static void qemu_rbd_parse_filename(const char 
> *filename, QDict *options,
>          } else if (!strcmp(name, "id")) {
>              qdict_put(options, "user" , qstring_from_str(value));
>          } else {
> -            /* FIXME: This is pretty ugly, and not the right way to do this.
> -             *        These should be contained in a structure, and then
> -             *        passed explicitly as individual key/value pairs to
> -             *        rados.  Consider this legacy code that needs to be
> -             *        updated. */
> -            char *tmp = g_malloc0(max_keypair_size);
> -            /* only use a delimiter if it is not the first keypair found */
> -            /* These are sets of unknown key/value pairs we'll pass along
> -             * to ceph */
> -            if (keypairs[0]) {
> -                snprintf(tmp, max_keypair_size, ":%s=%s", name, value);
> -                pstrcat(keypairs, max_keypair_size, tmp);
> -            } else {
> -                snprintf(keypairs, max_keypair_size, "%s=%s", name, value);
> +            /*
> +             * We pass these internally to qemu_rbd_set_keypairs(), so
> +             * we can get away with the simpler list of [ "key1",
> +             * "value1", "key2", "value2" ] rather than a raw dict
> +             * { "key1": "value1", "key2": "value2" } where we can't
> +             * guarantee order, or even a more correct but complex
> +             * [ { "key1": "value1" }, { "key2": "value2" } ]
> +             */
> +            if (!keypairs) {
> +                keypairs = qlist_new();
>              }
> -            g_free(tmp);
> +            qlist_append(keypairs, qstring_from_str(name));
> +            qlist_append(keypairs, qstring_from_str(value));
>          }
>      }
> 
> -    if (keypairs[0]) {
> -        qdict_put(options, "=keyvalue-pairs", qstring_from_str(keypairs));
> +    if (keypairs) {
> +        qdict_put(options, "=keyvalue-pairs",
> +                  qobject_to_json(QOBJECT(keypairs)));
>      }
> 
> -
>  done:
>      g_free(buf);
> -    g_free(keypairs);
> +    QDECREF(keypairs);
>      return;
>  }
> 
> @@ -244,36 +240,41 @@ static int qemu_rbd_set_auth(rados_t cluster, const 
> char *secretid,
>      return 0;
>  }
> 
> -static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs,
> +static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
>                                   Error **errp)
>  {
> -    char *p, *buf;
> -    char *name;
> -    char *value;
> +    QList *keypairs;
> +    QString *name;
> +    QString *value;
> +    const char *key;
> +    size_t remaining;
>      int ret = 0;
> 
> -    buf = g_strdup(keypairs);
> -    p = buf;
> +    if (!keypairs_json) {
> +        return ret;
> +    }
> +    keypairs = qobject_to_qlist(qobject_from_json(keypairs_json,
> +                                                  &error_abort));
> +    remaining = qlist_size(keypairs) / 2;
> +    assert(remaining);
> 
> -    while (p) {
> -        name = qemu_rbd_next_tok(p, '=', &p);
> -        if (!p) {
> -            error_setg(errp, "conf option %s has no value", name);
> -            ret = -EINVAL;
> -            break;
> -        }
> +    while (remaining--) {
> +        name = qobject_to_qstring(qlist_pop(keypairs));
> +        value = qobject_to_qstring(qlist_pop(keypairs));
> +        assert(name && value);
> +        key = qstring_get_str(name);
> 
> -        value = qemu_rbd_next_tok(p, ':', &p);
> -
> -        ret = rados_conf_set(cluster, name, value);
> +        ret = rados_conf_set(cluster, key, qstring_get_str(value));
> +        QDECREF(name);
> +        QDECREF(value);
>          if (ret < 0) {
> -            error_setg_errno(errp, -ret, "invalid conf option %s", name);
> +            error_setg_errno(errp, -ret, "invalid conf option %s", key);
>              ret = -EINVAL;
>              break;
>          }
>      }
> 
> -    g_free(buf);
> +    QDECREF(keypairs);
>      return ret;
>  }
> 
> -- 
> 2.9.3
> 

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc.git block

-Jeff