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(a)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;