6:34 p.m.
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
6:34 p.m.
New subject: [libvirt] [PATCH 1/2] vbox: Address false positive for NULL dereference
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
5:02 p.m.
New subject: [libvirt] [PATCH 1/2] vbox: Address false positive for NULL dereference
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
6:59 a.m.
New subject: [libvirt] [PATCH 1/2] vbox: Address false positive for NULL dereference
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)
6:34 p.m.
New subject: [libvirt] [PATCH 2/2] vbox: Adjust the UTF FREE macros
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
5:16 p.m.
New subject: [libvirt] [PATCH 2/2] vbox: Adjust the UTF FREE macros
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
4320
days inactive
4322
days old
5 comments
2 participants
participants (2)
-
Eric Blake -
John Ferlan