[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:56, John Ferlan wrote:
> 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;
> 
> 

Yup, that's exactly the patch that I wrote (haven't send it though yet).
ACK if you want to push it.

Michal


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