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);
>
>
>>
>> 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()
Yup, that due to the bug in virNWFilterHashTablePut().
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list