[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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);



> 
> 
>>  
>>          if (virHashAddEntry(table->hashTable, name, val) < 0) {
>> @@ -1006,11 +998,8 @@ virNWFilterVarAccessParse(const char *varAccess)
>>  
>>      if (input[idx] == '\0') {
>>          /* in the form 'IP', which is equivalent to IP[ 0] */
>> -        dest->varName = strndup(input, idx);
>> -        if (!dest->varName) {
>> -            virReportOOMError();
>> +        if (VIR_STRNDUP(dest->varName, input, idx) < 0)
>>              goto err_exit;
>> -        }
>>          dest->accessType = VIR_NWFILTER_VAR_ACCESS_ITERATOR;
>>          dest->u.iterId = 0;
>>          return dest;
>> @@ -1023,11 +1012,8 @@ virNWFilterVarAccessParse(const char *varAccess)
>>  
>>          varNameLen = idx;
>>  
>> -        dest->varName = strndup(input, varNameLen);
>> -        if (!dest->varName) {
>> -            virReportOOMError();
>> +        if (VIR_STRNDUP(dest->varName, input, varNameLen) < 0)
>>              goto err_exit;
>> -        }
>>  
>>          input += idx + 1;
>>          virSkipSpaces(&input);
> 
> 
> Coverity complains:
> 
> 
> 
> 
> 746  	static void
> 747  	addToTable(void *payload, const void *name, void *data)
> 748  	{
> 749  	    struct addToTableStruct *atts = (struct addToTableStruct *)data;
> 750  	    virNWFilterVarValuePtr val;
> 751  	
> 
> (1) Event cond_false: 	Condition "atts->errOccurred", taking false branch
> 
> 752  	    if (atts->errOccurred)
> 753  	        return;
> 754  	
> 755  	    val = virNWFilterVarValueCopy((virNWFilterVarValuePtr)payload);
> 
> (2) Event cond_false: 	Condition "!val", taking false branch
> 
> 756  	    if (!val) {
> 757  	        virReportOOMError();
> 758  	        atts->errOccurred = 1;
> 759  	        return;
> 
> (3) Event if_end: 	End of if statement
> 
> 760  	    }
> 761  	
> 
> (4) Event freed_arg: 	"virNWFilterHashTablePut(virNWFilterHashTablePtr, char const *, virNWFilterVarValuePtr, int)" frees "name". [details]
> (5) Event cond_true: 	Condition "virNWFilterHashTablePut(atts->target, (char const *)name, val, 1) < 0", taking true branch
> Also see events: 	[pass_freed_arg]
> 
> 762  	    if (virNWFilterHashTablePut(atts->target, (const char *)name, val, 1) < 0){
> 
> (6) Event pass_freed_arg: 	Passing freed pointer "name" as an argument to function "virReportErrorHelper(int, int, char const *, char const *, size_t, char const *, ...)".
> Also see events: 	[freed_arg]
> 
> 763  	        virReportError(VIR_ERR_INTERNAL_ERROR,
> 764  	                       _("Could not put variable '%s' into hashmap"),
> 765  	                       (const char *)name);
> 766  	        atts->errOccurred = 1;
> 767  	        virNWFilterVarValueFree(val);
> 768  	    }
> 769  	}
> 
> 
> The complaint being 'name' could be VIR_FREE()'d and we would be using it in a virReportError()

Yup, that due to the bug in virNWFilterHashTablePut().

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]