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 <address@hidden>
> > ---
> >  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