On 10/14/11 1:55 PM, "Eric Blake" <eblake@xxxxxxxxxx> wrote:
> On 10/14/2011 02:41 PM, Roopa Prabhu wrote:
>>
>>
>>
>> On 10/13/11 3:49 PM, "Eric Blake"<eblake@xxxxxxxxxx> wrote:
>>
>>> Detected by Coverity. Leak present since commit ca3b22b.
>>>
>>> * src/util/macvtap.c (doPortProfileOp8021Qbh): Release device name.
>>> ---
>>> getPhysfnDev allocates physfndev, but nothing freed it.
>>> Pushing under the trivial rule.
>>
>> Actually looks like physfndev is conditionally allocated in getPhysfnDev
>> Its better to modify getPhysfnDev to allocate physfndev every time.
>
> Also a good catch - otherwise we might have a double free or otherwise
> crash on freeing an invalid pointer.
>
>>
>>
>> I ACK this patch with the additional change below (looks ok ?)
>>
>> diff --git a/src/util/macvtap.c b/src/util/macvtap.c
>> index a020c90..b50b7d2 100644
>> --- a/src/util/macvtap.c
>> +++ b/src/util/macvtap.c
>> @@ -964,7 +964,7 @@ getPhysfnDev(const char *linkdev,
>> */
>>
>> *vf = PORT_SELF_VF;
>> - *physfndev = (char *)linkdev;
>> + *physfndev = strdup(linkdev);
>
> I had already pushed mine. Yours is missing a virReportOOMError() on
> allocation failure; but ACK with that improvement.
>
> Meanwhile, we need to scrub this file - it uses a convention of 1 on
> error, instead of the more typical -1 on error in the rest of the code
> base; but I want this bug fix to be separate from that subsequent cleanup.
>
> diff --git i/src/util/macvtap.c w/src/util/macvtap.c
> index b50b7d2..33f0a13 100644
> --- i/src/util/macvtap.c
> +++ w/src/util/macvtap.c
> @@ -965,6 +965,10 @@ getPhysfnDev(const char *linkdev,
>
> *vf = PORT_SELF_VF;
> *physfndev = strdup(linkdev);
> + if (!*physfndev) {
> + virReportOOMError();
> + rc = -1;
> + }
> }
>
> return rc;
>
Great. Thanks!. I will send out a patch for the bug fix and later work on
the fixing the error return convention.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list