qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract li


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options
Date: Fri, 24 Mar 2017 09:25:49 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)

Markus Armbruster <address@hidden> writes:

> Kevin Wolf <address@hidden> writes:
>
>> Am 23.03.2017 um 11:55 hat Markus Armbruster geschrieben:
>>> We have two list-values options:
>>> 
>>> * "server" is a list of InetSocketAddress.  We use members "host" and
>>>   "port", and silently ignore the rest.
>>> 
>>> * "auth-supported" is a list of RbdAuthMethod.  We use its only member
>>>   "auth".
>>> 
>>> Since qemu_rbd_open() takes options as a flattened QDict, options has
>>> keys of the form server.%d.host, server.%d.port and
>>> auth-supported.%d.auth, where %d counts up from zero.
>>> 
>>> qemu_rbd_array_opts() extracts these values as follows.  First, it
>>> calls qdict_array_entries() to find the list's length.  For each list
>>> element, it first formats the list's key prefix (e.g. "server.0."),
>>> then creates a new QDict holding the options with that key prefix,
>>> then converts that to a QemuOpts, so it can finally get the member
>>> values from there.
>>> 
>>> If there's one surefire way to make code using QDict more awkward,
>>> it's creating more of them and mixing in QemuOpts for good measure.
>>> 
>>> The conversion to QemuOpts abuses runtime_opts, as described in the
>>> commit before previous.
>>> 
>>> Rewrite to simply get the values straight from the options QDict.
>>> This removes the abuse of runtime_opts, so clean it up.
>>> 
>>> Signed-off-by: Markus Armbruster <address@hidden>
>>
>>> @@ -577,91 +557,59 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>>>      qemu_aio_unref(acb);
>>>  }
>>>  
>>> -#define RBD_MON_HOST          0
>>> -#define RBD_AUTH_SUPPORTED    1
>>> -
>>> -static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int 
>>> type,
>>> -                                 Error **errp)
>>> +static char *rbd_auth(QDict *options)
>>>  {
>>> -    int num_entries;
>>> -    QemuOpts *opts = NULL;
>>> -    QDict *sub_options;
>>> -    const char *host;
>>> -    const char *port;
>>> -    char *str;
>>> -    char *rados_str = NULL;
>>> -    Error *local_err = NULL;
>>> +    const char **vals = g_new(const char *, qdict_size(options));
>>> +    char keybuf[32];
>>> +    QObject *val;
>>> +    char *rados_str;
>>>      int i;
>>>  
>>> -    assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
>>> -
>>> -    num_entries = qdict_array_entries(options, prefix);
>>> +    for (i = 0;; i++) {
>>> +        sprintf(keybuf, "auth-supported.%d.auth", i);
>>> +        val = qdict_get(options, keybuf);
>>> +        if (!val) {
>>> +            break;
>>> +        }
>>>  
>>> -    if (num_entries < 0) {
>>> -        error_setg(errp, "Parse error on RBD QDict array");
>>> -        return NULL;
>>> +        vals[i] = qstring_get_str(qobject_to_qstring(val));
>>>      }
>>> +    vals[i] = NULL;
>>
>> In case of doubt, i is one more than vals can hold. (It segfaulted for
>> me when options was empty because I passed only options that are removed
>> before this function is called.)
>
> Yes, the g_new() above needs one extra slot.
>
>> You also want to remove the options from the QDict, otherwise
>> bdrv_open_inherit() will complain that the options are unknown.
>
> Okay.
>
>>>  
>>> -    for (i = 0; i < num_entries; i++) {
>>> -        char *strbuf = NULL;
>>> -        const char *value;
>>> -        char *rados_str_tmp;
>>> -
>>> -        str = g_strdup_printf("%s%d.", prefix, i);
>>> -        qdict_extract_subqdict(options, &sub_options, str);
>>> -        g_free(str);
>>> -
>>> -        opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>>> -        qemu_opts_absorb_qdict(opts, sub_options, &local_err);
>>> -        QDECREF(sub_options);
>>> -        if (local_err) {
>>> -            error_propagate(errp, local_err);
>>> -            g_free(rados_str);
>>> -            rados_str = NULL;
>>> -            goto exit;
>>> -        }
>>> +    rados_str = g_strjoinv(";", (char **)vals);
>>> +    g_free(vals);
>>> +    return rados_str;
>>> +}
>>>  
>>> -        if (type == RBD_MON_HOST) {
>>> -            host = qemu_opt_get(opts, "host");
>>> -            port = qemu_opt_get(opts, "port");
>>> +static char *rbd_mon_host(QDict *options)
>>> +{
>>> +    const char **vals = g_new(const char *, qdict_size(options));
>>> +    char keybuf[32];
>>> +    QObject *val;
>>> +    const char *host, *port;
>>> +    char *rados_str;
>>> +    int i;
>>>  
>>> -            value = host;
>>> -            if (port) {
>>> -                /* check for ipv6 */
>>> -                if (strchr(host, ':')) {
>>> -                    strbuf = g_strdup_printf("[%s]:%s", host, port);
>>> -                } else {
>>> -                    strbuf = g_strdup_printf("%s:%s", host, port);
>>> -                }
>>> -                value = strbuf;
>>> -            } else if (strchr(host, ':')) {
>>> -                strbuf = g_strdup_printf("[%s]", host);
>>> -                value = strbuf;
>>> -            }
>>> -        } else {
>>> -            value = qemu_opt_get(opts, "auth");
>>> +    for (i = 0;; i++) {
>>> +        sprintf(keybuf, "server.%d.host", i);
>>> +        val = qdict_get(options, keybuf);
>>> +        if (!val) {
>>> +            break;
>>>          }
>>> +        host = qstring_get_str(qobject_to_qstring(val));
>>> +        sprintf(keybuf, "server.%d.port", i);
>>> +        port = qdict_get_str(options, keybuf);
>>
>> This segfaults if the port option isn't given.
>
> @port is mandatory in BlockdevOptionsRbd.  If it's not there here, the
> options must have bypassed QAPI.  That would be bad news.  Can you
> explain how it can happen?

Answering myself, please correct mistakes.

There are two ways to create @options:

1. -blockdev and blockdev-add

   These create @options with a QAPI visitor from the command line
   option argument or QMP arguments, respectively.  This checks them
   against the QAPI schema.  Missing @port is rejected.

2. -drive and drive_add

   These appear to create @options manually, without checking against
   the QAPI schema.

   Crash reproducer: -drive driver=rbd,server.0.host=s0

In other words, we have *two* specifications for @options: the QAPI
schema, and the union of all the QemuOptsList that apply.  In case 1, we
check against both (I think).  In case 2, we only check against the
latter.

I understand how we got into this state, but it's not a good state to be
in.  We need to have our options defined in one way and one way only.

For 2.9, we cope with missing @port.

Post 2.9, we should either finish the QAPIfication of block
configuration we started with blockdev-add, or back it out, i.e. make
the QAPI schema accept anything, and rely on the QemuOpts-based
checking.

I want us to finish QAPIfication.

>>> -
>>> -        /* each iteration in the for loop will build upon the string, and 
>>> if
>>> -         * rados_str is NULL then it is our first pass */
>>> -        if (rados_str) {
>>> -            /* separate options with ';', as that  is what rados_conf_set()
>>> -             * requires */
>>> -            rados_str_tmp = rados_str;
>>> -            rados_str = g_strdup_printf("%s;%s", rados_str_tmp, value);
>>> -            g_free(rados_str_tmp);
>>> +        if (strchr(host, ':')) {
>>> +            vals[i] = g_strdup_printf("[%s]:%s", host, port);
>>>          } else {
>>> -            rados_str = g_strdup(value);
>>> +            vals[i] = g_strdup_printf("%s:%s", host, port);
>>>          }
>>> -
>>> -        g_free(strbuf);
>>> -        qemu_opts_del(opts);
>>> -        opts = NULL;
>>>      }
>>> +    vals[i] = NULL;
>>
>> Probably the same buffer overflow as above (but I didn't test that this
>> one really segfaults).
>
> Yes, same off-by-one.
>
>>> -exit:
>>> -    qemu_opts_del(opts);
>>> +    rados_str = g_strjoinv(";", (char **)vals);
>>> +    g_strfreev((char **)vals);
>>>      return rados_str;
>>>  }
>>>  
>>> @@ -685,24 +633,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
>>> *options, int flags,
>>>          return -EINVAL;
>>>      }
>>>  
>>> -    auth_supported = qemu_rbd_array_opts(options, "auth-supported.",
>>> -                                         RBD_AUTH_SUPPORTED, &local_err);
>>> -    if (local_err) {
>>> -        error_propagate(errp, local_err);
>>> -        r = -EINVAL;
>>> -        goto failed_opts;
>>> -    }
>>> -
>>> -    mon_host = qemu_rbd_array_opts(options, "server.",
>>> -                                   RBD_MON_HOST, &local_err);
>>> -    if (local_err) {
>>> -        error_propagate(errp, local_err);
>>> -        r = -EINVAL;
>>> -        goto failed_opts;
>>> -    }
>>> -
>>> +    auth_supported = rbd_auth(options);
>>> +    mon_host = rbd_mon_host(options);
>>>      secretid = qemu_opt_get(opts, "password-secret");
>>
>> Of course, this also changes the behaviour so that additional options in
>> server.* and auth-supported.* aren't silently ignored any more, but we
>> complain that they are unknown. I consider this a bonus bug fix, but it
>> should probably be spelt out in the commit message.
>
> Good point.

Note to self: this applies to -drive / drive_add, but not to -blockdev /
blockdev_add, because the QAPI schema kicks in there.  Example:
-drive driver=rbd,server.0.host=s0,server.0.port=p0,server.0.foo=bar



reply via email to

[Prev in Thread] Current Thread [Next in Thread]