[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 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


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




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