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