[libvirt] [PATCH 1/4] libvirtd: avoid memory leak on OOM
Detected by Coverity; only strikes on OOM so not serious. * daemon/libvirtd.c (daemonPidFilePath): Plug leak. --- daemon/libvirtd.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 60ee705..06d2077 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -310,8 +310,10 @@ daemonPidFilePath(bool privileged, if (!(userdir = virGetUserDirectory(geteuid()))) goto error; - if (virAsprintf(pidfile, "%s/.libvirt/libvirtd.pid", userdir) < 0) + if (virAsprintf(pidfile, "%s/.libvirt/libvirtd.pid", userdir) < 0) { + VIR_FREE(userdir); goto no_memory; + } VIR_FREE(userdir); } -- 1.7.4.4
Detected by Coverity. Unlikely to hit unless the file contents were corrupted. * src/util/interface.c (ifaceRestoreMacAddress): Plug leak. --- src/util/interface.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/util/interface.c b/src/util/interface.c index d51ceec..f486124 100644 --- a/src/util/interface.c +++ b/src/util/interface.c @@ -1,6 +1,7 @@ /* * interface.c: interface support functions * + * Copyright (C) 2011 Red Hat, Inc. * Copyright (C) 2010 IBM Corp. * Copyright (C) 2010 Stefan Berger * @@ -1100,6 +1101,7 @@ ifaceRestoreMacAddress(const char *linkdev, ifaceError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse MAC address from '%s'"), oldmacname); + VIR_FREE(macstr); return -1; } -- 1.7.4.4
At 07/01/2011 07:36 AM, Eric Blake Write:
Detected by Coverity. Unlikely to hit unless the file contents were corrupted.
* src/util/interface.c (ifaceRestoreMacAddress): Plug leak. --- src/util/interface.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/util/interface.c b/src/util/interface.c index d51ceec..f486124 100644 --- a/src/util/interface.c +++ b/src/util/interface.c @@ -1,6 +1,7 @@ /* * interface.c: interface support functions * + * Copyright (C) 2011 Red Hat, Inc. * Copyright (C) 2010 IBM Corp. * Copyright (C) 2010 Stefan Berger * @@ -1100,6 +1101,7 @@ ifaceRestoreMacAddress(const char *linkdev, ifaceError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse MAC address from '%s'"), oldmacname); + VIR_FREE(macstr); return -1; }
ACK
Detected by Coverity. The leak is on an error path, but I'm not sure whether that path is likely to be triggered in practice. * src/rpc/virnetserverservice.c (virNetServerServiceAccept): Plug leak. --- src/rpc/virnetserverservice.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index e84f72c..fcd783c 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -83,6 +83,7 @@ cleanup: error: virNetSocketFree(clientsock); + virNetServerClientFree(client); } -- 1.7.4.4
At 07/01/2011 07:36 AM, Eric Blake Write:
Detected by Coverity. The leak is on an error path, but I'm not sure whether that path is likely to be triggered in practice.
* src/rpc/virnetserverservice.c (virNetServerServiceAccept): Plug leak. --- src/rpc/virnetserverservice.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index e84f72c..fcd783c 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -83,6 +83,7 @@ cleanup:
error: virNetSocketFree(clientsock); + virNetServerClientFree(client);
If svc->dispatchFunc is NULL, we will goto here to do cleanup. Unfortunately, client->sock is clientsock and it will be freed again in virNetServerClientFree(). It may cause libvirtd crashed. If svc->dispatchFunc() failed, we close and free client. But we only free client here, not close it here. Do we need to close it here? I think the cleanup code should like this: if (client) { virNetServerClientClose(client); virNetServerClientFree(client); } else { virNetSocketFree(clientsock); }
}
On 06/30/2011 07:05 PM, Wen Congyang wrote:
At 07/01/2011 07:36 AM, Eric Blake Write:
Detected by Coverity. The leak is on an error path, but I'm not sure whether that path is likely to be triggered in practice.
error: virNetSocketFree(clientsock); + virNetServerClientFree(client);
If svc->dispatchFunc is NULL, we will goto here to do cleanup. Unfortunately, client->sock is clientsock and it will be freed again in virNetServerClientFree(). It may cause libvirtd crashed.
If svc->dispatchFunc() failed, we close and free client. But we only free client here, not close it here. Do we need to close it here?
I think the cleanup code should like this:
if (client) { virNetServerClientClose(client); virNetServerClientFree(client); } else { virNetSocketFree(clientsock); }
Makes sense to me, but I'd rather hear Dan's take on it as author of that code. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Jul 01, 2011 at 04:54:12PM -0600, Eric Blake wrote:
On 06/30/2011 07:05 PM, Wen Congyang wrote:
At 07/01/2011 07:36 AM, Eric Blake Write:
Detected by Coverity. The leak is on an error path, but I'm not sure whether that path is likely to be triggered in practice.
error: virNetSocketFree(clientsock); + virNetServerClientFree(client);
If svc->dispatchFunc is NULL, we will goto here to do cleanup. Unfortunately, client->sock is clientsock and it will be freed again in virNetServerClientFree(). It may cause libvirtd crashed.
If svc->dispatchFunc() failed, we close and free client. But we only free client here, not close it here. Do we need to close it here?
I think the cleanup code should like this:
if (client) { virNetServerClientClose(client); virNetServerClientFree(client); } else { virNetSocketFree(clientsock); }
Makes sense to me, but I'd rather hear Dan's take on it as author of that code.
Yep, ACK to Wen's version of the code Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Detected by Coverity. Some, but not all, error paths were clean; but they were repetitive so I refactored them. * src/util/pci.c (pciGetDevice): Plug leak. --- src/util/pci.c | 31 +++++++++++++++---------------- 1 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index 46a3a83..21c12b9 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -1294,7 +1294,8 @@ pciGetDevice(unsigned domain, unsigned function) { pciDevice *dev; - char *vendor, *product; + char *vendor = NULL; + char *product = NULL; if (VIR_ALLOC(dev) < 0) { virReportOOMError(); @@ -1313,22 +1314,19 @@ pciGetDevice(unsigned domain, pciReportError(VIR_ERR_INTERNAL_ERROR, _("dev->name buffer overflow: %.4x:%.2x:%.2x.%.1x"), dev->domain, dev->bus, dev->slot, dev->function); - pciFreeDevice(dev); - return NULL; + goto error; } if (virAsprintf(&dev->path, PCI_SYSFS "devices/%s/config", dev->name) < 0) { virReportOOMError(); - pciFreeDevice(dev); - return NULL; + goto error; } if (access(dev->path, F_OK) != 0) { virReportSystemError(errno, _("Device %s not found: could not access %s"), dev->name, dev->path); - pciFreeDevice(dev); - return NULL; + goto error; } vendor = pciReadDeviceID(dev, "vendor"); @@ -1338,10 +1336,7 @@ pciGetDevice(unsigned domain, pciReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to read product/vendor ID for %s"), dev->name); - VIR_FREE(product); - VIR_FREE(vendor); - pciFreeDevice(dev); - return NULL; + goto error; } /* strings contain '0x' prefix */ @@ -1350,16 +1345,20 @@ pciGetDevice(unsigned domain, pciReportError(VIR_ERR_INTERNAL_ERROR, _("dev->id buffer overflow: %s %s"), &vendor[2], &product[2]); - pciFreeDevice(dev); - return NULL; + goto error; } - VIR_FREE(product); - VIR_FREE(vendor); - VIR_DEBUG("%s %s: initialized", dev->id, dev->name); +cleanup: + VIR_FREE(product); + VIR_FREE(vendor); return dev; + +error: + pciFreeDevice(dev); + dev = NULL; + goto cleanup; } void -- 1.7.4.4
At 07/01/2011 07:36 AM, Eric Blake Write:
Detected by Coverity. Some, but not all, error paths were clean; but they were repetitive so I refactored them.
* src/util/pci.c (pciGetDevice): Plug leak. --- src/util/pci.c | 31 +++++++++++++++---------------- 1 files changed, 15 insertions(+), 16 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c index 46a3a83..21c12b9 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -1294,7 +1294,8 @@ pciGetDevice(unsigned domain, unsigned function) { pciDevice *dev; - char *vendor, *product; + char *vendor = NULL; + char *product = NULL;
if (VIR_ALLOC(dev) < 0) { virReportOOMError(); @@ -1313,22 +1314,19 @@ pciGetDevice(unsigned domain, pciReportError(VIR_ERR_INTERNAL_ERROR, _("dev->name buffer overflow: %.4x:%.2x:%.2x.%.1x"), dev->domain, dev->bus, dev->slot, dev->function); - pciFreeDevice(dev); - return NULL; + goto error; } if (virAsprintf(&dev->path, PCI_SYSFS "devices/%s/config", dev->name) < 0) { virReportOOMError(); - pciFreeDevice(dev); - return NULL; + goto error; }
if (access(dev->path, F_OK) != 0) { virReportSystemError(errno, _("Device %s not found: could not access %s"), dev->name, dev->path); - pciFreeDevice(dev); - return NULL; + goto error; }
vendor = pciReadDeviceID(dev, "vendor"); @@ -1338,10 +1336,7 @@ pciGetDevice(unsigned domain, pciReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to read product/vendor ID for %s"), dev->name); - VIR_FREE(product); - VIR_FREE(vendor); - pciFreeDevice(dev); - return NULL; + goto error; }
/* strings contain '0x' prefix */ @@ -1350,16 +1345,20 @@ pciGetDevice(unsigned domain, pciReportError(VIR_ERR_INTERNAL_ERROR, _("dev->id buffer overflow: %s %s"), &vendor[2], &product[2]); - pciFreeDevice(dev); - return NULL; + goto error; }
- VIR_FREE(product); - VIR_FREE(vendor); - VIR_DEBUG("%s %s: initialized", dev->id, dev->name);
+cleanup: + VIR_FREE(product); + VIR_FREE(vendor); return dev; + +error: + pciFreeDevice(dev); + dev = NULL; + goto cleanup; }
void
ACK
On 06/30/2011 07:13 PM, Wen Congyang wrote:
At 07/01/2011 07:36 AM, Eric Blake Write:
Detected by Coverity. Some, but not all, error paths were clean; but they were repetitive so I refactored them.
* src/util/pci.c (pciGetDevice): Plug leak. --- src/util/pci.c | 31 +++++++++++++++---------------- 1 files changed, 15 insertions(+), 16 deletions(-)
ACK
Thanks; 1, 2, and 4 now pushed. I'll revise 3 for my next round of Coverity findings. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
At 07/01/2011 07:36 AM, Eric Blake Write:
Detected by Coverity; only strikes on OOM so not serious.
* daemon/libvirtd.c (daemonPidFilePath): Plug leak. --- daemon/libvirtd.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 60ee705..06d2077 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -310,8 +310,10 @@ daemonPidFilePath(bool privileged, if (!(userdir = virGetUserDirectory(geteuid()))) goto error;
- if (virAsprintf(pidfile, "%s/.libvirt/libvirtd.pid", userdir) < 0) + if (virAsprintf(pidfile, "%s/.libvirt/libvirtd.pid", userdir) < 0) { + VIR_FREE(userdir); goto no_memory; + }
VIR_FREE(userdir); }
ACK
于 2011年07月01日 07:36, Eric Blake 写道:
Detected by Coverity; only strikes on OOM so not serious.
* daemon/libvirtd.c (daemonPidFilePath): Plug leak. --- daemon/libvirtd.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 60ee705..06d2077 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -310,8 +310,10 @@ daemonPidFilePath(bool privileged, if (!(userdir = virGetUserDirectory(geteuid()))) goto error;
- if (virAsprintf(pidfile, "%s/.libvirt/libvirtd.pid", userdir)< 0) + if (virAsprintf(pidfile, "%s/.libvirt/libvirtd.pid", userdir)< 0) { + VIR_FREE(userdir); goto no_memory; + }
VIR_FREE(userdir); }
ACK Regards Osier
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Osier Yang -
Wen Congyang