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