[libvirt] [PATCH 1/2] vbox: Address false positive for NULL dereference

[John Ferlan]

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)