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