From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52301) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xb9Sp-0005LK-T9 for qemu-devel@nongnu.org; Mon, 06 Oct 2014 10:40:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xb9Sl-0000wh-66 for qemu-devel@nongnu.org; Mon, 06 Oct 2014 10:40:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31028) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xb9Sk-0000wO-U0 for qemu-devel@nongnu.org; Mon, 06 Oct 2014 10:40:35 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s96EeYgo007656 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 6 Oct 2014 10:40:34 -0400 Date: Mon, 6 Oct 2014 15:40:30 +0100 From: "Richard W.M. Jones" Message-ID: <20141006144030.GN1349@redhat.com> References: <1412605930-3397-1-git-send-email-rjones@redhat.com> <5432A983.9040101@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5432A983.9040101@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] block/curl: Improve type safety of s->timeout. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com 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). > >=20 > > 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. > >=20 > > Example error message after this change has been applied: > >=20 > > $ ./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":"htt= ps", "file.url":"https://foo/bar", "file.timeout":-1 }': timeout paramete= r is too large or negative: Invalid argument > >=20 > > Signed-off-by: Richard W.M. Jones > > --- > > block/curl.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > >=20 > > 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(BlockDriverStat= e *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->time= out); > > curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, > > (void *)curl_read_cb); > > curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)sta= te); > > @@ -546,6 +546,10 @@ static int curl_open(BlockDriverState *bs, QDict= *options, int flags, > > =20 > > s->timeout =3D 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; > > + } > > =20 > > s->sslverify =3D qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIF= Y, true); > > =20 > >=20 >=20 > 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 se= conds that you allow the libcurl transfer operation to take. Norm= ally, name lookups can take a considerable time and limiting o= pera=E2=80=90 tions to less than a few minutes risk aborting perfectly n= ormal operations. This option will cause curl to use the SIGALR= M to enable time-outing system calls. In unix-like systems, this might cause signals to be used u= nless CURLOPT_NOSIGNAL is set. Default timeout is 0 (zero) which means it never times out. Rich. --=20 Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rj= ones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html