2012/7/8 Doug Goldstein <cardoe@gentoo.org>:
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(-)
-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