Re: [Qemu-devel] [PATCH 1/3] curl: Fix parsing of readahead option from filename

Re: [Qemu-devel] [PATCH 1/3] curl: Fix parsing of readahead option from filename

[Matthew Booth]

On 30/04/14 16:16, Kevin Wolf wrote:
> Am 30.04.2014 um 16:20 hat Matthew Booth geschrieben:
>> curl_parse_filename wasn't removing the option string from the url,
>> resulting in a 404.
>>
>> This change is a essentially a rewrite of that function as I also need
>> to extend it to handle more options. The rewrite is also much easier
>> to read.
>>
>> Signed-off-by: Matthew Booth <mbooth@redhat.com>
>> ---
>>  block/curl.c | 81 ++++++++++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 52 insertions(+), 29 deletions(-)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index d2f1084..4de6856 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -46,12 +46,15 @@
>>  #define CURL_NUM_STATES 8
>>  #define CURL_NUM_ACB    8
>>  #define SECTOR_SIZE     512
>> -#define READ_AHEAD_SIZE (256 * 1024)
>> +#define READ_AHEAD_DEFAULT (256 * 1024)
>>  
>>  #define FIND_RET_NONE   0
>>  #define FIND_RET_OK     1
>>  #define FIND_RET_WAIT   2
>>  
>> +#define CURL_BLOCK_OPT_URL       "url"
>> +#define CURL_BLOCK_OPT_READAHEAD "readahead"
>> +
>>  struct BDRVCURLState;
>>  
>>  typedef struct CURLAIOCB {
>> @@ -396,41 +399,60 @@ static void curl_clean_state(CURLState *s)
>>  static void curl_parse_filename(const char *filename, QDict *options,
>>                                  Error **errp)
>>  {
>> -
>> -    #define RA_OPTSTR ":readahead="
>>      char *file;
>> -    char *ra;
>> -    const char *ra_val;
>> -    int parse_state = 0;
>> +    char *end;
>>  
>>      file = g_strdup(filename);
>> +    end = file + strlen(file) - 1;
>> +
>> +    /* Don't fail if we've been passed an empty string.
>> +     * Only examine strings with a trailing ':'
>> +     */
>> +    if (end >= file && *end == ':') {
>> +        /* We exit this loop when we don't find a recognised option immediately
>> +         * preceding the trailing ':' of the form ':<option>=<value>'
>> +         *
>> +         * If filename has a trailing ':' but no preceding options, we don't
>> +         * remove the trailing ':'.
>> +         */
>> +        for (;;) {
>> +            /* Look for the preceding colon */
>> +            char *colon = memrchr(file, ':', end - file);
>> +            if (NULL == colon) {
>> +                break;
>> +            }
>>  
>> -    /* Parse a trailing ":readahead=#:" param, if present. */
>> -    ra = file + strlen(file) - 1;
>> -    while (ra >= file) {
>> -        if (parse_state == 0) {
>> -            if (*ra == ':') {
>> -                parse_state++;
>> -            } else {
>> +            char *opt_start = colon + 1;
>> +
>> +            /* Look for an equals sign */
>> +            char *equals = memchr(opt_start, '=', end - opt_start);
>> +            if (NULL == equals) {
>>                  break;
>>              }
>> -        } else if (parse_state == 1) {
>> -            if (*ra > '9' || *ra < '0') {
>> -                char *opt_start = ra - strlen(RA_OPTSTR) + 1;
>> -                if (opt_start > file &&
>> -                    strncmp(opt_start, RA_OPTSTR, strlen(RA_OPTSTR)) == 0) {
>> -                    ra_val = ra + 1;
>> -                    ra -= strlen(RA_OPTSTR) - 1;
>> -                    *ra = '\0';
>> -                    qdict_put(options, "readahead", qstring_from_str(ra_val));
>> -                }
>> +
>> +            size_t opt_len = equals - opt_start;
>> +            char *value = equals + 1;
>> +            size_t value_len = end - value;
>> +
>> +            if (opt_len == strlen(CURL_BLOCK_OPT_READAHEAD) &&
>> +                memcmp(opt_start, CURL_BLOCK_OPT_READAHEAD, opt_len) == 0) {
>> +                /* This is redundant after the first iteration */
>> +                *end = '\0';
>> +                qdict_put(options, CURL_BLOCK_OPT_READAHEAD,
>> +                          qstring_from_str(value));
>> +            } else {
>> +                /* Unknown option */
>>                  break;
> 
> So if we get an unknown option, we simply abort parsing the filename.
> This means that we'll try to open a URL that still contains an option
> and will probably fail with a rather confusing error message.
> 
> Shouldn't we set a clear error message about the unknown option here
> with error_setg() and immediately return?

TBH I was just trying to stay compatible with the behaviour which most
recently worked. Hence the weirdness with the trailing ':', for example.

I did consider whether to do ignore these options or not. I decided to
ignore them because the option syntax isn't safe: the string
':readahead=1k:' could be found at the end of a valid URL. Ignoring
unknown options reduces the chances of a false positive. For example, in:

http://example.com/path?query=foo:

How do you avoid throwing an error about the unknown option
'//example.com/path?query'? I could probably chuck in a couple of
heuristics to avoid the obvious false positives, but it would be even
messier. The best option is not to do this at all, and use the separated
options command line syntax. In fact, I might add a note to the docs
advising this.

Alternatively I could completely change the syntax. However, the only
'safe' syntax I can think of would involve ugly custom escaping, which
would probably catch out more people than unsafe option parsing. I'm
open to suggestions.

On a related note, do you know if it's possible to specify a backing
file with separated options? i.e.:

qemu-img create -f qcow2 -o
backingfile.url='http://example.com/path',backingfile.readahead='64k'
/tmp/foo.qcow2

I suspect that qcow2 only stores a string, so probably not, but I
thought I'd ask.

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

Re: [Qemu-devel] [PATCH 1/3] curl: Fix parsing of readahead option from filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

On 05/01/2014 02:56 AM, Matthew Booth wrote:
> On 30/04/14 16:16, Kevin Wolf wrote:
>> Am 30.04.2014 um 16:20 hat Matthew Booth geschrieben:
>>> curl_parse_filename wasn't removing the option string from the url,
>>> resulting in a 404.
>>>


> 
> Alternatively I could completely change the syntax. However, the only
> 'safe' syntax I can think of would involve ugly custom escaping, which
> would probably catch out more people than unsafe option parsing. I'm
> open to suggestions.

The name is a URI, so it should already be possible to use URI escaping
(percent-hex-hex) for any character that must be interpreted as part of
the filename rather than as a bogus query.  Therefore, you should be
strict and require that the user passes in a valid URI with all options
known and with the filename spelled correctly using URI escapes, rather
than loose and parsing what would otherwise be garbage as a possible
weird filename.

> 
> On a related note, do you know if it's possible to specify a backing
> file with separated options? i.e.:
> 
> qemu-img create -f qcow2 -o
> backingfile.url='http://example.com/path',backingfile.readahead='64k'
> /tmp/foo.qcow2

It should be possible; in fact, if the backing file is supported in
BlockdevOptions for the QMP blockdev-add command it already is.  If it
is not supported in BlockdevOptions, then we need structured options
added to make it possible.  But you can use any of the backing stores
that are already structured as your example.

> 
> I suspect that qcow2 only stores a string, so probably not, but I
> thought I'd ask.

There's a proposal for a new json: protocol, which allows storing ANY
arbitrary BlockdevOptions as a flat string in the qcow2 metadata (within
the limits of the header format which forces you to 1024 bytes or less).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/3] curl: Fix parsing of readahead option from filename

[Kevin Wolf]

Am 01.05.2014 um 10:56 hat Matthew Booth geschrieben:
> On 30/04/14 16:16, Kevin Wolf wrote:
> > Am 30.04.2014 um 16:20 hat Matthew Booth geschrieben:
> >> curl_parse_filename wasn't removing the option string from the url,
> >> resulting in a 404.
> >>
> >> This change is a essentially a rewrite of that function as I also need
> >> to extend it to handle more options. The rewrite is also much easier
> >> to read.
> >>
> >> Signed-off-by: Matthew Booth <mbooth@redhat.com>
> >> ---
> >>  block/curl.c | 81 ++++++++++++++++++++++++++++++++++++++----------------------
> >>  1 file changed, 52 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/block/curl.c b/block/curl.c
> >> index d2f1084..4de6856 100644
> >> --- a/block/curl.c
> >> +++ b/block/curl.c
> >> @@ -46,12 +46,15 @@
> >>  #define CURL_NUM_STATES 8
> >>  #define CURL_NUM_ACB    8
> >>  #define SECTOR_SIZE     512
> >> -#define READ_AHEAD_SIZE (256 * 1024)
> >> +#define READ_AHEAD_DEFAULT (256 * 1024)
> >>  
> >>  #define FIND_RET_NONE   0
> >>  #define FIND_RET_OK     1
> >>  #define FIND_RET_WAIT   2
> >>  
> >> +#define CURL_BLOCK_OPT_URL       "url"
> >> +#define CURL_BLOCK_OPT_READAHEAD "readahead"
> >> +
> >>  struct BDRVCURLState;
> >>  
> >>  typedef struct CURLAIOCB {
> >> @@ -396,41 +399,60 @@ static void curl_clean_state(CURLState *s)
> >>  static void curl_parse_filename(const char *filename, QDict *options,
> >>                                  Error **errp)
> >>  {
> >> -
> >> -    #define RA_OPTSTR ":readahead="
> >>      char *file;
> >> -    char *ra;
> >> -    const char *ra_val;
> >> -    int parse_state = 0;
> >> +    char *end;
> >>  
> >>      file = g_strdup(filename);
> >> +    end = file + strlen(file) - 1;
> >> +
> >> +    /* Don't fail if we've been passed an empty string.
> >> +     * Only examine strings with a trailing ':'
> >> +     */
> >> +    if (end >= file && *end == ':') {
> >> +        /* We exit this loop when we don't find a recognised option immediately
> >> +         * preceding the trailing ':' of the form ':<option>=<value>'
> >> +         *
> >> +         * If filename has a trailing ':' but no preceding options, we don't
> >> +         * remove the trailing ':'.
> >> +         */
> >> +        for (;;) {
> >> +            /* Look for the preceding colon */
> >> +            char *colon = memrchr(file, ':', end - file);
> >> +            if (NULL == colon) {
> >> +                break;
> >> +            }
> >>  
> >> -    /* Parse a trailing ":readahead=#:" param, if present. */
> >> -    ra = file + strlen(file) - 1;
> >> -    while (ra >= file) {
> >> -        if (parse_state == 0) {
> >> -            if (*ra == ':') {
> >> -                parse_state++;
> >> -            } else {
> >> +            char *opt_start = colon + 1;
> >> +
> >> +            /* Look for an equals sign */
> >> +            char *equals = memchr(opt_start, '=', end - opt_start);
> >> +            if (NULL == equals) {
> >>                  break;
> >>              }
> >> -        } else if (parse_state == 1) {
> >> -            if (*ra > '9' || *ra < '0') {
> >> -                char *opt_start = ra - strlen(RA_OPTSTR) + 1;
> >> -                if (opt_start > file &&
> >> -                    strncmp(opt_start, RA_OPTSTR, strlen(RA_OPTSTR)) == 0) {
> >> -                    ra_val = ra + 1;
> >> -                    ra -= strlen(RA_OPTSTR) - 1;
> >> -                    *ra = '\0';
> >> -                    qdict_put(options, "readahead", qstring_from_str(ra_val));
> >> -                }
> >> +
> >> +            size_t opt_len = equals - opt_start;
> >> +            char *value = equals + 1;
> >> +            size_t value_len = end - value;
> >> +
> >> +            if (opt_len == strlen(CURL_BLOCK_OPT_READAHEAD) &&
> >> +                memcmp(opt_start, CURL_BLOCK_OPT_READAHEAD, opt_len) == 0) {
> >> +                /* This is redundant after the first iteration */
> >> +                *end = '\0';
> >> +                qdict_put(options, CURL_BLOCK_OPT_READAHEAD,
> >> +                          qstring_from_str(value));
> >> +            } else {
> >> +                /* Unknown option */
> >>                  break;
> > 
> > So if we get an unknown option, we simply abort parsing the filename.
> > This means that we'll try to open a URL that still contains an option
> > and will probably fail with a rather confusing error message.
> > 
> > Shouldn't we set a clear error message about the unknown option here
> > with error_setg() and immediately return?
> 
> TBH I was just trying to stay compatible with the behaviour which most
> recently worked. Hence the weirdness with the trailing ':', for example.
> 
> I did consider whether to do ignore these options or not. I decided to
> ignore them because the option syntax isn't safe: the string
> ':readahead=1k:' could be found at the end of a valid URL. Ignoring
> unknown options reduces the chances of a false positive. For example, in:
> 
> http://example.com/path?query=foo:
> 
> How do you avoid throwing an error about the unknown option
> '//example.com/path?query'?

You don't? :-)

It's the same thing for local file names. If you want a safe way for
specifying files that have things like colons in their name, you
shouldn't use the filename string, but specific options that are taken
literally (like file.filename=foo:bar.qcow2), otherwise you get an error
about unknown protocols.

And Eric made the very good point that colons should be percent-encoded
anyway in a URL, so I really wouldn't worry too much about this. There
are enough ways for the user to achieve what they want, we don't have
to provide everything in the shortcut string.

>I could probably chuck in a couple of
> heuristics to avoid the obvious false positives, but it would be even
> messier. The best option is not to do this at all, and use the separated
> options command line syntax. In fact, I might add a note to the docs
> advising this.
> 
> Alternatively I could completely change the syntax. However, the only
> 'safe' syntax I can think of would involve ugly custom escaping, which
> would probably catch out more people than unsafe option parsing. I'm
> open to suggestions.

Let's not go there. We already have a safe syntax, and it's the
separated options.

> On a related note, do you know if it's possible to specify a backing
> file with separated options? i.e.:
> 
> qemu-img create -f qcow2 -o
> backingfile.url='http://example.com/path',backingfile.readahead='64k'
> /tmp/foo.qcow2
> 
> I suspect that qcow2 only stores a string, so probably not, but I
> thought I'd ask.

Like Eric said, the json: protocol is how we want to implement this.

If you're okay with specifying the option at runtime, you can already do
that with an option like this:

    -drive file=/tmp/foo.qcow2,backing.file.readahead=64k

Kevin

Re: [Qemu-devel] [PATCH 2/3] curl: Add sslverify option

[Matthew Booth]

On 30/04/14 16:32, Kevin Wolf wrote:
> Am 30.04.2014 um 16:20 hat Matthew Booth geschrieben:
>> This allows qemu to use images over https with a self-signed certificate. It
>> defaults to verifying the certificate.
>>
>> Signed-off-by: Matthew Booth <mbooth@redhat.com>
>> ---
>>  block/curl.c | 33 +++++++++++++++++++++++++++++++++
>>  1 file changed, 33 insertions(+)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index 4de6856..e427e52 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -23,6 +23,7 @@
>>   */
>>  #include "qemu-common.h"
>>  #include "block/block_int.h"
>> +#include "qapi/qmp/qbool.h"
>>  #include <curl/curl.h>
>>  
>>  // #define DEBUG
>> @@ -54,6 +55,7 @@
>>  
>>  #define CURL_BLOCK_OPT_URL       "url"
>>  #define CURL_BLOCK_OPT_READAHEAD "readahead"
>> +#define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
>>  
>>  struct BDRVCURLState;
>>  
>> @@ -91,6 +93,7 @@ typedef struct BDRVCURLState {
>>      CURLState states[CURL_NUM_STATES];
>>      char *url;
>>      size_t readahead_size;
>> +    bool sslverify;
>>      bool accept_range;
>>  } BDRVCURLState;
>>  
>> @@ -357,6 +360,7 @@ static CURLState *curl_init_state(BDRVCURLState *s)
>>              return NULL;
>>          }
>>          curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
>> +        curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER, s->sslverify);
>>          curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, 5);
>>          curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
>>                           (void *)curl_read_cb);
>> @@ -440,6 +444,27 @@ static void curl_parse_filename(const char *filename, QDict *options,
>>                  *end = '\0';
>>                  qdict_put(options, CURL_BLOCK_OPT_READAHEAD,
>>                            qstring_from_str(value));
>> +            } else if (opt_len == strlen(CURL_BLOCK_OPT_SSLVERIFY) &&
>> +                       memcmp(opt_start, CURL_BLOCK_OPT_SSLVERIFY,
>> +                       opt_len) == 0) {
>> +                /* This is redundant after the first iteration */
>> +                *end = '\0';
>> +
>> +                int sslverify;
>> +                if (value_len == strlen("on") &&
>> +                    memcmp(value, "on", value_len) == 0) {
>> +                    sslverify = 1;
>> +                } else if (value_len == strlen("off") &&
>> +                         memcmp(value, "off", value_len) == 0) {
> 
> Indentation is off here.

Thanks. I'll repost after we've come to a decision about the option syntax.

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490