[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] macvtap: plug memory leak for 802.1Qbh

On 10/14/2011 02:41 PM, Roopa Prabhu wrote:

On 10/13/11 3:49 PM, "Eric Blake"<eblake redhat com>  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;

Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]