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