Re: [Qemu-devel] [PATCH v2] block/curl: Improve type safety of s->timeout.

[Gonglei]

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 <address@hidden>
> Reviewed-by: Laszlo Ersek <address@hidden>
> ---

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 <address@hidden>

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);
>