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()
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list