Re: [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)