[libvirt] [PATCH 0/8] more coverity cleanups
Many are minor (silencing warnings or only affect OOM), but at least patch 5/8 is a real bug that needs to be fixed pre-release. Eric Blake (8): rpc: avoid memory leak on error qemu: silence coverity warnings rpc: silence coverity warning cgroup: silence coverity warning rpc: fix logic bug qemu: avoid null deref on low memory vmware: avoid null deref on failed lookup storage: avoid crash on parse error src/qemu/qemu_audit.c | 6 ++++-- src/qemu/qemu_command.c | 6 +++++- src/qemu/qemu_domain.c | 4 ++-- src/qemu/qemu_migration.c | 3 ++- src/rpc/virnetclient.c | 3 ++- src/rpc/virnetserverclient.c | 1 + src/rpc/virnetserverservice.c | 7 ++++++- src/storage/storage_backend_iscsi.c | 14 ++++++++++---- src/util/cgroup.c | 2 +- src/vmware/vmware_driver.c | 2 +- 10 files changed, 34 insertions(+), 14 deletions(-) -- 1.7.4.4
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. --- v2: incorporate fixes suggested here: https://www.redhat.com/archives/libvir-list/2011-July/msg00058.html src/rpc/virnetserverservice.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index e84f72c..8c250e2 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -82,7 +82,12 @@ cleanup: return; error: - virNetSocketFree(clientsock); + if (client) { + virNetServerClientClose(client); + virNetServerClientFree(client); + } else { + virNetSocketFree(clientsock); + } } -- 1.7.4.4
Coverity warns if the majority of callers check a function for errors, but a few don't; but in qemu_audit and qemu_domain, the choice to not check for failures was safe. In qemu_command, the failure to generate a uuid can only occur on a bad pointer. * src/qemu/qemu_audit.c (qemuAuditCgroup): Ignore failure to get cgroup controller. * src/qemu/qemu_domain.c (qemuDomainObjEnterMonitor) (qemuDomainObjEnterMonitorWithDriver): Ignore failure to get timestamp. * src/qemu/qemu_command.c (qemuParseCommandLine): Check for error. --- src/qemu/qemu_audit.c | 6 ++++-- src/qemu/qemu_command.c | 6 +++++- src/qemu/qemu_domain.c | 4 ++-- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index 1da0773..1baef40 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -31,6 +31,7 @@ #include "uuid.h" #include "logging.h" #include "memory.h" +#include "ignore-value.h" /* Return nn:mm in hex for block and character devices, and NULL * for other file types, stat failure, or allocation failure. */ @@ -264,8 +265,9 @@ qemuAuditCgroup(virDomainObjPtr vm, virCgroupPtr cgroup, return; } - virCgroupPathOfController(cgroup, VIR_CGROUP_CONTROLLER_DEVICES, - NULL, &controller); + ignore_value(virCgroupPathOfController(cgroup, + VIR_CGROUP_CONTROLLER_DEVICES, + NULL, &controller)); detail = virAuditEncode("cgroup", VIR_AUDIT_STR(controller)); VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 90a6653..fc15f87 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5842,7 +5842,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (VIR_ALLOC(cmd) < 0) goto no_memory; - virUUIDGenerate(def->uuid); + if (virUUIDGenerate(def->uuid) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to generate uuid")); + goto error; + } def->id = -1; def->mem.cur_balloon = def->mem.max_balloon = 64 * 1024; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3af1c86..4b65d87 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -608,7 +608,7 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj) qemuMonitorLock(priv->mon); qemuMonitorRef(priv->mon); - virTimeMs(&priv->monStart); + ignore_value(virTimeMs(&priv->monStart)); virDomainObjUnlock(obj); } @@ -651,7 +651,7 @@ void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, qemuMonitorLock(priv->mon); qemuMonitorRef(priv->mon); - virTimeMs(&priv->monStart); + ignore_value(virTimeMs(&priv->monStart)); virDomainObjUnlock(obj); qemuDriverUnlock(driver); } -- 1.7.4.4
At 2011-7-2 7:36, Eric Blake write:
Coverity warns if the majority of callers check a function for errors, but a few don't; but in qemu_audit and qemu_domain, the choice to not check for failures was safe. In qemu_command, the failure to generate a uuid can only occur on a bad pointer.
* src/qemu/qemu_audit.c (qemuAuditCgroup): Ignore failure to get cgroup controller. * src/qemu/qemu_domain.c (qemuDomainObjEnterMonitor) (qemuDomainObjEnterMonitorWithDriver): Ignore failure to get timestamp. * src/qemu/qemu_command.c (qemuParseCommandLine): Check for error. --- src/qemu/qemu_audit.c | 6 ++++-- src/qemu/qemu_command.c | 6 +++++- src/qemu/qemu_domain.c | 4 ++-- 3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index 1da0773..1baef40 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -31,6 +31,7 @@ #include "uuid.h" #include "logging.h" #include "memory.h" +#include "ignore-value.h"
/* Return nn:mm in hex for block and character devices, and NULL * for other file types, stat failure, or allocation failure. */ @@ -264,8 +265,9 @@ qemuAuditCgroup(virDomainObjPtr vm, virCgroupPtr cgroup, return; }
- virCgroupPathOfController(cgroup, VIR_CGROUP_CONTROLLER_DEVICES, - NULL,&controller); + ignore_value(virCgroupPathOfController(cgroup, + VIR_CGROUP_CONTROLLER_DEVICES, + NULL,&controller)); detail = virAuditEncode("cgroup", VIR_AUDIT_STR(controller));
VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 90a6653..fc15f87 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5842,7 +5842,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (VIR_ALLOC(cmd)< 0) goto no_memory;
- virUUIDGenerate(def->uuid); + if (virUUIDGenerate(def->uuid)< 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to generate uuid")); + goto error; + }
def->id = -1; def->mem.cur_balloon = def->mem.max_balloon = 64 * 1024; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3af1c86..4b65d87 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -608,7 +608,7 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj)
qemuMonitorLock(priv->mon); qemuMonitorRef(priv->mon); - virTimeMs(&priv->monStart); + ignore_value(virTimeMs(&priv->monStart)); virDomainObjUnlock(obj); }
@@ -651,7 +651,7 @@ void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver,
qemuMonitorLock(priv->mon); qemuMonitorRef(priv->mon); - virTimeMs(&priv->monStart); + ignore_value(virTimeMs(&priv->monStart)); virDomainObjUnlock(obj); qemuDriverUnlock(driver); }
ACK
Coverity noted that 4 out of 5 calls to virNetClientStreamRaiseError checked the return value. This case expects a particular value, so warn if our expectations went wrong due to some bug elsewhere. * src/rpc/virnetclient.c (virNetClientCallDispatchStream): Warn on unexpected scenario. --- src/rpc/virnetclient.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index b551b99..615de6c 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -602,7 +602,8 @@ static int virNetClientCallDispatchStream(virNetClientPtr client) if (thecall && thecall->expectReply) { VIR_DEBUG("Got a synchronous error"); /* Raise error now, so that this call will see it immediately */ - virNetClientStreamRaiseError(st); + if (!virNetClientStreamRaiseError(st)) + VIR_DEBUG("unable to raise synchronous error"); thecall->mode = VIR_NET_CLIENT_MODE_COMPLETE; } return 0; -- 1.7.4.4
At 2011-7-2 7:36, Eric Blake write:
Coverity noted that 4 out of 5 calls to virNetClientStreamRaiseError checked the return value. This case expects a particular value, so warn if our expectations went wrong due to some bug elsewhere.
* src/rpc/virnetclient.c (virNetClientCallDispatchStream): Warn on unexpected scenario. --- src/rpc/virnetclient.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index b551b99..615de6c 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -602,7 +602,8 @@ static int virNetClientCallDispatchStream(virNetClientPtr client) if (thecall&& thecall->expectReply) { VIR_DEBUG("Got a synchronous error"); /* Raise error now, so that this call will see it immediately */ - virNetClientStreamRaiseError(st); + if (!virNetClientStreamRaiseError(st)) + VIR_DEBUG("unable to raise synchronous error"); thecall->mode = VIR_NET_CLIENT_MODE_COMPLETE; } return 0; ACK
Coverity noted that most clients reacted to failure to hash; but in a best-effort kill loop, we can ignore failure. * src/util/cgroup.c (virCgroupKillInternal): Ignore hash failure. --- src/util/cgroup.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 2e5ef46..740cedf 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -1395,7 +1395,7 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr done = false; } - virHashAddEntry(pids, (void*)pid, (void*)1); + ignore_value(virHashAddEntry(pids, (void*)pid, (void*)1)); } VIR_FORCE_FCLOSE(fp); } -- 1.7.4.4
At 2011-7-2 7:36, Eric Blake write:
Coverity noted that most clients reacted to failure to hash; but in a best-effort kill loop, we can ignore failure.
* src/util/cgroup.c (virCgroupKillInternal): Ignore hash failure. --- src/util/cgroup.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 2e5ef46..740cedf 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -1395,7 +1395,7 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr done = false; }
- virHashAddEntry(pids, (void*)pid, (void*)1); + ignore_value(virHashAddEntry(pids, (void*)pid, (void*)1)); } VIR_FORCE_FCLOSE(fp); } ACK
Spotted by Coverity. If we don't update tmp each time through the loop, then if the filter being removed was not the head of the list, we accidentally lose all filters prior to the one we wanted to remove. * src/rpc/virnetserverclient.c (virNetServerClientRemoveFilter): Don't lose unrelated filters. --- src/rpc/virnetserverclient.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 5c23cf2..30d7fcb 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -240,6 +240,7 @@ void virNetServerClientRemoveFilter(virNetServerClientPtr client, VIR_FREE(tmp); break; } + prev = tmp; tmp = tmp->next; } -- 1.7.4.4
At 2011-7-2 7:36, Eric Blake write:
Spotted by Coverity. If we don't update tmp each time through the loop, then if the filter being removed was not the head of the list, we accidentally lose all filters prior to the one we wanted to remove.
* src/rpc/virnetserverclient.c (virNetServerClientRemoveFilter): Don't lose unrelated filters. --- src/rpc/virnetserverclient.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 5c23cf2..30d7fcb 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -240,6 +240,7 @@ void virNetServerClientRemoveFilter(virNetServerClientPtr client, VIR_FREE(tmp); break; } + prev = tmp; tmp = tmp->next; }
ACK
Detected by Coverity. qemuDomainEventQueue requires a non-NULL pointer; most callers silently drop the event if we encountered and OOM situation trying to create the event. * src/qemu/qemu_migration.c (qemuMigrationFinish): Check for OOM. --- src/qemu/qemu_migration.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 800b714..d7b27a0 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2552,7 +2552,8 @@ qemuMigrationFinish(struct qemud_driver *driver, VIR_DOMAIN_EVENT_RESUMED_MIGRATED); if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) { virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_USER); - qemuDomainEventQueue(driver, event); + if (event) + qemuDomainEventQueue(driver, event); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_PAUSED); -- 1.7.4.4
At 2011-7-2 7:36, Eric Blake write:
Detected by Coverity. qemuDomainEventQueue requires a non-NULL pointer; most callers silently drop the event if we encountered and OOM situation trying to create the event.
* src/qemu/qemu_migration.c (qemuMigrationFinish): Check for OOM. --- src/qemu/qemu_migration.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 800b714..d7b27a0 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2552,7 +2552,8 @@ qemuMigrationFinish(struct qemud_driver *driver, VIR_DOMAIN_EVENT_RESUMED_MIGRATED); if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) { virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_USER); - qemuDomainEventQueue(driver, event); + if (event) + qemuDomainEventQueue(driver, event); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_PAUSED); ACK
Detected by Coverity. * src/vmware/vmware_driver.c (vmwareDomainReboot): Check error before dereferencing memory. --- src/vmware/vmware_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 5e2c4ba..52582d6 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -461,7 +461,6 @@ vmwareDomainReboot(virDomainPtr dom, unsigned int flags ATTRIBUTE_UNUSED) vmwareDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); vmwareDriverUnlock(driver); - vmxPath = ((vmwareDomainPtr) vm->privateData)->vmxPath; if (!vm) { vmwareError(VIR_ERR_NO_DOMAIN, "%s", @@ -469,6 +468,7 @@ vmwareDomainReboot(virDomainPtr dom, unsigned int flags ATTRIBUTE_UNUSED) goto cleanup; } + vmxPath = ((vmwareDomainPtr) vm->privateData)->vmxPath; vmwareSetSentinal(cmd, vmw_types[driver->type]); vmwareSetSentinal(cmd, vmxPath); -- 1.7.4.4
At 2011-7-2 7:36, Eric Blake write:
Detected by Coverity.
* src/vmware/vmware_driver.c (vmwareDomainReboot): Check error before dereferencing memory. --- src/vmware/vmware_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 5e2c4ba..52582d6 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -461,7 +461,6 @@ vmwareDomainReboot(virDomainPtr dom, unsigned int flags ATTRIBUTE_UNUSED) vmwareDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); vmwareDriverUnlock(driver); - vmxPath = ((vmwareDomainPtr) vm->privateData)->vmxPath;
if (!vm) { vmwareError(VIR_ERR_NO_DOMAIN, "%s", @@ -469,6 +468,7 @@ vmwareDomainReboot(virDomainPtr dom, unsigned int flags ATTRIBUTE_UNUSED) goto cleanup; }
+ vmxPath = ((vmwareDomainPtr) vm->privateData)->vmxPath; vmwareSetSentinal(cmd, vmw_types[driver->type]); vmwareSetSentinal(cmd, vmxPath);
ACK
Coverity detected that we could crash on bogus input. Meanwhile, strtok_r is rather heavy compared to strchr. * src/storage/storage_backend_iscsi.c (virStorageBackendIQNFound): Check for parse failure, and use lighter-weight functions. --- src/storage/storage_backend_iscsi.c | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 15b5862..72887e3 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -183,8 +183,7 @@ virStorageBackendIQNFound(const char *initiatoriqn, int ret = IQN_MISSING, fd = -1; char ebuf[64]; FILE *fp = NULL; - char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL, - *saveptr = NULL; + char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL; virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "iface", NULL); @@ -232,8 +231,15 @@ virStorageBackendIQNFound(const char *initiatoriqn, iqn++; if (STREQ(iqn, initiatoriqn)) { - token = strtok_r(line, " ", &saveptr); - *ifacename = strdup(token); + token = strchr(line, ' '); + if (!token) { + ret = IQN_ERROR; + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing space when parsing output " + "of '%s'"), ISCSIADM); + goto out; + } + *ifacename = strndup(line, token - line); if (*ifacename == NULL) { ret = IQN_ERROR; virReportOOMError(); -- 1.7.4.4
On Fri, Jul 01, 2011 at 05:35:58PM -0600, Eric Blake wrote:
Many are minor (silencing warnings or only affect OOM), but at least patch 5/8 is a real bug that needs to be fixed pre-release.
Eric Blake (8): rpc: avoid memory leak on error qemu: silence coverity warnings rpc: silence coverity warning cgroup: silence coverity warning rpc: fix logic bug qemu: avoid null deref on low memory vmware: avoid null deref on failed lookup storage: avoid crash on parse error
src/qemu/qemu_audit.c | 6 ++++-- src/qemu/qemu_command.c | 6 +++++- src/qemu/qemu_domain.c | 4 ++-- src/qemu/qemu_migration.c | 3 ++- src/rpc/virnetclient.c | 3 ++- src/rpc/virnetserverclient.c | 1 + src/rpc/virnetserverservice.c | 7 ++++++- src/storage/storage_backend_iscsi.c | 14 ++++++++++---- src/util/cgroup.c | 2 +- src/vmware/vmware_driver.c | 2 +- 10 files changed, 34 insertions(+), 14 deletions(-)
Okay, I applied that patch set. I just had to review 1/8 and 8/8 a bit deeper since they are not obvious, but looks fine, ACK thanks ! 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/
participants (3)
-
Daniel Veillard -
Eric Blake -
Wen Congyang