[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] esx: Extend esxVI_CURL_Download for partial downloads



On Sun, Jul 8, 2012 at 5:44 AM, Matthias Bolte
<matthias bolte googlemail com> wrote:
> Also ensure that the virBuffer used to store the downloaded data
> does not overflow.
> ---
>
> v2:
>  - Ensure that the used virBuffer dos not overflow.
>
>  src/esx/esx_driver.c |    2 +-
>  src/esx/esx_vi.c     |   62 +++++++++++++++++++++++++++++++++++++++++++------
>  src/esx/esx_vi.h     |    3 +-
>  3 files changed, 57 insertions(+), 10 deletions(-)
>
> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
> index db2144c..95b9286 100644
> --- a/src/esx/esx_driver.c
> +++ b/src/esx/esx_driver.c
> @@ -2802,7 +2802,7 @@ esxDomainGetXMLDesc(virDomainPtr domain, unsigned int flags)
>
>      url = virBufferContentAndReset(&buffer);
>
> -    if (esxVI_CURL_Download(priv->primary->curl, url, &vmx) < 0) {
> +    if (esxVI_CURL_Download(priv->primary->curl, url, &vmx, 0, NULL) < 0) {
>          goto cleanup;
>      }
>
> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
> index 48718b6..0769e8b 100644
> --- a/src/esx/esx_vi.c
> +++ b/src/esx/esx_vi.c
> @@ -116,9 +116,9 @@ ESX_VI__TEMPLATE__FREE(CURL,
>  })
>
>  static size_t
> -esxVI_CURL_ReadString(char *data, size_t size, size_t nmemb, void *ptrptr)
> +esxVI_CURL_ReadString(char *data, size_t size, size_t nmemb, void *userdata)
>  {
> -    const char *content = *(const char **)ptrptr;
> +    const char *content = *(const char **)userdata;
>      size_t available = 0;
>      size_t requested = size * nmemb;
>
> @@ -138,16 +138,28 @@ esxVI_CURL_ReadString(char *data, size_t size, size_t nmemb, void *ptrptr)
>
>      memcpy(data, content, requested);
>
> -    *(const char **)ptrptr = content + requested;
> +    *(const char **)userdata = content + requested;
>
>      return requested;
>  }
>
>  static size_t
> -esxVI_CURL_WriteBuffer(char *data, size_t size, size_t nmemb, void *buffer)
> +esxVI_CURL_WriteBuffer(char *data, size_t size, size_t nmemb, void *userdata)
>  {
> +    virBufferPtr buffer = userdata;
> +
>      if (buffer != NULL) {
> -        virBufferAdd((virBufferPtr) buffer, data, size * nmemb);
> +        /*
> +         * Using a virBuffer to store the download data limits the downloadable
> +         * size. This is no problem as esxVI_CURL_Download and esxVI_CURL_Perform
> +         * are meant to download small things such as VMX files, VMDK metadata
> +         * files and SOAP responses.
> +         */
> +        if (virBufferUse(buffer) > UINT32_MAX / 2) {
> +            return 0;
> +        }

This could never be true since the type would have already resulted in
an overflow and therefore would be less than UINT32_MAX / 2 (which is
equivalent to INT32_MAX).  Something like the below would ensure that
you are always within the bounds of your type :

if ((size * nmemb) <= (INT32_MAX - virBufferUse(buffer)) {
   virBufferAdd(...)
   return size * nmemb;
}

> +
> +        virBufferAdd(buffer, data, size * nmemb);
>
>          return size * nmemb;
>      }
> @@ -160,7 +172,7 @@ esxVI_CURL_WriteBuffer(char *data, size_t size, size_t nmemb, void *buffer)
>  #if ESX_VI__CURL__ENABLE_DEBUG_OUTPUT
>  static int
>  esxVI_CURL_Debug(CURL *curl ATTRIBUTE_UNUSED, curl_infotype type,
> -                 char *info, size_t size, void *data ATTRIBUTE_UNUSED)
> +                 char *info, size_t size, void *userdata ATTRIBUTE_UNUSED)
>  {
>      char *buffer = NULL;
>
> @@ -355,8 +367,10 @@ esxVI_CURL_Connect(esxVI_CURL *curl, esxUtil_ParsedUri *parsedUri)
>  }
>
>  int
> -esxVI_CURL_Download(esxVI_CURL *curl, const char *url, char **content)
> +esxVI_CURL_Download(esxVI_CURL *curl, const char *url, char **content,
> +                    unsigned long long offset, unsigned long long *length)
>  {
> +    char *range = NULL;
>      virBuffer buffer = VIR_BUFFER_INITIALIZER;
>      int responseCode = 0;
>
> @@ -365,9 +379,33 @@ esxVI_CURL_Download(esxVI_CURL *curl, const char *url, char **content)
>          return -1;
>      }
>
> +    if (length != NULL && *length > 0) {
> +        /*
> +         * Using a virBuffer to store the download data limits the downloadable
> +         * size. This is no problem as esxVI_CURL_Download is meant to download
> +         * small things such as VMX of VMDK metadata files.
> +         */
> +        if (*length > UINT32_MAX / 2) {

Use INT32_MAX

> +            ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, "%s",
> +                         _("Download length it too large"));
> +            return -1;
> +        }
> +
> +        if (virAsprintf(&range, "%llu-%llu", offset, offset + *length - 1) < 0) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +    } else if (offset > 0) {
> +        if (virAsprintf(&range, "%llu-", offset) < 0) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +    }
> +
>      virMutexLock(&curl->lock);
>
>      curl_easy_setopt(curl->handle, CURLOPT_URL, url);
> +    curl_easy_setopt(curl->handle, CURLOPT_RANGE, range);
>      curl_easy_setopt(curl->handle, CURLOPT_WRITEDATA, &buffer);
>      curl_easy_setopt(curl->handle, CURLOPT_UPLOAD, 0);
>      curl_easy_setopt(curl->handle, CURLOPT_HTTPGET, 1);
> @@ -378,7 +416,7 @@ esxVI_CURL_Download(esxVI_CURL *curl, const char *url, char **content)
>
>      if (responseCode < 0) {
>          goto cleanup;
> -    } else if (responseCode != 200) {
> +    } else if (responseCode != 200 && responseCode != 206) {
>          ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR,
>                       _("HTTP response code %d for download from '%s'"),
>                       responseCode, url);
> @@ -390,9 +428,15 @@ esxVI_CURL_Download(esxVI_CURL *curl, const char *url, char **content)
>          goto cleanup;
>      }
>
> +    if (length != NULL) {
> +        *length = virBufferUse(&buffer);
> +    }
> +
>      *content = virBufferContentAndReset(&buffer);
>
>    cleanup:
> +    VIR_FREE(range);
> +
>      if (*content == NULL) {
>          virBufferFreeAndReset(&buffer);
>          return -1;
> @@ -414,6 +458,7 @@ esxVI_CURL_Upload(esxVI_CURL *curl, const char *url, const char *content)
>      virMutexLock(&curl->lock);
>
>      curl_easy_setopt(curl->handle, CURLOPT_URL, url);
> +    curl_easy_setopt(curl->handle, CURLOPT_RANGE, NULL);
>      curl_easy_setopt(curl->handle, CURLOPT_READDATA, &content);
>      curl_easy_setopt(curl->handle, CURLOPT_UPLOAD, 1);
>      curl_easy_setopt(curl->handle, CURLOPT_INFILESIZE, strlen(content));
> @@ -1231,6 +1276,7 @@ esxVI_Context_Execute(esxVI_Context *ctx, const char *methodName,
>      virMutexLock(&ctx->curl->lock);
>
>      curl_easy_setopt(ctx->curl->handle, CURLOPT_URL, ctx->url);
> +    curl_easy_setopt(ctx->curl->handle, CURLOPT_RANGE, NULL);
>      curl_easy_setopt(ctx->curl->handle, CURLOPT_WRITEDATA, &buffer);
>      curl_easy_setopt(ctx->curl->handle, CURLOPT_UPLOAD, 0);
>      curl_easy_setopt(ctx->curl->handle, CURLOPT_POSTFIELDS, request);
> diff --git a/src/esx/esx_vi.h b/src/esx/esx_vi.h
> index 9560bd2..49b7ca2 100644
> --- a/src/esx/esx_vi.h
> +++ b/src/esx/esx_vi.h
> @@ -167,7 +167,8 @@ struct _esxVI_CURL {
>  int esxVI_CURL_Alloc(esxVI_CURL **curl);
>  void esxVI_CURL_Free(esxVI_CURL **curl);
>  int esxVI_CURL_Connect(esxVI_CURL *curl, esxUtil_ParsedUri *parsedUri);
> -int esxVI_CURL_Download(esxVI_CURL *curl, const char *url, char **content);
> +int esxVI_CURL_Download(esxVI_CURL *curl, const char *url, char **content,
> +                        unsigned long long offset, unsigned long long *length);
>  int esxVI_CURL_Upload(esxVI_CURL *curl, const char *url, const char *content);
>
>
> --
> 1.7.4.1

Just a 2 nits mentioned above.

-- 
Doug Goldstein


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]