[libvirt] [PATCH 0/2] Resolve Coverity issues in vbox_tmpl.c
This pair of patches resolves what amount to false positives in the vbox_tmpl.c code relating to attempts to dereference NULL in the 2002 VBOX_API_VERSION call to 'vboxIIDFromUUID()'. It also removes "false postives" from the various VBOX_UTF*_FREE() macros. Some code paths checked if the arg was valid prior to calling free. That triggered Coverity into tagging other uses of the variables where the check wasn't performed. While these changes resolve the Coverity issues there still exists a "problem" where the VBOX_UTF<n>_TO_UTF<m> macros don't check the return status from the called function nor does the code that uses the macros always check the returned arg2 for validity (125 occurrances). John Ferlan (2): vbox: Address false positive for NULL dereference vbox: Adjust the UTF FREE macros src/vbox/vbox_tmpl.c | 77 ++++++++++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 39 deletions(-) -- 1.7.11.7
Resolve a false positive from 'vboxIIDFromUUID_v2_x()'. The code sets 'iid->value = &iid->backing' unconditionally prior to calling 'nsIDFromChar()'. The 'vboxIIDUnalloc_v2_x()' checks iid->value to not be &iid->backing. The iid->backing is a static buffer within the initialized structure. --- src/vbox/vbox_tmpl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 2b3fa25..d2cd0b8 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -444,6 +444,7 @@ vboxIIDFromUUID_v2_x(vboxGlobalData *data, vboxIID_v2_x *iid, iid->value = &iid->backing; + sa_assert(iid->value); nsIDFromChar(iid->value, uuid); } -- 1.7.11.7
On 01/23/2013 05:34 PM, John Ferlan wrote:
Resolve a false positive from 'vboxIIDFromUUID_v2_x()'. The code sets 'iid->value = &iid->backing' unconditionally prior to calling 'nsIDFromChar()'. The 'vboxIIDUnalloc_v2_x()' checks iid->value to not be &iid->backing. The iid->backing is a static buffer within the initialized structure. --- src/vbox/vbox_tmpl.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 2b3fa25..d2cd0b8 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -444,6 +444,7 @@ vboxIIDFromUUID_v2_x(vboxGlobalData *data, vboxIID_v2_x *iid,
iid->value = &iid->backing;
+ sa_assert(iid->value);
I don't know why Coverity complained on this one (might be worth raising it as a bug), but as the comment is harmless, I see no reason to avoid it. ACK and pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/25/2013 06:02 PM, Eric Blake wrote:
On 01/23/2013 05:34 PM, John Ferlan wrote:
Resolve a false positive from 'vboxIIDFromUUID_v2_x()'. The code sets 'iid->value = &iid->backing' unconditionally prior to calling 'nsIDFromChar()'. The 'vboxIIDUnalloc_v2_x()' checks iid->value to not be &iid->backing. The iid->backing is a static buffer within the initialized structure. --- src/vbox/vbox_tmpl.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 2b3fa25..d2cd0b8 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -444,6 +444,7 @@ vboxIIDFromUUID_v2_x(vboxGlobalData *data, vboxIID_v2_x *iid,
iid->value = &iid->backing;
+ sa_assert(iid->value);
I don't know why Coverity complained on this one (might be worth raising it as a bug), but as the comment is harmless, I see no reason to avoid it.
ACK and pushed.
Thanks for the push - I agree that something doesn't seem right. I created some sample code and will pass it along to the coverity list to see what I get. John The rest of this is a few more details from the code - cut-n-paste from Coverity output. struct _vboxIID_v2_x { /* IID is represented by a pointer to a nsID. */ nsID *value; /* backing is used in cases where we need to create or copy an IID. * We cannot allocate memory that can be freed by ComUnallocMem. * Therefore, we use this stack allocated nsID instead. */ nsID backing; }; # define VBOX_IID_INITIALIZER { NULL, { 0, 0, 0, { 0, 0, 0, 0, 0, 0, 0, 0 } } } <Frame #1> 7631 static virNetworkPtr 7632 vboxNetworkLookupByUUID(virConnectPtr conn, const unsigned char *uuid) 7633 { (1) Event cond_false: Condition "!data->vboxObj", taking false branch (2) Event if_end: End of if statement (3) Event cond_false: Condition "!host", taking false branch (4) Event if_end: End of if statement 7634 VBOX_OBJECT_HOST_CHECK(conn, virNetworkPtr, NULL); (5) Event assign_zero: Assigning: "iid.value" = "NULL". Also see events: [var_deref_model] 7635 vboxIID iid = VBOX_IID_INITIALIZER; 7636 (6) Event var_deref_model: Passing "&iid" to function "vboxIIDFromUUID_v2_x(vboxGlobalData *, vboxIID_v2_x *, unsigned char const *)", which dereferences null "iid.value". [details] Also see events: [assign_zero] 7637 vboxIIDFromUUID(&iid, uuid); [notes] * At this point we have "iid->value = NULL" and "iid->backing" = { 0, 0, 0, { 0, 0, 0, 0, 0, 0, 0, 0 } } * Take away the initializer and there are no Coverity warnings * Call is a macro that expands to one of 3 functions, in this case: 490 # define vboxIIDFromUUID(iid, uuid) vboxIIDFromUUID_v2_x(data, iid, uuid) <Frame #2> 460 static void 461 vboxIIDFromUUID_v2_x(vboxGlobalData *data, vboxIID_v2_x *iid, 462 const unsigned char *uuid) 463 { 464 vboxIIDUnalloc_v2_x(data, iid); 465 466 iid->value = &iid->backing; 467 (1) Event deref_parm_in_call: Function "nsIDFromChar(nsID *, unsigned char const *)" dereferences "iid->value". [details] 468 nsIDFromChar(iid->value, uuid); 469 } 470 [notes] * The Unalloc call returns immediately because iid->value = NULL 440 static void 441 vboxIIDUnalloc_v2_x(vboxGlobalData *data, vboxIID_v2_x *iid) 442 { 443 if (iid->value == NULL) { 444 return; 445 } 446 447 if (iid->value != &iid->backing) { 448 data->pFuncs->pfnComUnallocMem(iid->value); 449 } 450 451 iid->value = NULL; 452 } * We set iid->value = &iid->backing. * Call nsIDFromChar() <Frame #3> 324 static void nsIDFromChar(nsID *iid, const unsigned char *uuid) { ... (9) Event deref_parm_in_call: Function "memcpy(void * restrict, void const * restrict, size_t)" dereferences "iid". 361 memcpy(iid, uuidinterim, VIR_UUID_BUFLEN); 362 } [notes] * Events 1-8 are from the for() loop in nsIDFromChar (irrelevant for this issue)
Adjust the macros to free memory allocated during various calls to perform the check if parameter is NULL prior to really freeing and to set the pointer to NULL after done freeing. --- src/vbox/vbox_tmpl.c | 76 +++++++++++++++++++++++++--------------------------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index d2cd0b8..86e08da 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -80,9 +80,30 @@ #define VIR_FROM_THIS VIR_FROM_VBOX -#define VBOX_UTF16_FREE(arg) data->pFuncs->pfnUtf16Free(arg) -#define VBOX_UTF8_FREE(arg) data->pFuncs->pfnUtf8Free(arg) -#define VBOX_COM_UNALLOC_MEM(arg) data->pFuncs->pfnComUnallocMem(arg) +#define VBOX_UTF16_FREE(arg) \ + do { \ + if (arg) { \ + data->pFuncs->pfnUtf16Free((arg)); \ + (arg) = NULL; \ + } \ + } while (0) + +#define VBOX_UTF8_FREE(arg) \ + do { \ + if (arg) { \ + data->pFuncs->pfnUtf8Free((arg)); \ + (arg) = NULL; \ + } \ + } while (0) + +#define VBOX_COM_UNALLOC_MEM(arg) \ + do { \ + if (arg) { \ + data->pFuncs->pfnComUnallocMem((arg)); \ + (arg) = NULL; \ + } \ + } while (0) + #define VBOX_UTF16_TO_UTF8(arg1, arg2) data->pFuncs->pfnUtf16ToUtf8(arg1, arg2) #define VBOX_UTF8_TO_UTF16(arg1, arg2) data->pFuncs->pfnUtf8ToUtf16(arg1, arg2) @@ -1401,14 +1422,8 @@ static virDomainPtr vboxDomainLookupByName(virConnectPtr conn, const char *name) ret->id = i + 1; } - if (machineNameUtf8) { - VBOX_UTF8_FREE(machineNameUtf8); - machineNameUtf8 = NULL; - } - if (machineNameUtf16) { - VBOX_COM_UNALLOC_MEM(machineNameUtf16); - machineNameUtf16 = NULL; - } + VBOX_UTF8_FREE(machineNameUtf8); + VBOX_COM_UNALLOC_MEM(machineNameUtf16); if (matched == 1) break; } @@ -1971,10 +1986,8 @@ static int vboxDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { ret = 0; } - if (machineName) - VBOX_UTF8_FREE(machineName); - if (machineNameUtf16) - VBOX_COM_UNALLOC_MEM(machineNameUtf16); + VBOX_UTF8_FREE(machineName); + VBOX_COM_UNALLOC_MEM(machineNameUtf16); if (info->nrVirtCpu) break; } @@ -2435,10 +2448,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { VBOX_UTF16_TO_UTF8(valueDisplayUtf16, &valueDisplayUtf8); VBOX_UTF16_FREE(valueDisplayUtf16); - if (strlen(valueDisplayUtf8) <= 0) { + if (strlen(valueDisplayUtf8) <= 0) VBOX_UTF8_FREE(valueDisplayUtf8); - valueDisplayUtf8 = NULL; - } } if (STREQ(valueTypeUtf8, "sdl")) { @@ -2468,8 +2479,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { } totalPresent++; } - if (valueDisplayUtf8) - VBOX_UTF8_FREE(valueDisplayUtf8); + VBOX_UTF8_FREE(valueDisplayUtf8); } if (STREQ(valueTypeUtf8, "vrdp")) @@ -3562,10 +3572,8 @@ vboxStartMachine(virDomainPtr dom, int i, IMachine *machine, VBOX_UTF16_TO_UTF8(valueDisplayUtf16, &valueDisplayUtf8); VBOX_UTF16_FREE(valueDisplayUtf16); - if (strlen(valueDisplayUtf8) <= 0) { + if (strlen(valueDisplayUtf8) <= 0) VBOX_UTF8_FREE(valueDisplayUtf8); - valueDisplayUtf8 = NULL; - } } if (STREQ(valueTypeUtf8, "sdl")) { @@ -3613,8 +3621,7 @@ vboxStartMachine(virDomainPtr dom, int i, IMachine *machine, } else { guiPresent = 1; } - if (valueDisplayUtf8) - VBOX_UTF8_FREE(valueDisplayUtf8); + VBOX_UTF8_FREE(valueDisplayUtf8); if (guiPresent) { if (guiDisplay) { @@ -4592,10 +4599,7 @@ vboxAttachSerial(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) } VBOX_RELEASE(serialPort); - if (pathUtf16) { - VBOX_UTF16_FREE(pathUtf16); - pathUtf16 = NULL; - } + VBOX_UTF16_FREE(pathUtf16); } } } @@ -4659,10 +4663,7 @@ vboxAttachParallel(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) parallelPort->vtbl->SetEnabled(parallelPort, 1); VBOX_RELEASE(parallelPort); - if (pathUtf16) { - VBOX_UTF16_FREE(pathUtf16); - pathUtf16 = NULL; - } + VBOX_UTF16_FREE(pathUtf16); } } } @@ -8459,8 +8460,7 @@ static virStorageVolPtr vboxStorageVolLookupByName(virStoragePoolPtr pool, const break; } - if (nameUtf8) - VBOX_UTF8_FREE(nameUtf8); + VBOX_UTF8_FREE(nameUtf8); } } } @@ -8591,8 +8591,7 @@ static virStorageVolPtr vboxStorageVolLookupByPath(virConnectPtr conn, const cha vboxIIDUnalloc(&hddIID); } - if (hddNameUtf8) - VBOX_UTF8_FREE(hddNameUtf8); + VBOX_UTF8_FREE(hddNameUtf8); } VBOX_MEDIUM_RELEASE(hardDisk); @@ -8832,8 +8831,7 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, VIR_DEBUG("deregistering hdd:%d", deregister); } - if (controller) - VBOX_UTF16_FREE(controller); + VBOX_UTF16_FREE(controller); } vboxIIDUnalloc(&iid); } -- 1.7.11.7
On 01/23/2013 05:34 PM, John Ferlan wrote:
Adjust the macros to free memory allocated during various calls to perform the check if parameter is NULL prior to really freeing and to set the pointer to NULL after done freeing. --- src/vbox/vbox_tmpl.c | 76 +++++++++++++++++++++++++--------------------------- 1 file changed, 37 insertions(+), 39 deletions(-)
+#define VBOX_UTF16_FREE(arg) \ + do { \ + if (arg) { \ + data->pFuncs->pfnUtf16Free((arg)); \
Unneeded set of () here, but it doesn't hurt anything. Also, indentation is off. Modifying cfg.mk will make it easy to enforce that we don't re-introduce the problem. ACK with this squashed in, so I pushed. diff --git i/cfg.mk w/cfg.mk index a687eb9..2dfde01 100644 --- i/cfg.mk +++ w/cfg.mk @@ -93,6 +93,9 @@ VC_LIST_ALWAYS_EXCLUDE_REGEX = \ # Functions like free() that are no-ops on NULL arguments. useless_free_options = \ + --name=VBOX_UTF16_FREE \ + --name=VBOX_UTF8_FREE \ + --name=VBOX_COM_UNALLOC_MEM \ --name=VIR_FREE \ --name=qemuCapsFree \ --name=qemuMigrationCookieFree \ diff --git i/src/vbox/vbox_tmpl.c w/src/vbox/vbox_tmpl.c index 86e08da..0a164dc 100644 --- i/src/vbox/vbox_tmpl.c +++ w/src/vbox/vbox_tmpl.c @@ -8,7 +8,7 @@ */ /* - * Copyright (C) 2010-2012 Red Hat, Inc. + * Copyright (C) 2010-2013 Red Hat, Inc. * Copyright (C) 2008-2009 Sun Microsystems, Inc. * * This file is part of a free software library; you can redistribute @@ -80,28 +80,28 @@ #define VIR_FROM_THIS VIR_FROM_VBOX -#define VBOX_UTF16_FREE(arg) \ - do { \ - if (arg) { \ - data->pFuncs->pfnUtf16Free((arg)); \ - (arg) = NULL; \ - } \ +#define VBOX_UTF16_FREE(arg) \ + do { \ + if (arg) { \ + data->pFuncs->pfnUtf16Free(arg); \ + (arg) = NULL; \ + } \ } while (0) -#define VBOX_UTF8_FREE(arg) \ - do { \ - if (arg) { \ - data->pFuncs->pfnUtf8Free((arg)); \ - (arg) = NULL; \ - } \ +#define VBOX_UTF8_FREE(arg) \ + do { \ + if (arg) { \ + data->pFuncs->pfnUtf8Free(arg); \ + (arg) = NULL; \ + } \ } while (0) -#define VBOX_COM_UNALLOC_MEM(arg) \ - do { \ - if (arg) { \ - data->pFuncs->pfnComUnallocMem((arg)); \ - (arg) = NULL; \ - } \ +#define VBOX_COM_UNALLOC_MEM(arg) \ + do { \ + if (arg) { \ + data->pFuncs->pfnComUnallocMem(arg); \ + (arg) = NULL; \ + } \ } while (0) #define VBOX_UTF16_TO_UTF8(arg1, arg2) data->pFuncs->pfnUtf16ToUtf8(arg1, arg2) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
John Ferlan