[libvirt] [PATCH] macvtap: plug memory leak for 802.1Qbh
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. src/util/macvtap.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 9bf7fa6..a020c90 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -983,27 +983,27 @@ doPortProfileOp8021Qbh(const char *ifname, # ifndef IFLA_VF_PORT_MAX (void)ifname; (void)macaddr; (void)virtPort; (void)vm_uuid; (void)virtPortOp; macvtapError(VIR_ERR_INTERNAL_ERROR, "%s", _("Kernel VF Port support was missing at compile time.")); rc = 1; # else /* IFLA_VF_PORT_MAX */ - char *physfndev; + char *physfndev = NULL; unsigned char hostuuid[VIR_UUID_BUFLEN]; int32_t vf; bool nltarget_kernel = true; int ifindex; int vlanid = -1; rc = getPhysfnDev(ifname, &vf, &physfndev); if (rc) goto err_exit; if (ifaceGetIndex(true, physfndev, &ifindex) < 0) { rc = 1; goto err_exit; @@ -1049,26 +1049,27 @@ doPortProfileOp8021Qbh(const char *ifname, NULL, NULL, vf, PORT_REQUEST_DISASSOCIATE); break; default: macvtapError(VIR_ERR_INTERNAL_ERROR, _("operation type %d not supported"), virtPortOp); rc = 1; } err_exit: + VIR_FREE(physfndev); # endif /* IFLA_VF_PORT_MAX */ return rc; } /** * vpAssociatePortProfile * * @macvtap_ifname: The name of the macvtap device * @virtPort: pointer to the object holding port profile parameters * @vmuuid : the UUID of the virtual machine * @vmOp : The VM operation (i.e., create, no-op) -- 1.7.4.4
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.
src/util/macvtap.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 9bf7fa6..a020c90 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -983,27 +983,27 @@ doPortProfileOp8021Qbh(const char *ifname, # ifndef IFLA_VF_PORT_MAX
(void)ifname; (void)macaddr; (void)virtPort; (void)vm_uuid; (void)virtPortOp; macvtapError(VIR_ERR_INTERNAL_ERROR, "%s", _("Kernel VF Port support was missing at compile time.")); rc = 1;
# else /* IFLA_VF_PORT_MAX */
- char *physfndev; + char *physfndev = NULL; unsigned char hostuuid[VIR_UUID_BUFLEN]; int32_t vf; bool nltarget_kernel = true; int ifindex; int vlanid = -1;
rc = getPhysfnDev(ifname, &vf, &physfndev); if (rc) goto err_exit;
if (ifaceGetIndex(true, physfndev, &ifindex) < 0) { rc = 1; goto err_exit; @@ -1049,26 +1049,27 @@ doPortProfileOp8021Qbh(const char *ifname, NULL, NULL, vf, PORT_REQUEST_DISASSOCIATE); break;
default: macvtapError(VIR_ERR_INTERNAL_ERROR, _("operation type %d not supported"), virtPortOp); rc = 1; }
err_exit: + VIR_FREE(physfndev);
Thanks for catching this Eric Actually looks like physfndev is conditionally allocated in getPhysfnDev Its better to modify getPhysfnDev to allocate physfndev every time. 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); } return rc;
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
On 10/14/11 1:55 PM, "Eric Blake" <eblake@redhat.com> wrote:
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;
Great. Thanks!. I will send out a patch for the bug fix and later work on the fixing the error return convention.
On 10/14/2011 03:05 PM, Roopa Prabhu wrote:
I had already pushed mine. Yours is missing a virReportOOMError() on allocation failure; but ACK with that improvement.
Great. Thanks!. I will send out a patch for the bug fix and later work on the fixing the error return convention.
I already pushed the bug fix in your name: commit 80b077ee5ea0dd899b87d370f9fa892e727832f5 Author: Roopa Prabhu <roprabhu@cisco.com> Date: Fri Oct 14 13:41:46 2011 -0700 macvtap: avoid invalid free Commit 0472f39 plugged a leak, but introduced another bug: Actually looks like physfndev is conditionally allocated in getPhysfnDev Its better to modify getPhysfnDev to allocate physfndev every time. so all that remains is cleaning up the error return convention. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/14/11 2:08 PM, "Eric Blake" <eblake@redhat.com> wrote:
On 10/14/2011 03:05 PM, Roopa Prabhu wrote:
I had already pushed mine. Yours is missing a virReportOOMError() on allocation failure; but ACK with that improvement.
Great. Thanks!. I will send out a patch for the bug fix and later work on the fixing the error return convention.
I already pushed the bug fix in your name:
commit 80b077ee5ea0dd899b87d370f9fa892e727832f5 Author: Roopa Prabhu <roprabhu@cisco.com> Date: Fri Oct 14 13:41:46 2011 -0700
macvtap: avoid invalid free
Commit 0472f39 plugged a leak, but introduced another bug:
Actually looks like physfndev is conditionally allocated in getPhysfnDev Its better to modify getPhysfnDev to allocate physfndev every time.
so all that remains is cleaning up the error return convention.
Thanks eric.
participants (2)
-
Eric Blake -
Roopa Prabhu