[libvirt] [PATCH v4 01/13] Adapt to VIR_STRDUP and VIR_STRNDUP in src/conf/*

[John Ferlan]

On 05/23/2013 09:44 AM, Michal Privoznik wrote:
> On 23.05.2013 15:04, John Ferlan wrote:
>> On 05/20/2013 01:55 PM, Michal Privoznik wrote:
>>> ---
>> [...snip...]
>>
>>> diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c
>>> index 2509c0d..ac5f7ed 100644
>>> --- a/src/conf/nwfilter_params.c
>>> +++ b/src/conf/nwfilter_params.c
>>> @@ -79,19 +79,15 @@ virNWFilterVarValueCopy(const virNWFilterVarValuePtr val)
>>>  
>>>      switch (res->valType) {
>>>      case NWFILTER_VALUE_TYPE_SIMPLE:
>>> -        if (val->u.simple.value) {
>>> -            res->u.simple.value = strdup(val->u.simple.value);
>>> -            if (!res->u.simple.value)
>>> -                goto err_exit;
>>> -        }
>>> +        if (VIR_STRDUP(res->u.simple.value, val->u.simple.value) < 0)
>>> +            goto err_exit;
>>>          break;
>>>      case NWFILTER_VALUE_TYPE_ARRAY:
>>>          if (VIR_ALLOC_N(res->u.array.values, val->u.array.nValues) < 0)
>>>              goto err_exit;
>>>          res->u.array.nValues = val->u.array.nValues;
>>>          for (i = 0; i < val->u.array.nValues; i++) {
>>> -            str = strdup(val->u.array.values[i]);
>>> -            if (!str)
>>> +            if (VIR_STRDUP(str, val->u.array.values[i]) < 0)
>>>                  goto err_exit;
>>>              res->u.array.values[i] = str;
>>>          }
>>> @@ -133,12 +129,10 @@ virNWFilterVarValueCreateSimple(char *value)
>>>  virNWFilterVarValuePtr
>>>  virNWFilterVarValueCreateSimpleCopyValue(const char *value)
>>>  {
>>> -    char *val = strdup(value);
>>> +    char *val;
>>>  
>>> -    if (!val) {
>>> -        virReportOOMError();
>>> +    if (VIR_STRDUP(val, value) < 0)
>>>          return NULL;
>>> -    }
>>>      return virNWFilterVarValueCreateSimple(val);
>>>  }
>>>  
>>> @@ -654,17 +648,15 @@ virNWFilterHashTablePut(virNWFilterHashTablePtr table,
>>>  {
>>>      if (!virHashLookup(table->hashTable, name)) {
>>>          if (copyName) {
>>> -            name = strdup(name);
>>> -            if (!name) {
>>> -                virReportOOMError();
>>> +            char *newName;
>>> +            if (VIR_STRDUP(newName, name) < 0)
>>>                  return -1;
>>> -            }
>>>  
>>>              if (VIR_REALLOC_N(table->names, table->nNames + 1) < 0) {
>>>                  VIR_FREE(name);
>>>                  return -1;
>>>              }
>>> -            table->names[table->nNames++] = (char *)name;
>>> +            table->names[table->nNames++] = newName;
>>>          }
>>
>> Coverity complains (see below too)
>>
>> 648  	{
>>
>> (1) Event cond_true: 	Condition "!virHashLookup(table->hashTable, name)", taking true branch
>>
>> 649  	    if (!virHashLookup(table->hashTable, name)) {
>>
>> (2) Event cond_true: 	Condition "copyName", taking true branch
>>
>> 650  	        if (copyName) {
>> 651  	            char *newName;
>>
>> (3) Event cond_false: 	Condition "virStrdup(&newName, name, true /* 1 */, VIR_FROM_NWFILTER, "conf/nwfilter_params.c", <anonymous>, 652) < 0", taking false branch
>>
>> 652  	            if (VIR_STRDUP(newName, name) < 0)
>> 653  	                return -1;
>> 654  	
>>
>> (5) Event cond_false: 	Condition "virReallocN(&table->names, 8UL /* sizeof (*table->names) */, table->nNames + 1) < 0", taking false branch
>>
>> 655  	            if (VIR_REALLOC_N(table->names, table->nNames + 1) < 0) {
>> 656  	                VIR_FREE(name);
>> 657  	                return -1;
>>
>> (6) Event if_end: 	End of if statement
>>
>> 658  	            }
>> 659  	            table->names[table->nNames++] = newName;
>> 660  	        }
>> 661  	
>>
>> (7) Event cond_true: 	Condition "virHashAddEntry(table->hashTable, name, val) < 0", taking true branch
>>
>> 662  	        if (virHashAddEntry(table->hashTable, name, val) < 0) {
>>
>> (8) Event cond_true: 	Condition "copyName", taking true branch
>>
>> 663  	            if (copyName) {
>>
>> (9) Event freed_arg: 	"virFree(void *)" frees parameter "name". [details]
>>
>> 664  	                VIR_FREE(name);
>> 665  	                table->nNames--;
>> 666  	            }
>> 667  	            return -1;
>> 668  	        }
>>
>>
>> In particular, the "VIR_FREE(name)" causes a problem in one of the callers,
>> see below.  So I think the fix is to dump "newNames" up one level, then change:
>>
>> 664  	                VIR_FREE(name);
>> 665  	                table->nNames--;
>>
>> to 
>>
>> 664  	                VIR_FREE(newName);
>> 665  	                table->nNames--;
>>
>>
>> John
> 
> Right, it took me a while to parse the coverity output, and here's the
> part of the original code that confused me:
> 
>      if (!virHashLookup(table->hashTable, name)) {
>          if (copyName) {
>              name = strdup(name);
> 
> 
> 

Taking advantage that when returned to the caller name is what it was...

The following seems right (that is use newName instead of name locally):


diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c
index ac5f7ed..44b9f05 100644
--- a/src/conf/nwfilter_params.c
+++ b/src/conf/nwfilter_params.c
@@ -647,13 +647,13 @@ virNWFilterHashTablePut(virNWFilterHashTablePtr table,
                         int copyName)
 {
     if (!virHashLookup(table->hashTable, name)) {
+        char *newName = NULL;
         if (copyName) {
-            char *newName;
             if (VIR_STRDUP(newName, name) < 0)
                 return -1;

             if (VIR_REALLOC_N(table->names, table->nNames + 1) < 0) {
-                VIR_FREE(name);
+                VIR_FREE(newName);
                 return -1;
             }
             table->names[table->nNames++] = newName;
@@ -661,7 +661,7 @@ virNWFilterHashTablePut(virNWFilterHashTablePtr table,

         if (virHashAddEntry(table->hashTable, name, val) < 0) {
             if (copyName) {
-                VIR_FREE(name);
+                VIR_FREE(newName);
                 table->nNames--;
             }
             return -1;