* [Qemu-devel] [PATCH v2] block/curl: Improve type safety of s->timeout. @ 2014-10-26 11:05 Richard W.M. Jones 2014-10-26 11:05 ` Richard W.M. Jones 2014-10-29 9:58 ` Stefan Hajnoczi 0 siblings, 2 replies; 4+ messages in thread From: Richard W.M. Jones @ 2014-10-26 11:05 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, arei.gonglei, lersek, stefanha v2: - Define the maximum timeout in a macro. - Reduce the maximum timeout to 10000 s (instead of 100000 s). ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Qemu-devel] [PATCH v2] block/curl: Improve type safety of s->timeout. 2014-10-26 11:05 [Qemu-devel] [PATCH v2] block/curl: Improve type safety of s->timeout Richard W.M. Jones @ 2014-10-26 11:05 ` Richard W.M. Jones 2014-10-26 11:15 ` Gonglei 2014-10-29 9:58 ` Stefan Hajnoczi 1 sibling, 1 reply; 4+ messages in thread From: Richard W.M. Jones @ 2014-10-26 11:05 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, arei.gonglei, lersek, stefanha qemu_opt_get_number returns a uint64_t, and curl_easy_setopt expects a long (not an int). There is no warning about the latter type error because curl_easy_setopt uses a varargs argument. Store the timeout (which is a positive number of seconds) as a uint64_t. Check that the number given by the user is reasonable. Zero is permissible (meaning no timeout is enforced by cURL). Cast it to long before calling curl_easy_setopt to fix the type error. 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> Reviewed-by: Laszlo Ersek <lersek@redhat.com> --- block/curl.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/block/curl.c b/block/curl.c index b4157cc..bbee3ca 100644 --- a/block/curl.c +++ b/block/curl.c @@ -64,6 +64,7 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle, #define SECTOR_SIZE 512 #define READ_AHEAD_DEFAULT (256 * 1024) #define CURL_TIMEOUT_DEFAULT 5 +#define CURL_TIMEOUT_MAX 10000 #define FIND_RET_NONE 0 #define FIND_RET_OK 1 @@ -112,7 +113,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 +391,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 +547,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 > CURL_TIMEOUT_MAX) { + 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); -- 2.0.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block/curl: Improve type safety of s->timeout. 2014-10-26 11:05 ` Richard W.M. Jones @ 2014-10-26 11:15 ` Gonglei 0 siblings, 0 replies; 4+ messages in thread From: Gonglei @ 2014-10-26 11:15 UTC (permalink / raw) To: Richard W.M. Jones Cc: kwolf@redhat.com, lersek@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On 2014/10/26 19:05, Richard W.M. Jones wrote: > qemu_opt_get_number returns a uint64_t, and curl_easy_setopt expects a > long (not an int). There is no warning about the latter type error > because curl_easy_setopt uses a varargs argument. > > Store the timeout (which is a positive number of seconds) as a > uint64_t. Check that the number given by the user is reasonable. > Zero is permissible (meaning no timeout is enforced by cURL). > > Cast it to long before calling curl_easy_setopt to fix the type error. > > 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> > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > --- You can add change logs in this place instead of a cover-letter for one patch :) > block/curl.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > Reviewed-by: Gonglei <arei.gonglei@huawei.com> Best regards, -Gonglei > diff --git a/block/curl.c b/block/curl.c > index b4157cc..bbee3ca 100644 > --- a/block/curl.c > +++ b/block/curl.c > @@ -64,6 +64,7 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle, > #define SECTOR_SIZE 512 > #define READ_AHEAD_DEFAULT (256 * 1024) > #define CURL_TIMEOUT_DEFAULT 5 > +#define CURL_TIMEOUT_MAX 10000 > > #define FIND_RET_NONE 0 > #define FIND_RET_OK 1 > @@ -112,7 +113,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 +391,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 +547,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 > CURL_TIMEOUT_MAX) { > + 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); > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block/curl: Improve type safety of s->timeout. 2014-10-26 11:05 [Qemu-devel] [PATCH v2] block/curl: Improve type safety of s->timeout Richard W.M. Jones 2014-10-26 11:05 ` Richard W.M. Jones @ 2014-10-29 9:58 ` Stefan Hajnoczi 1 sibling, 0 replies; 4+ messages in thread From: Stefan Hajnoczi @ 2014-10-29 9:58 UTC (permalink / raw) To: Richard W.M. Jones; +Cc: kwolf, arei.gonglei, lersek, qemu-devel, stefanha [-- Attachment #1: Type: text/plain, Size: 299 bytes --] On Sun, Oct 26, 2014 at 11:05:26AM +0000, Richard W.M. Jones wrote: > v2: > > - Define the maximum timeout in a macro. > > - Reduce the maximum timeout to 10000 s (instead of 100000 s). > > Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan [-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-10-29 9:59 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-26 11:05 [Qemu-devel] [PATCH v2] block/curl: Improve type safety of s->timeout Richard W.M. Jones 2014-10-26 11:05 ` Richard W.M. Jones 2014-10-26 11:15 ` Gonglei 2014-10-29 9:58 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).