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

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;