[Qemu-devel] [PATCH] block/curl: Improve type safety of s->timeout.

[Qemu-devel] [PATCH] block/curl: Improve type safety of s->timeout.

[Richard W.M. Jones]

qemu_opt_get_number returns a uint64_t, and curl_easy_setopt expects a
long (not an int).

Store the timeout (which is a positive number of seconds) as a
uint64_t.  Check that the number given by the user is reasonable.
Cast it to long before calling curl_easy_setopt.

Example error message after this change has been applied:

$ ./qemu-img create -f qcow2 /tmp/test.qcow2 \
    -b 'json: { "file.driver":"https",
                "file.url":"https://foo/bar",
                "file.timeout":-1 }'
qemu-img: /tmp/test.qcow2: Could not open 'json: { "file.driver":"https", "file.url":"https://foo/bar", "file.timeout":-1 }': timeout parameter is too large or negative: Invalid argument

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 block/curl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 225407c..5233ff6 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -112,7 +112,7 @@ typedef struct BDRVCURLState {
     char *url;
     size_t readahead_size;
     bool sslverify;
-    int timeout;
+    uint64_t timeout;
     char *cookie;
     bool accept_range;
     AioContext *aio_context;
@@ -390,7 +390,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
         if (s->cookie) {
             curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie);
         }
-        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, s->timeout);
+        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout);
         curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
                          (void *)curl_read_cb);
         curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state);
@@ -546,6 +546,10 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->timeout = qemu_opt_get_number(opts, CURL_BLOCK_OPT_TIMEOUT,
                                      CURL_TIMEOUT_DEFAULT);
+    if (s->timeout > 100000) {
+        error_setg(errp, "timeout parameter is too large or negative");
+        goto out_noclean;
+    }
 
     s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
 
-- 
2.0.4

Re: [Qemu-devel] [PATCH] block/curl: Improve type safety of s->timeout.

[Laszlo Ersek]

On 10/06/14 16:32, Richard W.M. Jones wrote:
> qemu_opt_get_number returns a uint64_t, and curl_easy_setopt expects a
> long (not an int).
> 
> Store the timeout (which is a positive number of seconds) as a
> uint64_t.  Check that the number given by the user is reasonable.
> Cast it to long before calling curl_easy_setopt.
> 
> Example error message after this change has been applied:
> 
> $ ./qemu-img create -f qcow2 /tmp/test.qcow2 \
>     -b 'json: { "file.driver":"https",
>                 "file.url":"https://foo/bar",
>                 "file.timeout":-1 }'
> qemu-img: /tmp/test.qcow2: Could not open 'json: { "file.driver":"https", "file.url":"https://foo/bar", "file.timeout":-1 }': timeout parameter is too large or negative: Invalid argument
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  block/curl.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 225407c..5233ff6 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -112,7 +112,7 @@ typedef struct BDRVCURLState {
>      char *url;
>      size_t readahead_size;
>      bool sslverify;
> -    int timeout;
> +    uint64_t timeout;
>      char *cookie;
>      bool accept_range;
>      AioContext *aio_context;
> @@ -390,7 +390,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
>          if (s->cookie) {
>              curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie);
>          }
> -        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, s->timeout);
> +        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout);
>          curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
>                           (void *)curl_read_cb);
>          curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state);
> @@ -546,6 +546,10 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      s->timeout = qemu_opt_get_number(opts, CURL_BLOCK_OPT_TIMEOUT,
>                                       CURL_TIMEOUT_DEFAULT);
> +    if (s->timeout > 100000) {
> +        error_setg(errp, "timeout parameter is too large or negative");
> +        goto out_noclean;
> +    }
>  
>      s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
>  
> 

Since we're validating s->timeout -- is a zero value okay?

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] block/curl: Improve type safety of s->timeout.

[Richard W.M. Jones]

On Mon, Oct 06, 2014 at 04:38:59PM +0200, Laszlo Ersek wrote:
> On 10/06/14 16:32, Richard W.M. Jones wrote:
> > qemu_opt_get_number returns a uint64_t, and curl_easy_setopt expects a
> > long (not an int).
> > 
> > Store the timeout (which is a positive number of seconds) as a
> > uint64_t.  Check that the number given by the user is reasonable.
> > Cast it to long before calling curl_easy_setopt.
> > 
> > Example error message after this change has been applied:
> > 
> > $ ./qemu-img create -f qcow2 /tmp/test.qcow2 \
> >     -b 'json: { "file.driver":"https",
> >                 "file.url":"https://foo/bar",
> >                 "file.timeout":-1 }'
> > qemu-img: /tmp/test.qcow2: Could not open 'json: { "file.driver":"https", "file.url":"https://foo/bar", "file.timeout":-1 }': timeout parameter is too large or negative: Invalid argument
> > 
> > Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> > ---
> >  block/curl.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/curl.c b/block/curl.c
> > index 225407c..5233ff6 100644
> > --- a/block/curl.c
> > +++ b/block/curl.c
> > @@ -112,7 +112,7 @@ typedef struct BDRVCURLState {
> >      char *url;
> >      size_t readahead_size;
> >      bool sslverify;
> > -    int timeout;
> > +    uint64_t timeout;
> >      char *cookie;
> >      bool accept_range;
> >      AioContext *aio_context;
> > @@ -390,7 +390,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
> >          if (s->cookie) {
> >              curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie);
> >          }
> > -        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, s->timeout);
> > +        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout);
> >          curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
> >                           (void *)curl_read_cb);
> >          curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state);
> > @@ -546,6 +546,10 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
> >  
> >      s->timeout = qemu_opt_get_number(opts, CURL_BLOCK_OPT_TIMEOUT,
> >                                       CURL_TIMEOUT_DEFAULT);
> > +    if (s->timeout > 100000) {
> > +        error_setg(errp, "timeout parameter is too large or negative");
> > +        goto out_noclean;
> > +    }
> >  
> >      s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
> >  
> > 
> 
> Since we're validating s->timeout -- is a zero value okay?

Yes it's OK.  It means wait forever:

       CURLOPT_TIMEOUT
              Pass a long as parameter containing the maximum time in  seconds
              that you allow the libcurl transfer operation to take. Normally,
              name lookups can take a considerable time  and  limiting  opera‐
              tions  to less than a few minutes risk aborting perfectly normal
              operations. This option will cause curl to use  the  SIGALRM  to
              enable time-outing system calls.

              In unix-like systems, this might cause signals to be used unless
              CURLOPT_NOSIGNAL is set.

              Default timeout is 0 (zero) which means it never times out.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [Qemu-devel] [PATCH] block/curl: Improve type safety of s->timeout.

[Laszlo Ersek]

On 10/06/14 16:40, Richard W.M. Jones wrote:
> On Mon, Oct 06, 2014 at 04:38:59PM +0200, Laszlo Ersek wrote:
>> On 10/06/14 16:32, Richard W.M. Jones wrote:
>>> qemu_opt_get_number returns a uint64_t, and curl_easy_setopt expects a
>>> long (not an int).
>>>
>>> Store the timeout (which is a positive number of seconds) as a
>>> uint64_t.  Check that the number given by the user is reasonable.
>>> Cast it to long before calling curl_easy_setopt.
>>>
>>> Example error message after this change has been applied:
>>>
>>> $ ./qemu-img create -f qcow2 /tmp/test.qcow2 \
>>>     -b 'json: { "file.driver":"https",
>>>                 "file.url":"https://foo/bar",
>>>                 "file.timeout":-1 }'
>>> qemu-img: /tmp/test.qcow2: Could not open 'json: { "file.driver":"https", "file.url":"https://foo/bar", "file.timeout":-1 }': timeout parameter is too large or negative: Invalid argument
>>>
>>> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
>>> ---
>>>  block/curl.c | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/curl.c b/block/curl.c
>>> index 225407c..5233ff6 100644
>>> --- a/block/curl.c
>>> +++ b/block/curl.c
>>> @@ -112,7 +112,7 @@ typedef struct BDRVCURLState {
>>>      char *url;
>>>      size_t readahead_size;
>>>      bool sslverify;
>>> -    int timeout;
>>> +    uint64_t timeout;
>>>      char *cookie;
>>>      bool accept_range;
>>>      AioContext *aio_context;
>>> @@ -390,7 +390,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
>>>          if (s->cookie) {
>>>              curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie);
>>>          }
>>> -        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, s->timeout);
>>> +        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout);
>>>          curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
>>>                           (void *)curl_read_cb);
>>>          curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state);
>>> @@ -546,6 +546,10 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>>>  
>>>      s->timeout = qemu_opt_get_number(opts, CURL_BLOCK_OPT_TIMEOUT,
>>>                                       CURL_TIMEOUT_DEFAULT);
>>> +    if (s->timeout > 100000) {
>>> +        error_setg(errp, "timeout parameter is too large or negative");
>>> +        goto out_noclean;
>>> +    }
>>>  
>>>      s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
>>>  
>>>
>>
>> Since we're validating s->timeout -- is a zero value okay?
> 
> Yes it's OK.  It means wait forever:
> 
>        CURLOPT_TIMEOUT
>               Pass a long as parameter containing the maximum time in  seconds
>               that you allow the libcurl transfer operation to take. Normally,
>               name lookups can take a considerable time  and  limiting  opera‐
>               tions  to less than a few minutes risk aborting perfectly normal
>               operations. This option will cause curl to use  the  SIGALRM  to
>               enable time-outing system calls.
> 
>               In unix-like systems, this might cause signals to be used unless
>               CURLOPT_NOSIGNAL is set.
> 
>               Default timeout is 0 (zero) which means it never times out.
> 
> Rich.
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH] block/curl: Improve type safety of s->timeout.

[Stefan Hajnoczi]

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

On Mon, Oct 06, 2014 at 03:32:10PM +0100, Richard W.M. Jones wrote:
> qemu_opt_get_number returns a uint64_t, and curl_easy_setopt expects a
> long (not an int).
> 
> Store the timeout (which is a positive number of seconds) as a
> uint64_t.  Check that the number given by the user is reasonable.
> Cast it to long before calling curl_easy_setopt.
> 
> Example error message after this change has been applied:
> 
> $ ./qemu-img create -f qcow2 /tmp/test.qcow2 \
>     -b 'json: { "file.driver":"https",
>                 "file.url":"https://foo/bar",
>                 "file.timeout":-1 }'
> qemu-img: /tmp/test.qcow2: Could not open 'json: { "file.driver":"https", "file.url":"https://foo/bar", "file.timeout":-1 }': timeout parameter is too large or negative: Invalid argument
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  block/curl.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] block/curl: Improve type safety of s->timeout.

[Gonglei]

On 2014/10/28 22:29, Stefan Hajnoczi wrote:

> On Mon, Oct 06, 2014 at 03:32:10PM +0100, Richard W.M. Jones wrote:
>> qemu_opt_get_number returns a uint64_t, and curl_easy_setopt expects a
>> long (not an int).
>>
>> Store the timeout (which is a positive number of seconds) as a
>> uint64_t.  Check that the number given by the user is reasonable.
>> Cast it to long before calling curl_easy_setopt.
>>
>> Example error message after this change has been applied:
>>
>> $ ./qemu-img create -f qcow2 /tmp/test.qcow2 \
>>     -b 'json: { "file.driver":"https",
>>                 "file.url":"https://foo/bar",
>>                 "file.timeout":-1 }'
>> qemu-img: /tmp/test.qcow2: Could not open 'json: { "file.driver":"https", "file.url":"https://foo/bar", "file.timeout":-1 }': timeout parameter is too large or negative: Invalid argument
>>
>> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
>> ---
>>  block/curl.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> Thanks, applied to my block tree:
> https://github.com/stefanha/qemu/commits/block
> 
> Stefan

Please don't, the patch had been posted v2 which is better:

[PATCH v2] block/curl: Improve type safety of s->timeout.
http://lists.gnu.org/archive/html/qemu-devel/2014-10/msg03112.html

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH] block/curl: Improve type safety of s->timeout.

[Stefan Hajnoczi]

On Wed, Oct 29, 2014 at 2:08 AM, Gonglei <arei.gonglei@huawei.com> wrote:
> On 2014/10/28 22:29, Stefan Hajnoczi wrote:
>
>> On Mon, Oct 06, 2014 at 03:32:10PM +0100, Richard W.M. Jones wrote:
>>> qemu_opt_get_number returns a uint64_t, and curl_easy_setopt expects a
>>> long (not an int).
>>>
>>> Store the timeout (which is a positive number of seconds) as a
>>> uint64_t.  Check that the number given by the user is reasonable.
>>> Cast it to long before calling curl_easy_setopt.
>>>
>>> Example error message after this change has been applied:
>>>
>>> $ ./qemu-img create -f qcow2 /tmp/test.qcow2 \
>>>     -b 'json: { "file.driver":"https",
>>>                 "file.url":"https://foo/bar",
>>>                 "file.timeout":-1 }'
>>> qemu-img: /tmp/test.qcow2: Could not open 'json: { "file.driver":"https", "file.url":"https://foo/bar", "file.timeout":-1 }': timeout parameter is too large or negative: Invalid argument
>>>
>>> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
>>> ---
>>>  block/curl.c | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> Thanks, applied to my block tree:
>> https://github.com/stefanha/qemu/commits/block
>>
>> Stefan
>
> Please don't, the patch had been posted v2 which is better:
>
> [PATCH v2] block/curl: Improve type safety of s->timeout.
> http://lists.gnu.org/archive/html/qemu-devel/2014-10/msg03112.html

Sorry, I missed that thread because the cover letter has no 0/N
numbering as required in:
http://qemu-project.org/Contribute/SubmitAPatch

Numbering is a hard requirement so tools know how many emails to wait
for before processing emails.

The "patches" tool (https://github.com/stefanha/patches/) rejects
series that have no numbering.  Single patch emails without a cover
letter or numbering are fine, too.

Stefan