From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53012) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xb9Vk-0006w3-77 for qemu-devel@nongnu.org; Mon, 06 Oct 2014 10:43:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xb9Vc-0002AZ-Nb for qemu-devel@nongnu.org; Mon, 06 Oct 2014 10:43:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32749) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xb9Vc-0002AR-F3 for qemu-devel@nongnu.org; Mon, 06 Oct 2014 10:43:32 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s96EhVTU009354 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 6 Oct 2014 10:43:31 -0400 Message-ID: <5432AA8F.70901@redhat.com> Date: Mon, 06 Oct 2014 16:43:27 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1412605930-3397-1-git-send-email-rjones@redhat.com> <5432A983.9040101@redhat.com> <20141006144030.GN1349@redhat.com> In-Reply-To: <20141006144030.GN1349@redhat.com> Content-Type: text/plain; charset=utf-8 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: "Richard W.M. Jones" Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com 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":"htt= ps", "file.url":"https://foo/bar", "file.timeout":-1 }': timeout paramete= r is too large or negative: Invalid argument >>> >>> Signed-off-by: Richard W.M. Jones >>> --- >>> 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(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 >>> >> >> Since we're validating s->timeout -- is a zero value okay? >=20 > Yes it's OK. It means wait forever: >=20 > CURLOPT_TIMEOUT > Pass a long as parameter containing the maximum time in = seconds > that you allow the libcurl transfer operation to take. No= rmally, > name lookups can take a considerable time and limiting = opera=E2=80=90 > tions to less than a few minutes risk aborting perfectly= normal > operations. This option will cause curl to use the SIGA= LRM to > enable time-outing system calls. >=20 > In unix-like systems, this might cause signals to be used= unless > CURLOPT_NOSIGNAL is set. >=20 > Default timeout is 0 (zero) which means it never times ou= t. >=20 > Rich. >=20 Reviewed-by: Laszlo Ersek