On Thu, Jun 02, 2011 at 05:07:56PM -0600, Eric Blake wrote:

Spotted by coverity. Triggers on failed stat, although I'm not sure how easy that condition is, so I'm not sure if this is a runtime memory hog. Regression introduced in commit 8077d64 (unreleased).

* src/util/storage_file.c (virStorageFileGetMetadataFromFD): Reduce need for malloc, avoiding a leak. --- src/util/storage_file.c | 15 +++++++-------- 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 8dbd933..6b3b756 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -831,11 +831,6 @@ virStorageFileGetMetadataFromFD(const char *path, int ret = -1; struct stat sb;

- if (VIR_ALLOC_N(head, len) < 0) { - virReportOOMError(); - return -1; - } - memset(meta, 0, sizeof (*meta));

if (fstat(fd, &sb) < 0) { @@ -847,13 +842,17 @@ virStorageFileGetMetadataFromFD(const char *path,

/* No header to probe for directories */ if (S_ISDIR(sb.st_mode)) { - ret = 0; - goto cleanup; + return 0; }

if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { virReportSystemError(errno, _("cannot seek to start of '%s'"), path); - goto cleanup; + return -1; + } + + if (VIR_ALLOC_N(head, len) < 0) { + virReportOOMError(); + return -1; }

if ((len = read(fd, head, len)) < 0) {

ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Thu, Jun 02, 2011 at 05:07:57PM -0600, Eric Blake wrote:

Detected by Coverity. Only possible on OOM situations.

* src/locking/lock_manager.c (virLockManagerPluginNew): Plug leak. --- src/locking/lock_manager.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index 6197fd4..138cc91 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -120,7 +120,7 @@ virLockManagerPluginPtr virLockManagerPluginNew(const char *name, { void *handle = NULL; virLockDriverPtr driver; - virLockManagerPluginPtr plugin; + virLockManagerPluginPtr plugin = NULL; const char *moddir = getenv("LIBVIRT_LOCK_MANAGER_PLUGIN_DIR"); char *modfile = NULL;

@@ -182,6 +182,7 @@ virLockManagerPluginPtr virLockManagerPluginNew(const char *name, return plugin;

cleanup: + VIR_FREE(plugin); VIR_FREE(modfile); if (handle) dlclose(handle);

ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Thu, Jun 02, 2011 at 05:07:58PM -0600, Eric Blake wrote:

Detected by Coverity. Only possible in OOM situations.

* daemon/remote.c (remoteDispatchDomainScreenshot): Plug leak. --- daemon/remote.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c index 2a32ee8..49058f2 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1453,6 +1453,7 @@ remoteDispatchDomainScreenshot(struct qemud_server *server ATTRIBUTE_UNUSED, *mime_p = strdup(mime); if (*mime_p == NULL) { virReportOOMError(); + VIR_FREE(mime_p); goto cleanup; }

ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Thu, Jun 02, 2011 at 05:07:59PM -0600, Eric Blake wrote:

Detected by Coverity. This leaked a cpumap on every iteration of the loop. Leak introduced in commit 1cc4d02 (v0.9.0).

* src/qemu/qemu_process.c (qemuProcessSetVcpuAffinites): Plug leak, and hoist allocation outside loop. --- src/qemu/qemu_process.c | 21 +++++++++++++-------- 1 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 116253e..f175d50 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1195,6 +1195,8 @@ qemuProcessSetVcpuAffinites(virConnectPtr conn, pid_t vcpupid; unsigned char *cpumask; int vcpu, cpumaplen, hostcpus, maxcpu; + unsigned char *cpumap = NULL; + int ret = -1;

if (virNodeGetInfo(conn, &nodeinfo) != 0) { return -1; @@ -1216,18 +1218,18 @@ qemuProcessSetVcpuAffinites(virConnectPtr conn, if (maxcpu > hostcpus) maxcpu = hostcpus;

+ if (VIR_ALLOC_N(cpumap, cpumaplen) < 0) { + virReportOOMError(); + return -1; + } + for (vcpu = 0; vcpu < def->cputune.nvcpupin; vcpu++) { if (vcpu != def->cputune.vcpupin[vcpu]->vcpuid) continue;

int i; - unsigned char *cpumap = NULL; - - if (VIR_ALLOC_N(cpumap, cpumaplen) < 0) { - virReportOOMError(); - return -1; - }

+ memset(cpumap, 0, cpumaplen); cpumask = (unsigned char *)def->cputune.vcpupin[vcpu]->cpumask; vcpupid = priv->vcpupids[vcpu];

@@ -1249,11 +1251,14 @@ qemuProcessSetVcpuAffinites(virConnectPtr conn, cpumap, cpumaplen, maxcpu) < 0) { - return -1; + goto cleanup; } }

- return 0; + ret = 0; +cleanup: + VIR_FREE(cpumap); + return ret; }

Whoops !!! ACK, better to allocate out of the loop, fix looks fine, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Thu, Jun 02, 2011 at 05:08:00PM -0600, Eric Blake wrote:

Detected by Coverity. Introduced in commit aaf2b70, and turned into a regression in the next few commits through 4e6e6672 (unreleased).

* src/conf/domain_event.c (virDomainEventStateFree): Free object, per documentation. --- src/conf/domain_event.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 34a9d91..fabc1a5 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -1,7 +1,7 @@ /* * domain_event.c: domain event queue processing helpers * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2008 VirtualIron * * This library is free software; you can redistribute it and/or @@ -555,6 +555,7 @@ virDomainEventStateFree(virDomainEventStatePtr state)

if (state->timer != -1) virEventRemoveTimeout(state->timer); + VIR_FREE(state); }

ACK, otherwise we leak in various ways from different drivers, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Thu, Jun 02, 2011 at 05:08:01PM -0600, Eric Blake wrote:

Coverity complained that infd could be -1 at the point where it is passed to write, when in reality, this code can only be reached if infd is non-negative.

* src/util/command.c (virCommandProcessIO): Help out coverity. --- src/util/command.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/util/command.c b/src/util/command.c index a2f7ff6..b51bdcf 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -961,6 +961,9 @@ virCommandProcessIO(virCommandPtr cmd) } else { int done;

+ /* Coverity 5.3.0 can't see that we only get here if + * infd is in the set because it was non-negative. */ + sa_assert(infd != -1); done = write(infd, cmd->inbuf + inoff, inlen - inoff); if (done < 0) {

ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Coverity couldn't see that priv is clean on failure. But on failure, we might as well guarantee that callers don't try to free uninitialized memory. * src/remote/remote_driver.c (remoteGenericOpen): Even on failure, pass priv back to caller. --- In reading my own mail, I decided that patch 10 can be made simpler. v2: rewrite to avoid sa_assert and be more robust. src/remote/remote_driver.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 14c3d24..8335a1a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2545,10 +2545,8 @@ remoteGenericOpen(virConnectPtr conn, virConnectAuthPtr auth, * use the UNIX transport. This handles Xen driver * which doesn't have its own impl of the network APIs. */ struct private_data *priv; - int ret; - ret = remoteOpenSecondaryDriver(conn, auth, flags, &priv); - if (ret == VIR_DRV_OPEN_SUCCESS) - *genericPrivateData = priv; + int ret = remoteOpenSecondaryDriver(conn, auth, flags, &priv); + *genericPrivateData = priv; return ret; } } -- 1.7.4.4