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

[Matthias Bolte]

2012/7/8 Doug Goldstein <cardoe at gentoo.org>:
> On Sun, Jul 8, 2012 at 5:44 AM, Matthias Bolte
> <matthias.bolte at 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(-)

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

That's not true, virBufferUse returns unsigned int. Also virBuffer
stores it's size internally as unsigned int. I just looked it up
again. But then there is a bug in virBufferGrow that uses an
intermediate int variable to store the new size. This limits a
virBuffer to hold at most 2GB. Also virBuffer doesn't do any overflow
checks internally.

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

Therefore this isn't safe either. virBufferAdd can grow the buffer
beyond the required size, for INT32_MAX it'll probably overflow. I'll
use INT32_MAX / 2 as the limit here, because now I assume that a
virBuffer can safely hold at most INT32_MAX / 2. This is the same
pattern as with UINT32_MAX / 2 where I assumed a virBuffer would be
unsigned int clean in it's internals.

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

I'll use INT32_MAX / 2 for the reason explain above.

-- 
Matthias Bolte
http://photron.blogspot.com