| Message ID | 1414312958-21967-2-git-send-email-rjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2014/10/26 16:42, 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> > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > --- > block/curl.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/block/curl.c b/block/curl.c > index b4157cc..2b40802 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) { why 100000? And it's a magic number. Best regards, -Gonglei > + 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); >
On Sun, Oct 26, 2014 at 04:57:46PM +0800, Gonglei wrote: > On 2014/10/26 16:42, 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> > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > --- > > block/curl.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/block/curl.c b/block/curl.c > > index b4157cc..2b40802 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) { > > why 100000? And it's a magic number. It's just there to stop unreasonable timeouts or negative numbers. 100000 s is 27 hours, and no webserver I know of would keep a connection open that long. Possibly not even the IP stack. What's the difference between defining a number at the top of the file to be used once, and placing it exactly where it is used? Except the former introduces long range dependencies into the code making it harder to read and more fragile when changed. Rich.
On 2014/10/26 18:22, Richard W.M. Jones wrote: > It's just there to stop unreasonable timeouts or negative numbers. > 100000 s is 27 hours, and no webserver I know of would keep a > connection open that long. Possibly not even the IP stack. > Yes, it is. But 26 hours is OK? I just think we should assure the timeout as reasonable range, absolutely 100000 is too big IMO. > What's the difference between defining a number at the top of the file > to be used once, and placing it exactly where it is used? Except the > former introduces long range dependencies into the code making it > harder to read and more fragile when changed. That's the purpose using macro. If this value is used only one place in the curl.c (or other c files) now and future, you are fine with it. :) Best regards, -Gonglei
On Sun, Oct 26, 2014 at 06:45:02PM +0800, Gonglei wrote: > On 2014/10/26 18:22, Richard W.M. Jones wrote: > > > It's just there to stop unreasonable timeouts or negative numbers. > > 100000 s is 27 hours, and no webserver I know of would keep a > > connection open that long. Possibly not even the IP stack. > > > > Yes, it is. But 26 hours is OK? I just think we should assure the timeout > as reasonable range, absolutely 100000 is too big IMO. > > > What's the difference between defining a number at the top of the file > > to be used once, and placing it exactly where it is used? Except the > > former introduces long range dependencies into the code making it > > harder to read and more fragile when changed. > > > That's the purpose using macro. If this value is used only one place in the > curl.c (or other c files) now and future, you are fine with it. :) I don't understand this part. Can you explain how you think a macro should be used? Thanks, Rich.
On 2014/10/26 18:48, Richard W.M. Jones wrote: > On Sun, Oct 26, 2014 at 06:45:02PM +0800, Gonglei wrote: >> On 2014/10/26 18:22, Richard W.M. Jones wrote: >> >>> It's just there to stop unreasonable timeouts or negative numbers. >>> 100000 s is 27 hours, and no webserver I know of would keep a >>> connection open that long. Possibly not even the IP stack. >>> >> >> Yes, it is. But 26 hours is OK? I just think we should assure the timeout >> as reasonable range, absolutely 100000 is too big IMO. >> >>> What's the difference between defining a number at the top of the file >>> to be used once, and placing it exactly where it is used? Except the >>> former introduces long range dependencies into the code making it >>> harder to read and more fragile when changed. >> >> >> That's the purpose using macro. If this value is used only one place in the >> curl.c (or other c files) now and future, you are fine with it. :) > > I don't understand this part. Can you explain how you think a macro > should be used? > Sorry for misapprehension. I mean that's the purpose using macro what your said: " Except the former introduces long range dependencies into the code making it harder to read and more fragile when changed." Best regards, -Gonglei
On Sun, Oct 26, 2014 at 06:55:21PM +0800, Gonglei wrote: > On 2014/10/26 18:48, Richard W.M. Jones wrote: > > > On Sun, Oct 26, 2014 at 06:45:02PM +0800, Gonglei wrote: > >> On 2014/10/26 18:22, Richard W.M. Jones wrote: > >> > >>> It's just there to stop unreasonable timeouts or negative numbers. > >>> 100000 s is 27 hours, and no webserver I know of would keep a > >>> connection open that long. Possibly not even the IP stack. > >>> > >> > >> Yes, it is. But 26 hours is OK? I just think we should assure the timeout > >> as reasonable range, absolutely 100000 is too big IMO. > >> > >>> What's the difference between defining a number at the top of the file > >>> to be used once, and placing it exactly where it is used? Except the > >>> former introduces long range dependencies into the code making it > >>> harder to read and more fragile when changed. > >> > >> > >> That's the purpose using macro. If this value is used only one place in the > >> curl.c (or other c files) now and future, you are fine with it. :) > > > > I don't understand this part. Can you explain how you think a macro > > should be used? > > > > Sorry for misapprehension. I mean that's the purpose using macro what your said: > " Except the former introduces long range dependencies into the code making it > harder to read and more fragile when changed." OK, so our coding style is about introducing fragility and long range dependencies. v2 coming up ... Rich.
diff --git a/block/curl.c b/block/curl.c index b4157cc..2b40802 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> Reviewed-by: Laszlo Ersek <lersek@redhat.com> --- block/curl.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)