Re: [libvirt] [PATCH v2] esx: Extend esxVI_CURL_Download for partial downloads
2012/7/8 Doug Goldstein <cardoe(a)gentoo.org>:
On Sun, Jul 8, 2012 at 5:44 AM, Matthias Bolte
<matthias.bolte(a)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