| Message ID | 1412605930-3397-1-git-send-email-rjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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>
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
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
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
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);
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(-)