[libvirt] [PATCH 0/n] Coverity run
I'm doing another coverity cleanup run; Coverity reported as many as 75 errors this time around, but that definitely includes some false positives. I'll add to this thread as I sort through more reports to see which ones are real, but to get things started: Eric Blake (3): rpc: avoid double close on error rpc: avoid uninitialized memory use python: avoid unlikely sign extension bug python/libvirt-override.c | 2 +- src/rpc/virnetserver.c | 2 +- src/rpc/virnettlscontext.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) -- 1.7.4.4
Spotted by coverity. If pipe2 fails, then we attempt to close uninitialized fds, which may result in a double-close. * src/rpc/virnetserver.c (virNetServerSignalSetup): Initialize fds. --- src/rpc/virnetserver.c | 2 +- 1 file changed, 1 insertions(+), 1 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 2dae2ff..4deeca1 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -471,7 +471,7 @@ cleanup: static int virNetServerSignalSetup(virNetServerPtr srv) { - int fds[2]; + int fds[2] = { -1, -1 }; if (srv->sigwrite != -1) return 0; -- 1.7.4.4
On 08/02/2011 12:12 PM, Eric Blake wrote:
Spotted by coverity. If pipe2 fails, then we attempt to close uninitialized fds, which may result in a double-close.
* src/rpc/virnetserver.c (virNetServerSignalSetup): Initialize fds. --- src/rpc/virnetserver.c | 2 +- 1 file changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 2dae2ff..4deeca1 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -471,7 +471,7 @@ cleanup:
static int virNetServerSignalSetup(virNetServerPtr srv) { - int fds[2]; + int fds[2] = { -1, -1 };
if (srv->sigwrite != -1) return 0;
ACK.
Spotted by Coverity. Gnutls documents that buffer must be NULL if gnutls_x509_crt_get_key_purpose_oid is to be used to determine the correct size needed for allocating a buffer. * src/rpc/virnettlscontext.c (virNetTLSContextCheckCertKeyPurpose): Initialize buffer. --- src/rpc/virnettlscontext.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 2a58ede..be08207 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -264,7 +264,7 @@ static int virNetTLSContextCheckCertKeyPurpose(gnutls_x509_crt_t cert, int i; unsigned int purposeCritical; unsigned int critical; - char *buffer; + char *buffer = NULL; size_t size; bool allowClient = false, allowServer = false; -- 1.7.4.4
On 08/02/2011 12:12 PM, Eric Blake wrote:
Spotted by Coverity. Gnutls documents that buffer must be NULL if gnutls_x509_crt_get_key_purpose_oid is to be used to determine the correct size needed for allocating a buffer.
* src/rpc/virnettlscontext.c (virNetTLSContextCheckCertKeyPurpose): Initialize buffer. --- src/rpc/virnettlscontext.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 2a58ede..be08207 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -264,7 +264,7 @@ static int virNetTLSContextCheckCertKeyPurpose(gnutls_x509_crt_t cert, int i; unsigned int purposeCritical; unsigned int critical; - char *buffer; + char *buffer = NULL; size_t size; bool allowClient = false, allowServer = false;
ACK.
Detected by Coverity; same analysis as for commit f73198df. * python/libvirt-override.c (libvirt_virDomainGetVcpuPinInfo): Use correct type. --- python/libvirt-override.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/python/libvirt-override.c b/python/libvirt-override.c index aa84438..b5650e2 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -1114,7 +1114,7 @@ libvirt_virDomainGetVcpuPinInfo(PyObject *self ATTRIBUTE_UNUSED, virNodeInfo nodeinfo; virDomainInfo dominfo; unsigned char *cpumaps; - int cpumaplen, vcpu, pcpu; + size_t cpumaplen, vcpu, pcpu; unsigned int flags; int i_retval; -- 1.7.4.4
On 08/02/2011 12:12 PM, Eric Blake wrote:
Detected by Coverity; same analysis as for commit f73198df.
* python/libvirt-override.c (libvirt_virDomainGetVcpuPinInfo): Use correct type. --- python/libvirt-override.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c index aa84438..b5650e2 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -1114,7 +1114,7 @@ libvirt_virDomainGetVcpuPinInfo(PyObject *self ATTRIBUTE_UNUSED, virNodeInfo nodeinfo; virDomainInfo dominfo; unsigned char *cpumaps; - int cpumaplen, vcpu, pcpu; + size_t cpumaplen, vcpu, pcpu; unsigned int flags; int i_retval;
ACK.
On 08/02/2011 11:28 AM, Laine Stump wrote:
On 08/02/2011 12:12 PM, Eric Blake wrote:
Detected by Coverity; same analysis as for commit f73198df.
* python/libvirt-override.c (libvirt_virDomainGetVcpuPinInfo): Use correct type. --- python/libvirt-override.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c index aa84438..b5650e2 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -1114,7 +1114,7 @@ libvirt_virDomainGetVcpuPinInfo(PyObject *self ATTRIBUTE_UNUSED, virNodeInfo nodeinfo; virDomainInfo dominfo; unsigned char *cpumaps; - int cpumaplen, vcpu, pcpu; + size_t cpumaplen, vcpu, pcpu; unsigned int flags; int i_retval;
ACK.
1-3 pushed, and I'll have some more shortly. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Right now, every re-run of configure re-evaluates whether a static analysis tool is in use. But if you run configure under coverity, make a tweak, and then do an incremental rebuild with gcc but not coverity to test the tweak, then rerun a build under coverity, then configure does not get rerun, and static analysis ends up with lots of false positives. This patch caches the static analysis result, and also makes it easier to force static analysis even if the existing checks are insufficient to detect newer versions of the static analyzer tools. * configure.ac (lv_cv_static_analysis): New cache variable. --- I _thought_ some of those false positives were looking familiar from my last coverity run. Turns out I inadvertantly managed to lose my STATIC_ANALYSIS define. configure.ac | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 7c4fb32..10487e5 100644 --- a/configure.ac +++ b/configure.ac @@ -2404,9 +2404,16 @@ cp -f COPYING.LIB COPYING # Detect when running under the clang static analyzer's scan-build driver # or Coverity-prevent's cov-build. Define STATIC_ANALYSIS accordingly. +AC_CACHE_CHECK([whether this build is done by a static analysis tool], + [lv_cv_static_analysis], [ + lv_cv_static_analysis=no + if test -n "${CCC_ANALYZER_ANALYSIS+set}" || \ + test -n "$COVERITY_BUILD_COMMAND$COVERITY_LD_PRELOAD"; then + lv_cv_static_analysis=yes + fi + ]) t=0 -test -n "${CCC_ANALYZER_ANALYSIS+set}" && t=1 -test -n "$COVERITY_BUILD_COMMAND$COVERITY_LD_PRELOAD" && t=1 +test "x$lv_cv_static_analysis" = xyes && t=1 AC_DEFINE_UNQUOTED([STATIC_ANALYSIS], [$t], [Define to 1 when performing static analysis.]) -- 1.7.4.4
On 08/02/2011 02:31 PM, Eric Blake wrote:
Right now, every re-run of configure re-evaluates whether a static analysis tool is in use. But if you run configure under coverity, make a tweak, and then do an incremental rebuild with gcc but not coverity to test the tweak, then rerun a build under coverity, then configure does not get rerun, and static analysis ends up with lots of false positives.
This patch caches the static analysis result, and also makes it easier to force static analysis even if the existing checks are insufficient to detect newer versions of the static analyzer tools.
* configure.ac (lv_cv_static_analysis): New cache variable. ---
I _thought_ some of those false positives were looking familiar from my last coverity run. Turns out I inadvertantly managed to lose my STATIC_ANALYSIS define.
configure.ac | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/configure.ac b/configure.ac index 7c4fb32..10487e5 100644 --- a/configure.ac +++ b/configure.ac @@ -2404,9 +2404,16 @@ cp -f COPYING.LIB COPYING
# Detect when running under the clang static analyzer's scan-build driver # or Coverity-prevent's cov-build. Define STATIC_ANALYSIS accordingly. +AC_CACHE_CHECK([whether this build is done by a static analysis tool], + [lv_cv_static_analysis], [ + lv_cv_static_analysis=no + if test -n "${CCC_ANALYZER_ANALYSIS+set}" || \ + test -n "$COVERITY_BUILD_COMMAND$COVERITY_LD_PRELOAD"; then + lv_cv_static_analysis=yes + fi + ]) t=0 -test -n "${CCC_ANALYZER_ANALYSIS+set}"&& t=1 -test -n "$COVERITY_BUILD_COMMAND$COVERITY_LD_PRELOAD"&& t=1 +test "x$lv_cv_static_analysis" = xyes&& t=1 AC_DEFINE_UNQUOTED([STATIC_ANALYSIS], [$t], [Define to 1 when performing static analysis.])
ACK.
Detected by Coverity. We want to compare the result of fnmatch 'rv', not our pre-set return value 'ret'. * src/rpc/virnetsaslcontext.c (virNetSASLContextCheckIdentity): Check correct variable. --- src/rpc/virnetsaslcontext.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/rpc/virnetsaslcontext.c b/src/rpc/virnetsaslcontext.c index a0752dd..ef36e2c 100644 --- a/src/rpc/virnetsaslcontext.c +++ b/src/rpc/virnetsaslcontext.c @@ -130,11 +130,11 @@ int virNetSASLContextCheckIdentity(virNetSASLContextPtr ctxt, int rv = fnmatch (*wildcards, identity, 0); if (rv == 0) { ret = 1; goto cleanup; /* Succesful match */ } - if (ret != FNM_NOMATCH) { + if (rv != FNM_NOMATCH) { virNetError(VIR_ERR_INTERNAL_ERROR, _("Malformed TLS whitelist regular expression '%s'"), *wildcards); goto cleanup; } -- 1.7.4.4
On 08/02/2011 03:06 PM, Eric Blake wrote:
Detected by Coverity. We want to compare the result of fnmatch 'rv', not our pre-set return value 'ret'.
* src/rpc/virnetsaslcontext.c (virNetSASLContextCheckIdentity): Check correct variable. --- src/rpc/virnetsaslcontext.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnetsaslcontext.c b/src/rpc/virnetsaslcontext.c index a0752dd..ef36e2c 100644 --- a/src/rpc/virnetsaslcontext.c +++ b/src/rpc/virnetsaslcontext.c @@ -130,11 +130,11 @@ int virNetSASLContextCheckIdentity(virNetSASLContextPtr ctxt, int rv = fnmatch (*wildcards, identity, 0); if (rv == 0) { ret = 1; goto cleanup; /* Succesful match */ } - if (ret != FNM_NOMATCH) { + if (rv != FNM_NOMATCH) { virNetError(VIR_ERR_INTERNAL_ERROR, _("Malformed TLS whitelist regular expression '%s'"), *wildcards); goto cleanup; }
ACK.
On Tue, Aug 02, 2011 at 13:06:44 -0600, Eric Blake wrote:
Detected by Coverity. We want to compare the result of fnmatch 'rv', not our pre-set return value 'ret'.
* src/rpc/virnetsaslcontext.c (virNetSASLContextCheckIdentity): Check correct variable. --- src/rpc/virnetsaslcontext.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ACK Jirka
Coverity detected that 5 of 6 callers of virJSONValueArrayGet checked for a NULL return; and that by not checking we risk a null deref during an error. The error is unlikely since the prior call to virJSONValueArraySize would probably have already caught any botched JSON array parse, but better safe than sorry. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONGetBlockJobInfo): Check for NULL. (qemuMonitorJSONExtractPtyPaths): Fix typo. --- src/qemu/qemu_monitor_json.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index b7a6a12..2a9a078 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1018,7 +1018,7 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply, int thread; if (!entry) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("character device information was missing aray element")); + _("character device information was missing array element")); goto cleanup; } @@ -2266,7 +2266,7 @@ static int qemuMonitorJSONExtractPtyPaths(virJSONValuePtr reply, const char *id; if (!entry) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("character device information was missing aray element")); + _("character device information was missing array element")); goto cleanup; } @@ -2855,6 +2855,11 @@ static int qemuMonitorJSONGetBlockJobInfo(virJSONValuePtr reply, for (i = 0; i < nr_results; i++) { virJSONValuePtr entry = virJSONValueArrayGet(data, i); + if (!entry) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing array element")); + return -1; + } if (qemuMonitorJSONGetBlockJobInfoOne(entry, device, info) == 0) return 1; } -- 1.7.4.4
On 08/02/2011 03:21 PM, Eric Blake wrote:
Coverity detected that 5 of 6 callers of virJSONValueArrayGet checked for a NULL return; and that by not checking we risk a null deref during an error. The error is unlikely since the prior call to virJSONValueArraySize would probably have already caught any botched JSON array parse, but better safe than sorry.
ACK.
Detected by Coverity. Freeing the wrong variable results in both a memory leak and the likelihood of the caller dereferencing through a freed pointer. * src/rpc/virnettlscontext.c (virNetTLSSessionNew): Free correct variable. --- src/rpc/virnettlscontext.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index be08207..eeffe54 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -1164,17 +1164,17 @@ virNetTLSSessionPtr virNetTLSSessionNew(virNetTLSContextPtr ctxt, if (VIR_ALLOC(sess) < 0) { virReportOOMError(); return NULL; } if (virMutexInit(&sess->lock) < 0) { virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to initialized mutex")); - VIR_FREE(ctxt); + VIR_FREE(sess); return NULL; } sess->refs = 1; if (hostname && !(sess->hostname = strdup(hostname))) { virReportOOMError(); goto error; -- 1.7.4.4
On 08/02/2011 03:38 PM, Eric Blake wrote:
Detected by Coverity. Freeing the wrong variable results in both a memory leak and the likelihood of the caller dereferencing through a freed pointer.
* src/rpc/virnettlscontext.c (virNetTLSSessionNew): Free correct variable. --- src/rpc/virnettlscontext.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index be08207..eeffe54 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -1164,17 +1164,17 @@ virNetTLSSessionPtr virNetTLSSessionNew(virNetTLSContextPtr ctxt, if (VIR_ALLOC(sess)< 0) { virReportOOMError(); return NULL; }
if (virMutexInit(&sess->lock)< 0) { virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to initialized mutex")); - VIR_FREE(ctxt); + VIR_FREE(sess); return NULL;
You could just as well replace this with a goto error, but ACK anyway.
}
sess->refs = 1; if (hostname&& !(sess->hostname = strdup(hostname))) { virReportOOMError(); goto error;
Detected by Coverity. Introduced in commit 85aa40e. * src/conf/domain_conf.c (virDomainDiskDefForeachPath): Plug leak. --- src/conf/domain_conf.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e182cd6..010ce57 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11315,7 +11315,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, virDomainDiskDefPathIterator iter, void *opaque) { - virHashTablePtr paths; + virHashTablePtr paths = NULL; int format; int ret = -1; size_t depth = 0; @@ -11339,7 +11339,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unknown disk format '%s' for %s"), disk->driverType, disk->src); - return -1; + goto cleanup; } } else { if (allowProbing) { @@ -11348,7 +11348,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("no disk format for %s and probing is disabled"), disk->src); - return -1; + goto cleanup; } } -- 1.7.4.4
On 08/02/2011 03:50 PM, Eric Blake wrote:
Detected by Coverity. Introduced in commit 85aa40e.
leak of meta - important point, since the thing being leaked doesn't show up anywhere in the diff. ACK
* src/conf/domain_conf.c (virDomainDiskDefForeachPath): Plug leak. --- src/conf/domain_conf.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e182cd6..010ce57 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11315,7 +11315,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, virDomainDiskDefPathIterator iter, void *opaque) { - virHashTablePtr paths; + virHashTablePtr paths = NULL; int format; int ret = -1; size_t depth = 0; @@ -11339,7 +11339,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unknown disk format '%s' for %s"), disk->driverType, disk->src); - return -1; + goto cleanup; } } else { if (allowProbing) { @@ -11348,7 +11348,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("no disk format for %s and probing is disabled"), disk->src); - return -1; + goto cleanup; } }
On 08/02/2011 02:53 PM, Laine Stump wrote:
On 08/02/2011 03:50 PM, Eric Blake wrote:
Detected by Coverity. Introduced in commit 85aa40e.
leak of meta - important point, since the thing being leaked doesn't show up anywhere in the diff.
ACK
Good point. I've now pushed 4-8, with this commit comment for 8: conf: avoid memory leak on disk operations Detected by Coverity. Leak on meta introduced in commit 85aa40e. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Quite a few leaks detected by coverity. For chr, the leaks were close enough to the allocations to plug in place; for disk, the leaks were separated from the allocation by enough other lines with intermediate failure cases that I refactored the cleanup instead. * src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks. --- src/qemu/qemu_command.c | 23 ++++++++++++++++------- 1 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6a2e2ae..c638420 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5938,6 +5938,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, int video = VIR_DOMAIN_VIDEO_TYPE_CIRRUS; int nvirtiodisk = 0; qemuDomainCmdlineDefPtr cmd = NULL; + virDomainDiskDefPtr disk = NULL; if (pidfile) *pidfile = NULL; @@ -6124,7 +6125,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, STRPREFIX(arg, "-fd") || STREQ(arg, "-cdrom")) { WANT_VALUE(); - virDomainDiskDefPtr disk; if (VIR_ALLOC(disk) < 0) goto no_memory; @@ -6239,6 +6239,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } def->disks[def->ndisks++] = disk; + disk = NULL; } else if (STREQ(arg, "-no-acpi")) { def->features &= ~(1 << VIR_DOMAIN_FEATURE_ACPI); } else if (STREQ(arg, "-no-reboot")) { @@ -6307,8 +6308,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (!(chr = virDomainChrDefNew())) goto error; - if (qemuParseCommandLineChr(&chr->source, val) < 0) + if (qemuParseCommandLineChr(&chr->source, val) < 0) { + virDomainChrDefFree(chr); goto error; + } if (VIR_REALLOC_N(def->serials, def->nserials+1) < 0) { virDomainChrDefFree(chr); goto no_memory; @@ -6325,8 +6328,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (!(chr = virDomainChrDefNew())) goto error; - if (qemuParseCommandLineChr(&chr->source, val) < 0) + if (qemuParseCommandLineChr(&chr->source, val) < 0) { + virDomainChrDefFree(chr); goto error; + } if (VIR_REALLOC_N(def->parallels, def->nparallels+1) < 0) { virDomainChrDefFree(chr); goto no_memory; @@ -6353,7 +6358,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, } def->inputs[def->ninputs++] = input; } else if (STRPREFIX(val, "disk:")) { - virDomainDiskDefPtr disk; if (VIR_ALLOC(disk) < 0) goto no_memory; disk->src = strdup(val + strlen("disk:")); @@ -6376,6 +6380,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } def->disks[def->ndisks++] = disk; + disk = NULL; } else { virDomainHostdevDefPtr hostdev; if (!(hostdev = qemuParseCommandLineUSB(val))) @@ -6399,7 +6404,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, def->nets[def->nnets++] = net; } } else if (STREQ(arg, "-drive")) { - virDomainDiskDefPtr disk; WANT_VALUE(); if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk))) goto error; @@ -6408,6 +6412,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } def->disks[def->ndisks++] = disk; + disk = NULL; if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) nvirtiodisk++; @@ -6516,8 +6521,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (VIR_ALLOC(chr) < 0) goto no_memory; - if (qemuParseCommandLineChr(chr, val) < 0) + if (qemuParseCommandLineChr(chr, val) < 0) { + VIR_FREE(chr); goto error; + } *monConfig = chr; } @@ -6545,10 +6552,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, char *hosts, *port, *saveptr = NULL, *token; virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0 ; i < def->ndisks ; i++) { - virDomainDiskDefPtr disk = def->disks[i]; + disk = def->disks[i]; if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { first_rbd_disk = disk; + disk = NULL; break; } } @@ -6676,6 +6684,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, no_memory: virReportOOMError(); error: + VIR_FREE(disk); VIR_FREE(cmd); virDomainDefFree(def); VIR_FREE(nics); -- 1.7.4.4
On 08/02/2011 04:10 PM, Eric Blake wrote:
Quite a few leaks detected by coverity. For chr, the leaks were close enough to the allocations to plug in place; for disk, the leaks were separated from the allocation by enough other lines with intermediate failure cases that I refactored the cleanup instead.
* src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks. --- src/qemu/qemu_command.c | 23 ++++++++++++++++------- 1 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6a2e2ae..c638420 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5938,6 +5938,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, int video = VIR_DOMAIN_VIDEO_TYPE_CIRRUS; int nvirtiodisk = 0; qemuDomainCmdlineDefPtr cmd = NULL; + virDomainDiskDefPtr disk = NULL;
if (pidfile) *pidfile = NULL; @@ -6124,7 +6125,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, STRPREFIX(arg, "-fd") || STREQ(arg, "-cdrom")) { WANT_VALUE(); - virDomainDiskDefPtr disk; if (VIR_ALLOC(disk)< 0) goto no_memory;
@@ -6239,6 +6239,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } def->disks[def->ndisks++] = disk; + disk = NULL; } else if (STREQ(arg, "-no-acpi")) { def->features&= ~(1<< VIR_DOMAIN_FEATURE_ACPI); } else if (STREQ(arg, "-no-reboot")) { @@ -6307,8 +6308,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (!(chr = virDomainChrDefNew())) goto error;
- if (qemuParseCommandLineChr(&chr->source, val)< 0) + if (qemuParseCommandLineChr(&chr->source, val)< 0) { + virDomainChrDefFree(chr); goto error; + } if (VIR_REALLOC_N(def->serials, def->nserials+1)< 0) { virDomainChrDefFree(chr); goto no_memory; @@ -6325,8 +6328,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (!(chr = virDomainChrDefNew())) goto error;
- if (qemuParseCommandLineChr(&chr->source, val)< 0) + if (qemuParseCommandLineChr(&chr->source, val)< 0) { + virDomainChrDefFree(chr); goto error; + } if (VIR_REALLOC_N(def->parallels, def->nparallels+1)< 0) { virDomainChrDefFree(chr); goto no_memory; @@ -6353,7 +6358,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, } def->inputs[def->ninputs++] = input; } else if (STRPREFIX(val, "disk:")) { - virDomainDiskDefPtr disk; if (VIR_ALLOC(disk)< 0) goto no_memory; disk->src = strdup(val + strlen("disk:")); @@ -6376,6 +6380,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } def->disks[def->ndisks++] = disk; + disk = NULL; } else { virDomainHostdevDefPtr hostdev; if (!(hostdev = qemuParseCommandLineUSB(val))) @@ -6399,7 +6404,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, def->nets[def->nnets++] = net; } } else if (STREQ(arg, "-drive")) { - virDomainDiskDefPtr disk; WANT_VALUE(); if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk))) goto error; @@ -6408,6 +6412,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } def->disks[def->ndisks++] = disk; + disk = NULL;
if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) nvirtiodisk++; @@ -6516,8 +6521,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (VIR_ALLOC(chr)< 0) goto no_memory;
- if (qemuParseCommandLineChr(chr, val)< 0) + if (qemuParseCommandLineChr(chr, val)< 0) { + VIR_FREE(chr);
Shouldn't this be virDomainChrDefFree(chr)?
goto error; + }
*monConfig = chr; } @@ -6545,10 +6552,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, char *hosts, *port, *saveptr = NULL, *token; virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0 ; i< def->ndisks ; i++) { - virDomainDiskDefPtr disk = def->disks[i]; + disk = def->disks[i]; if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK&& disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { first_rbd_disk = disk; + disk = NULL; break; }
If you never hit the if condition, disk will be left pointing to one of the disks in def, and will be freed during cleanup. I think you want to set disk = NULL after this look is finished as well, don't you? (Either that, or just use a different variable).
} @@ -6676,6 +6684,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, no_memory: virReportOOMError(); error: + VIR_FREE(disk); VIR_FREE(cmd); virDomainDefFree(def); VIR_FREE(nics);
On 08/02/2011 02:48 PM, Laine Stump wrote:
On 08/02/2011 04:10 PM, Eric Blake wrote:
Quite a few leaks detected by coverity. For chr, the leaks were close enough to the allocations to plug in place; for disk, the leaks were separated from the allocation by enough other lines with intermediate failure cases that I refactored the cleanup instead.
- if (qemuParseCommandLineChr(chr, val)< 0) + if (qemuParseCommandLineChr(chr, val)< 0) { + VIR_FREE(chr);
Shouldn't this be virDomainChrDefFree(chr)?
virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0 ; i< def->ndisks ; i++) { - virDomainDiskDefPtr disk = def->disks[i]; + disk = def->disks[i]; if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK&& disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { first_rbd_disk = disk; + disk = NULL; break; }
If you never hit the if condition, disk will be left pointing to one of the disks in def, and will be freed during cleanup. I think you want to set disk = NULL after this look is finished as well, don't you? (Either that, or just use a different variable).
Both good points. I'm squashing this in to create v2. diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index c638420..194f3ff 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -6522,7 +6522,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; if (qemuParseCommandLineChr(chr, val) < 0) { - VIR_FREE(chr); + virDomainChrSourceDefFree(chr); goto error; } @@ -6552,11 +6552,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, char *hosts, *port, *saveptr = NULL, *token; virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0 ; i < def->ndisks ; i++) { - disk = def->disks[i]; - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && - disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { - first_rbd_disk = disk; - disk = NULL; + if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK && + def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { + first_rbd_disk = def->disks[i]; break; } } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/02/2011 05:46 PM, Eric Blake wrote:
On 08/02/2011 02:48 PM, Laine Stump wrote:
On 08/02/2011 04:10 PM, Eric Blake wrote:
Quite a few leaks detected by coverity. For chr, the leaks were close enough to the allocations to plug in place; for disk, the leaks were separated from the allocation by enough other lines with intermediate failure cases that I refactored the cleanup instead.
- if (qemuParseCommandLineChr(chr, val)< 0) + if (qemuParseCommandLineChr(chr, val)< 0) { + VIR_FREE(chr);
Shouldn't this be virDomainChrDefFree(chr)?
virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0 ; i< def->ndisks ; i++) { - virDomainDiskDefPtr disk = def->disks[i]; + disk = def->disks[i]; if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK&& disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { first_rbd_disk = disk; + disk = NULL; break; }
If you never hit the if condition, disk will be left pointing to one of the disks in def, and will be freed during cleanup. I think you want to set disk = NULL after this look is finished as well, don't you? (Either that, or just use a different variable).
Both good points. I'm squashing this in to create v2.
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index c638420..194f3ff 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -6522,7 +6522,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory;
if (qemuParseCommandLineChr(chr, val) < 0) { - VIR_FREE(chr); + virDomainChrSourceDefFree(chr); goto error; }
@@ -6552,11 +6552,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, char *hosts, *port, *saveptr = NULL, *token; virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0 ; i < def->ndisks ; i++) { - disk = def->disks[i]; - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && - disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { - first_rbd_disk = disk; - disk = NULL; + if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK && + def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { + first_rbd_disk = def->disks[i]; break; } }
ACK with those changes.
Warning detected by Coverity. No need for the NULL check, and removing it silences the warning without any semantic change. * src/qemu/qemu_migration.c (qemuMigrationFinish): All entries to endjob had non-NULL vm. --- Oops - I forgot to label the subject for 9/3 properly. src/qemu/qemu_migration.c | 12 +++++------- 1 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8bdbcaf..7aeea69 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2569,13 +2569,11 @@ qemuMigrationFinish(struct qemud_driver *driver, VIR_WARN("Unable to encode migration cookie"); endjob: - if (vm) { - if (qemuMigrationJobFinish(driver, vm) == 0) { - vm = NULL; - } else if (!vm->persistent && !virDomainObjIsActive(vm)) { - virDomainRemoveInactive(&driver->domains, vm); - vm = NULL; - } + if (qemuMigrationJobFinish(driver, vm) == 0) { + vm = NULL; + } else if (!vm->persistent && !virDomainObjIsActive(vm)) { + virDomainRemoveInactive(&driver->domains, vm); + vm = NULL; } cleanup: -- 1.7.4.4
On 08/02/2011 04:20 PM, Eric Blake wrote:
Warning detected by Coverity. No need for the NULL check, and removing it silences the warning without any semantic change.
True. The first function called in qemuMigrationFinish dereferences vm, so we would have crashed long before if it was NULL. ACK.
Detected by Coverity. Leak introduced by typo in commit 58e668d2. * src/qemu/qemu_driver.c (doCoreDump): Use correct function. --- src/qemu/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0297159..ea24df8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2717,7 +2717,7 @@ doCoreDump(struct qemud_driver *driver, cleanup: VIR_FORCE_CLOSE(fd); - virFileDirectFdClose(directFd); + virFileDirectFdFree(directFd); if (ret != 0) unlink(path); return ret; -- 1.7.4.4
On 08/02/2011 04:38 PM, Eric Blake wrote:
Detected by Coverity. Leak introduced by typo in commit 58e668d2.
* src/qemu/qemu_driver.c (doCoreDump): Use correct function. --- src/qemu/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0297159..ea24df8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2717,7 +2717,7 @@ doCoreDump(struct qemud_driver *driver,
cleanup: VIR_FORCE_CLOSE(fd); - virFileDirectFdClose(directFd); + virFileDirectFdFree(directFd); if (ret != 0) unlink(path); return ret;
ACK.
On 08/02/2011 02:59 PM, Laine Stump wrote:
On 08/02/2011 04:38 PM, Eric Blake wrote:
Detected by Coverity. Leak introduced by typo in commit 58e668d2.
* src/qemu/qemu_driver.c (doCoreDump): Use correct function. --- src/qemu/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0297159..ea24df8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2717,7 +2717,7 @@ doCoreDump(struct qemud_driver *driver,
cleanup: VIR_FORCE_CLOSE(fd); - virFileDirectFdClose(directFd); + virFileDirectFdFree(directFd); if (ret != 0) unlink(path); return ret;
ACK.
I've pushed 10 and 11; I'll respin 9 to address your comments. More still probably on the way... -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Quite a few leaks detected by coverity. For chr, the leaks were close enough to the allocations to plug in place; for disk, the leaks were separated from the allocation by enough other lines with intermediate failure cases that I refactored the cleanup instead. * src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks. --- v2: address review comments from v1 src/qemu/qemu_command.c | 27 +++++++++++++++++---------- 1 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6a2e2ae..194f3ff 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5938,6 +5938,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, int video = VIR_DOMAIN_VIDEO_TYPE_CIRRUS; int nvirtiodisk = 0; qemuDomainCmdlineDefPtr cmd = NULL; + virDomainDiskDefPtr disk = NULL; if (pidfile) *pidfile = NULL; @@ -6124,7 +6125,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, STRPREFIX(arg, "-fd") || STREQ(arg, "-cdrom")) { WANT_VALUE(); - virDomainDiskDefPtr disk; if (VIR_ALLOC(disk) < 0) goto no_memory; @@ -6239,6 +6239,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } def->disks[def->ndisks++] = disk; + disk = NULL; } else if (STREQ(arg, "-no-acpi")) { def->features &= ~(1 << VIR_DOMAIN_FEATURE_ACPI); } else if (STREQ(arg, "-no-reboot")) { @@ -6307,8 +6308,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (!(chr = virDomainChrDefNew())) goto error; - if (qemuParseCommandLineChr(&chr->source, val) < 0) + if (qemuParseCommandLineChr(&chr->source, val) < 0) { + virDomainChrDefFree(chr); goto error; + } if (VIR_REALLOC_N(def->serials, def->nserials+1) < 0) { virDomainChrDefFree(chr); goto no_memory; @@ -6325,8 +6328,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (!(chr = virDomainChrDefNew())) goto error; - if (qemuParseCommandLineChr(&chr->source, val) < 0) + if (qemuParseCommandLineChr(&chr->source, val) < 0) { + virDomainChrDefFree(chr); goto error; + } if (VIR_REALLOC_N(def->parallels, def->nparallels+1) < 0) { virDomainChrDefFree(chr); goto no_memory; @@ -6353,7 +6358,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, } def->inputs[def->ninputs++] = input; } else if (STRPREFIX(val, "disk:")) { - virDomainDiskDefPtr disk; if (VIR_ALLOC(disk) < 0) goto no_memory; disk->src = strdup(val + strlen("disk:")); @@ -6376,6 +6380,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } def->disks[def->ndisks++] = disk; + disk = NULL; } else { virDomainHostdevDefPtr hostdev; if (!(hostdev = qemuParseCommandLineUSB(val))) @@ -6399,7 +6404,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, def->nets[def->nnets++] = net; } } else if (STREQ(arg, "-drive")) { - virDomainDiskDefPtr disk; WANT_VALUE(); if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk))) goto error; @@ -6408,6 +6412,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } def->disks[def->ndisks++] = disk; + disk = NULL; if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) nvirtiodisk++; @@ -6516,8 +6521,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (VIR_ALLOC(chr) < 0) goto no_memory; - if (qemuParseCommandLineChr(chr, val) < 0) + if (qemuParseCommandLineChr(chr, val) < 0) { + virDomainChrSourceDefFree(chr); goto error; + } *monConfig = chr; } @@ -6545,10 +6552,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, char *hosts, *port, *saveptr = NULL, *token; virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0 ; i < def->ndisks ; i++) { - virDomainDiskDefPtr disk = def->disks[i]; - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && - disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { - first_rbd_disk = disk; + if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK && + def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { + first_rbd_disk = def->disks[i]; break; } } @@ -6676,6 +6682,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, no_memory: virReportOOMError(); error: + VIR_FREE(disk); VIR_FREE(cmd); virDomainDefFree(def); VIR_FREE(nics); -- 1.7.4.4
On 08/02/2011 03:46 PM, Eric Blake wrote:
Quite a few leaks detected by coverity. For chr, the leaks were close enough to the allocations to plug in place; for disk, the leaks were separated from the allocation by enough other lines with intermediate failure cases that I refactored the cleanup instead.
* src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks. ---
v2: address review comments from v1
squash this in too (serves me right for hitting send prior to 'make check' completing). diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 194f3ff..067e53e 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -6411,11 +6411,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, virDomainDiskDefFree(disk); goto no_memory; } - def->disks[def->ndisks++] = disk; - disk = NULL; - if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) nvirtiodisk++; + + def->disks[def->ndisks++] = disk; + disk = NULL; } else if (STREQ(arg, "-pcidevice")) { virDomainHostdevDefPtr hostdev; WANT_VALUE(); -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/02/2011 03:50 PM, Eric Blake wrote:
On 08/02/2011 03:46 PM, Eric Blake wrote:
Quite a few leaks detected by coverity. For chr, the leaks were close enough to the allocations to plug in place; for disk, the leaks were separated from the allocation by enough other lines with intermediate failure cases that I refactored the cleanup instead.
* src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks. ---
v2: address review comments from v1
squash this in too (serves me right for hitting send prior to 'make check' completing).
Yet more to squash in. diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 067e53e..76df0aa 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -6226,18 +6226,14 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, } if (!(disk->src || disk->nhosts > 0) || - !disk->dst) { - virDomainDiskDefFree(disk); + !disk->dst) goto no_memory; - } if (virDomainDiskDefAssignAddress(caps, disk) < 0) goto error; - if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) { - virDomainDiskDefFree(disk); + if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) goto no_memory; - } def->disks[def->ndisks++] = disk; disk = NULL; } else if (STREQ(arg, "-no-acpi")) { @@ -6361,24 +6357,17 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (VIR_ALLOC(disk) < 0) goto no_memory; disk->src = strdup(val + strlen("disk:")); - if (!disk->src) { - virDomainDiskDefFree(disk); + if (!disk->src) goto no_memory; - } if (STRPREFIX(disk->src, "/dev/")) disk->type = VIR_DOMAIN_DISK_TYPE_BLOCK; else disk->type = VIR_DOMAIN_DISK_TYPE_FILE; disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; disk->bus = VIR_DOMAIN_DISK_BUS_USB; - if (!(disk->dst = strdup("sda"))) { - virDomainDiskDefFree(disk); + if (!(disk->dst = strdup("sda")) || + VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) goto no_memory; - } - if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) { - virDomainDiskDefFree(disk); - goto no_memory; - } def->disks[def->ndisks++] = disk; disk = NULL; } else { @@ -6407,10 +6396,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, WANT_VALUE(); if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk))) goto error; - if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) { - virDomainDiskDefFree(disk); + if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) goto no_memory; - } if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) nvirtiodisk++; @@ -6682,7 +6669,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, no_memory: virReportOOMError(); error: - VIR_FREE(disk); + virDomainDiskDefFree(disk); VIR_FREE(cmd); virDomainDefFree(def); VIR_FREE(nics); -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/02/2011 05:46 PM, Eric Blake wrote:
Quite a few leaks detected by coverity. For chr, the leaks were close enough to the allocations to plug in place; for disk, the leaks were separated from the allocation by enough other lines with intermediate failure cases that I refactored the cleanup instead.
Sorry, two more problems I didn't notice before: 1) instead of VIR_FREE(disk) down at the bottom, you should be calling virDomainDiskDefFree() 2) There are some places in that qemuParseCommandLine where virDomainDiskDefFree() is called right before goto no_memory (or goto error), but since virDomainDiskDefFree() doesn't/can't set disk to NULL, you'll end up with a double free.
* src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks. ---
v2: address review comments from v1
src/qemu/qemu_command.c | 27 +++++++++++++++++---------- 1 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6a2e2ae..194f3ff 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5938,6 +5938,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, int video = VIR_DOMAIN_VIDEO_TYPE_CIRRUS; int nvirtiodisk = 0; qemuDomainCmdlineDefPtr cmd = NULL; + virDomainDiskDefPtr disk = NULL;
if (pidfile) *pidfile = NULL; @@ -6124,7 +6125,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, STRPREFIX(arg, "-fd") || STREQ(arg, "-cdrom")) { WANT_VALUE(); - virDomainDiskDefPtr disk; if (VIR_ALLOC(disk)< 0) goto no_memory;
@@ -6239,6 +6239,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } def->disks[def->ndisks++] = disk; + disk = NULL; } else if (STREQ(arg, "-no-acpi")) { def->features&= ~(1<< VIR_DOMAIN_FEATURE_ACPI); } else if (STREQ(arg, "-no-reboot")) { @@ -6307,8 +6308,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (!(chr = virDomainChrDefNew())) goto error;
- if (qemuParseCommandLineChr(&chr->source, val)< 0) + if (qemuParseCommandLineChr(&chr->source, val)< 0) { + virDomainChrDefFree(chr); goto error; + } if (VIR_REALLOC_N(def->serials, def->nserials+1)< 0) { virDomainChrDefFree(chr); goto no_memory; @@ -6325,8 +6328,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (!(chr = virDomainChrDefNew())) goto error;
- if (qemuParseCommandLineChr(&chr->source, val)< 0) + if (qemuParseCommandLineChr(&chr->source, val)< 0) { + virDomainChrDefFree(chr); goto error; + } if (VIR_REALLOC_N(def->parallels, def->nparallels+1)< 0) { virDomainChrDefFree(chr); goto no_memory; @@ -6353,7 +6358,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, } def->inputs[def->ninputs++] = input; } else if (STRPREFIX(val, "disk:")) { - virDomainDiskDefPtr disk; if (VIR_ALLOC(disk)< 0) goto no_memory; disk->src = strdup(val + strlen("disk:")); @@ -6376,6 +6380,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } def->disks[def->ndisks++] = disk; + disk = NULL; } else { virDomainHostdevDefPtr hostdev; if (!(hostdev = qemuParseCommandLineUSB(val))) @@ -6399,7 +6404,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, def->nets[def->nnets++] = net; } } else if (STREQ(arg, "-drive")) { - virDomainDiskDefPtr disk; WANT_VALUE(); if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk))) goto error; @@ -6408,6 +6412,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } def->disks[def->ndisks++] = disk; + disk = NULL;
if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) nvirtiodisk++; @@ -6516,8 +6521,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (VIR_ALLOC(chr)< 0) goto no_memory;
- if (qemuParseCommandLineChr(chr, val)< 0) + if (qemuParseCommandLineChr(chr, val)< 0) { + virDomainChrSourceDefFree(chr); goto error; + }
*monConfig = chr; } @@ -6545,10 +6552,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, char *hosts, *port, *saveptr = NULL, *token; virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0 ; i< def->ndisks ; i++) { - virDomainDiskDefPtr disk = def->disks[i]; - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK&& - disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { - first_rbd_disk = disk; + if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK&& + def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { + first_rbd_disk = def->disks[i]; break; } } @@ -6676,6 +6682,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, no_memory: virReportOOMError(); error: + VIR_FREE(disk); VIR_FREE(cmd); virDomainDefFree(def); VIR_FREE(nics);
Coverity complained that 395 out of 409 virAsprintf calls are checked, and therefore assumed that the remaining cases are bugs waiting to happen. But in each of these cases, a failed virAsprintf will properly set the target string to NULL, and pass on that failure to the caller, without wasting efforts to check the call. Adding the ignore_value silences Coverity. * src/conf/domain_audit.c (virDomainAuditGetRdev): Ignore virAsprintf return value, when it behaves like we need. * src/network/bridge_driver.c (networkDnsmasqLeaseFileNameDefault) (networkRadvdConfigFileName, networkBridgeDummyNicName) (networkRadvdPidfileBasename): Likewise. * src/util/storage_file.c (absolutePathFromBaseFile): Likewise. * src/openvz/openvz_driver.c (openvzGenerateContainerVethName): Likewise. * src/util/command.c (virCommandTranslateStatus): Likewise. --- src/conf/domain_audit.c | 2 +- src/network/bridge_driver.c | 22 ++++++++++++---------- src/openvz/openvz_driver.c | 2 +- src/util/command.c | 8 +++++--- src/util/storage_file.c | 2 +- 5 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 963eecb..9d89c94 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -46,7 +46,7 @@ virDomainAuditGetRdev(const char *path) (S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode))) { int maj = major(sb.st_rdev); int min = minor(sb.st_rdev); - virAsprintf(&ret, "%02X:%02X", maj, min); + ignore_value(virAsprintf(&ret, "%02X:%02X", maj, min)); } return ret; } diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0a60bb8..c7d2dfd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -60,6 +60,7 @@ #include "dnsmasq.h" #include "util/network.h" #include "configmake.h" +#include "ignore-value.h" #define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network" #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network" @@ -125,8 +126,8 @@ networkDnsmasqLeaseFileNameDefault(const char *netname) { char *leasefile; - virAsprintf(&leasefile, DNSMASQ_STATE_DIR "/%s.leases", - netname); + ignore_value(virAsprintf(&leasefile, DNSMASQ_STATE_DIR "/%s.leases", + netname)); return leasefile; } @@ -139,7 +140,7 @@ networkRadvdPidfileBasename(const char *netname) /* this is simple but we want to be sure it's consistently done */ char *pidfilebase; - virAsprintf(&pidfilebase, "%s-radvd", netname); + ignore_value(virAsprintf(&pidfilebase, "%s-radvd", netname)); return pidfilebase; } @@ -148,8 +149,8 @@ networkRadvdConfigFileName(const char *netname) { char *configfile; - virAsprintf(&configfile, RADVD_STATE_DIR "/%s-radvd.conf", - netname); + ignore_value(virAsprintf(&configfile, RADVD_STATE_DIR "/%s-radvd.conf", + netname)); return configfile; } @@ -166,12 +167,13 @@ networkBridgeDummyNicName(const char *brname) * a possible numeric ending (eg virbr0, virbr1, etc), we grab * the first 8 and last 3 characters of the string. */ - virAsprintf(&nicname, "%.*s%s%s", - /* space for last 3 chars + "-nic" + NULL */ - (int)(IFNAMSIZ - (3 + sizeof(dummyNicSuffix))), - brname, brname + strlen(brname) - 3, dummyNicSuffix); + ignore_value(virAsprintf(&nicname, "%.*s%s%s", + /* space for last 3 chars + "-nic" + NULL */ + (int)(IFNAMSIZ - (3 + sizeof(dummyNicSuffix))), + brname, brname + strlen(brname) - 3, + dummyNicSuffix)); } else { - virAsprintf(&nicname, "%s%s", brname, dummyNicSuffix); + ignore_value(virAsprintf(&nicname, "%s%s", brname, dummyNicSuffix)); } return nicname; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index df2079e..b9dc712 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -710,7 +710,7 @@ openvzGenerateContainerVethName(int veid) } /* set new name */ - virAsprintf(&name, "eth%d", max + 1); + ignore_value(virAsprintf(&name, "eth%d", max + 1)); } VIR_FREE(temp); diff --git a/src/util/command.c b/src/util/command.c index 475eb62..26fcb28 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -1543,11 +1543,13 @@ virCommandTranslateStatus(int status) { char *buf; if (WIFEXITED(status)) { - virAsprintf(&buf, _("exit status %d"), WEXITSTATUS(status)); + ignore_value(virAsprintf(&buf, _("exit status %d"), + WEXITSTATUS(status))); } else if (WIFSIGNALED(status)) { - virAsprintf(&buf, _("fatal signal %d"), WTERMSIG(status)); + ignore_value(virAsprintf(&buf, _("fatal signal %d"), + WTERMSIG(status))); } else { - virAsprintf(&buf, _("invalid value %d"), status); + ignore_value(virAsprintf(&buf, _("invalid value %d"), status)); } return buf; } diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 68e82a9..f33ea74 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -512,7 +512,7 @@ absolutePathFromBaseFile(const char *base_file, const char *path) if (d_len > INT_MAX) return NULL; - virAsprintf(&res, "%.*s/%s", (int) d_len, base_file, path); + ignore_value(virAsprintf(&res, "%.*s/%s", (int) d_len, base_file, path)); return res; } -- 1.7.4.4
On 08/02/2011 05:53 PM, Eric Blake wrote:
Coverity complained that 395 out of 409 virAsprintf calls are checked, and therefore assumed that the remaining cases are bugs waiting to happen. But in each of these cases, a failed virAsprintf will properly set the target string to NULL, and pass on that failure to the caller, without wasting efforts to check the call. Adding the ignore_value silences Coverity.
ACK.
Detected by Coverity. * src/rpc/virnetserverclient.c (virNetServerClientDispatchRead): Avoid null deref on OOM. --- src/rpc/virnetserverclient.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 2f6c040..e246fa5 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -786,9 +786,10 @@ readmore: if (client->nrequests < client->nrequests_max) { if (!(client->rx = virNetMessageNew())) { client->wantClose = true; + } else { + client->rx->bufferLength = VIR_NET_MESSAGE_LEN_MAX; + client->nrequests++; } - client->rx->bufferLength = VIR_NET_MESSAGE_LEN_MAX; - client->nrequests++; } virNetServerClientUpdateEvent(client); } -- 1.7.4.4
On 08/02/2011 05:58 PM, Eric Blake wrote:
Detected by Coverity.
* src/rpc/virnetserverclient.c (virNetServerClientDispatchRead): Avoid null deref on OOM. --- src/rpc/virnetserverclient.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 2f6c040..e246fa5 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -786,9 +786,10 @@ readmore: if (client->nrequests< client->nrequests_max) { if (!(client->rx = virNetMessageNew())) { client->wantClose = true; + } else { + client->rx->bufferLength = VIR_NET_MESSAGE_LEN_MAX; + client->nrequests++; } - client->rx->bufferLength = VIR_NET_MESSAGE_LEN_MAX; - client->nrequests++; } virNetServerClientUpdateEvent(client); }
ACK.
In virNetServerNew, Coverity didn't realize that srv->mdsnGroupName can only be non-NULL if mdsnGroupName was non-NULL. In virNetServerRun, Coverity didn't realize that the array is non-NULL if the array count is non-zero. * src/rpc/virnetserver.c (virNetServerNew): Use alternate pointer. (virNetServerRun): Give coverity a hint. --- src/rpc/virnetserver.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 4deeca1..5e4826b 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -308,7 +308,8 @@ virNetServerPtr virNetServerNew(size_t min_workers, if (srv->mdnsGroupName) { if (!(srv->mdns = virNetServerMDNSNew())) goto error; - if (!(srv->mdnsGroup = virNetServerMDNSAddGroup(srv->mdns, mdnsGroupName))) + if (!(srv->mdnsGroup = virNetServerMDNSAddGroup(srv->mdns, + srv->mdnsGroupName))) goto error; } #endif @@ -702,6 +703,9 @@ void virNetServerRun(virNetServerPtr srv) reprocess: for (i = 0 ; i < srv->nclients ; i++) { + /* Coverity 5.3.0 couldn't see that srv->clients is non-NULL + * if srv->nclients is non-zero. */ + sa_assert(srv->clients); if (virNetServerClientWantClose(srv->clients[i])) virNetServerClientClose(srv->clients[i]); if (virNetServerClientIsClosed(srv->clients[i])) { -- 1.7.4.4
On 08/02/2011 06:04 PM, Eric Blake wrote:
In virNetServerNew, Coverity didn't realize that srv->mdsnGroupName can only be non-NULL if mdsnGroupName was non-NULL.
In virNetServerRun, Coverity didn't realize that the array is non-NULL if the array count is non-zero.
ACK.
Coverity detected that ifaceGetNthParent had already dereferenced 'nth' prior to the conditional; all callers already complied with passing a non-NULL pointer so make this part of the contract. * src/util/interface.h (ifaceGetNthParent): Add annotations. * src/util/interface.c (ifaceGetNthParent): Drop useless null check. --- src/util/interface.c | 3 +-- src/util/interface.h | 5 ++++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/util/interface.c b/src/util/interface.c index f5eecfb..8b4522b 100644 --- a/src/util/interface.c +++ b/src/util/interface.c @@ -1060,8 +1060,7 @@ ifaceGetNthParent(int ifindex, const char *ifname, unsigned int nthParent, i++; } - if (nth) - *nth = i - 1; + *nth = i - 1; return rc; } diff --git a/src/util/interface.h b/src/util/interface.h index 9647653..47c0eb0 100644 --- a/src/util/interface.h +++ b/src/util/interface.h @@ -1,6 +1,7 @@ /* * interface.h: interface helper APIs for libvirt * + * Copyright (C) 2011 Red Hat, Inc. * Copyright (C) 2010 IBM Corporation, Inc. * * See COPYING.LIB for the License of this software @@ -67,7 +68,9 @@ int ifaceMacvtapLinkDump(bool nltarget_kernel, const char *ifname, int ifindex, int ifaceGetNthParent(int ifindex, const char *ifname, unsigned int nthParent, int *parent_ifindex, char *parent_ifname, - unsigned int *nth); + unsigned int *nth) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) + ATTRIBUTE_NONNULL(6); int ifaceReplaceMacAddress(const unsigned char *macaddress, const char *linkdev, -- 1.7.4.4
On 08/02/2011 06:09 PM, Eric Blake wrote:
Coverity detected that ifaceGetNthParent had already dereferenced 'nth' prior to the conditional; all callers already complied with passing a non-NULL pointer so make this part of the contract.
ACK.
Leak detected by Coverity; only possible on unlikely ptsname_r failure. Additionally, the man page for ptsname_r states that failure is merely non-zero, not necessarily -1. * src/util/util.c (virFileOpenTtyAt): Avoid leak on ptsname_r failure. --- src/util/util.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 03a9e1a..2e2a6a0 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1126,8 +1126,10 @@ int virFileOpenTtyAt(const char *ptmx, goto cleanup; } - if (ptsname_r(*ttymaster, *ttyName, PATH_MAX) < 0) + if (ptsname_r(*ttymaster, *ttyName, PATH_MAX) != 0) { + VIR_FREE(*ttyName); goto cleanup; + } } rc = 0; -- 1.7.4.4
On 08/02/2011 06:27 PM, Eric Blake wrote:
Leak detected by Coverity; only possible on unlikely ptsname_r failure. Additionally, the man page for ptsname_r states that failure is merely non-zero, not necessarily -1.
Double ACK. Two bugs in one!
Quite a few leaks detected by coverity. For chr, the leaks were close enough to the allocations to plug in place; for disk, the leaks were separated from the allocation by enough other lines with intermediate failure cases that I refactored the cleanup instead. * src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks. --- v3: squash in more fixes: free 'disk' correctly, and only once; don't reference disk after it is set to NULL src/qemu/qemu_command.c | 56 +++++++++++++++++++++-------------------------- 1 files changed, 25 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6a2e2ae..76df0aa 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5938,6 +5938,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, int video = VIR_DOMAIN_VIDEO_TYPE_CIRRUS; int nvirtiodisk = 0; qemuDomainCmdlineDefPtr cmd = NULL; + virDomainDiskDefPtr disk = NULL; if (pidfile) *pidfile = NULL; @@ -6124,7 +6125,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, STRPREFIX(arg, "-fd") || STREQ(arg, "-cdrom")) { WANT_VALUE(); - virDomainDiskDefPtr disk; if (VIR_ALLOC(disk) < 0) goto no_memory; @@ -6226,19 +6226,16 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, } if (!(disk->src || disk->nhosts > 0) || - !disk->dst) { - virDomainDiskDefFree(disk); + !disk->dst) goto no_memory; - } if (virDomainDiskDefAssignAddress(caps, disk) < 0) goto error; - if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) { - virDomainDiskDefFree(disk); + if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) goto no_memory; - } def->disks[def->ndisks++] = disk; + disk = NULL; } else if (STREQ(arg, "-no-acpi")) { def->features &= ~(1 << VIR_DOMAIN_FEATURE_ACPI); } else if (STREQ(arg, "-no-reboot")) { @@ -6307,8 +6304,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (!(chr = virDomainChrDefNew())) goto error; - if (qemuParseCommandLineChr(&chr->source, val) < 0) + if (qemuParseCommandLineChr(&chr->source, val) < 0) { + virDomainChrDefFree(chr); goto error; + } if (VIR_REALLOC_N(def->serials, def->nserials+1) < 0) { virDomainChrDefFree(chr); goto no_memory; @@ -6325,8 +6324,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (!(chr = virDomainChrDefNew())) goto error; - if (qemuParseCommandLineChr(&chr->source, val) < 0) + if (qemuParseCommandLineChr(&chr->source, val) < 0) { + virDomainChrDefFree(chr); goto error; + } if (VIR_REALLOC_N(def->parallels, def->nparallels+1) < 0) { virDomainChrDefFree(chr); goto no_memory; @@ -6353,29 +6354,22 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, } def->inputs[def->ninputs++] = input; } else if (STRPREFIX(val, "disk:")) { - virDomainDiskDefPtr disk; if (VIR_ALLOC(disk) < 0) goto no_memory; disk->src = strdup(val + strlen("disk:")); - if (!disk->src) { - virDomainDiskDefFree(disk); + if (!disk->src) goto no_memory; - } if (STRPREFIX(disk->src, "/dev/")) disk->type = VIR_DOMAIN_DISK_TYPE_BLOCK; else disk->type = VIR_DOMAIN_DISK_TYPE_FILE; disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; disk->bus = VIR_DOMAIN_DISK_BUS_USB; - if (!(disk->dst = strdup("sda"))) { - virDomainDiskDefFree(disk); - goto no_memory; - } - if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) { - virDomainDiskDefFree(disk); + if (!(disk->dst = strdup("sda")) || + VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) goto no_memory; - } def->disks[def->ndisks++] = disk; + disk = NULL; } else { virDomainHostdevDefPtr hostdev; if (!(hostdev = qemuParseCommandLineUSB(val))) @@ -6399,18 +6393,16 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, def->nets[def->nnets++] = net; } } else if (STREQ(arg, "-drive")) { - virDomainDiskDefPtr disk; WANT_VALUE(); if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk))) goto error; - if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) { - virDomainDiskDefFree(disk); + if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) goto no_memory; - } - def->disks[def->ndisks++] = disk; - if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) nvirtiodisk++; + + def->disks[def->ndisks++] = disk; + disk = NULL; } else if (STREQ(arg, "-pcidevice")) { virDomainHostdevDefPtr hostdev; WANT_VALUE(); @@ -6516,8 +6508,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (VIR_ALLOC(chr) < 0) goto no_memory; - if (qemuParseCommandLineChr(chr, val) < 0) + if (qemuParseCommandLineChr(chr, val) < 0) { + virDomainChrSourceDefFree(chr); goto error; + } *monConfig = chr; } @@ -6545,10 +6539,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, char *hosts, *port, *saveptr = NULL, *token; virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0 ; i < def->ndisks ; i++) { - virDomainDiskDefPtr disk = def->disks[i]; - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && - disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { - first_rbd_disk = disk; + if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK && + def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { + first_rbd_disk = def->disks[i]; break; } } @@ -6676,6 +6669,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, no_memory: virReportOOMError(); error: + virDomainDiskDefFree(disk); VIR_FREE(cmd); virDomainDefFree(def); VIR_FREE(nics); -- 1.7.4.4
On 08/02/2011 06:34 PM, Eric Blake wrote:
Quite a few leaks detected by coverity. For chr, the leaks were close enough to the allocations to plug in place; for disk, the leaks were separated from the allocation by enough other lines with intermediate failure cases that I refactored the cleanup instead.
Okay, this time I think it's done. ACK.
Coverity gets confused by our logic. Add some hints to silence false positives. * src/qemu/qemu_driver.c (qemudDomainGetVcpuPinInfo): Add hint. (qemuDomainGetMemoryParameters): Likewise. --- src/qemu/qemu_driver.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2044e23..ce19be7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3432,6 +3432,9 @@ qemudDomainGetVcpuPinInfo(virDomainPtr dom, goto cleanup; } + /* Coverity didn't realize that targetDef must be set if we got here. */ + sa_assert(targetDef); + if (nodeGetInfo(dom->conn, &nodeinfo) < 0) goto cleanup; hostcpus = VIR_NODEINFO_MAXCPUS(nodeinfo); @@ -6139,6 +6142,9 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, param->value.ul = 0; param->type = VIR_TYPED_PARAM_ULLONG; + /* Coverity does not realize that if we get here, group is set. */ + sa_assert(group); + switch (i) { case 0: /* fill memory hard limit here */ rc = virCgroupGetMemoryHardLimit(group, &val); -- 1.7.4.4
On 08/02/2011 04:52 PM, Laine Stump wrote:
On 08/02/2011 06:47 PM, Eric Blake wrote:
Coverity gets confused by our logic. Add some hints to silence false positives.
ACK.
I've pushed through 17, and run out of time for any more today. Hopefully I picked out the juicy bugs, as evidenced by the fact that my later reports were merely silencing false positives. Any further analysis on my part will be post-0.9.4. Thanks for the speedy reviews today (it helps that the patches were mostly small). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Aug 02, 2011 at 05:00:19PM -0600, Eric Blake wrote:
On 08/02/2011 04:52 PM, Laine Stump wrote:
On 08/02/2011 06:47 PM, Eric Blake wrote:
Coverity gets confused by our logic. Add some hints to silence false positives.
ACK.
I've pushed through 17, and run out of time for any more today. Hopefully I picked out the juicy bugs, as evidenced by the fact that my later reports were merely silencing false positives. Any further analysis on my part will be post-0.9.4.
Thanks for the speedy reviews today (it helps that the patches were mostly small).
Thanks a lot for doing this Eric, this definitely squashed a couple of real but yet undetected bug waiting for 0.9.4 !!! 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 (4)
-
Daniel Veillard -
Eric Blake -
Jiri Denemark -
Laine Stump