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 <rjo...@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 <ler...@redhat.com>
