11:12 a.m.
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
11:12 a.m.
New subject: [libvirt] [PATCH 1/3] rpc: avoid double close on error
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
12:26 p.m.
New subject: [libvirt] [PATCH 1/3] rpc: avoid double close on error
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.
11:12 a.m.
New subject: [libvirt] [PATCH 2/3] rpc: avoid uninitialized memory use
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
12:27 p.m.
New subject: [libvirt] [PATCH 2/3] rpc: avoid uninitialized memory use
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.
11:12 a.m.
New subject: [libvirt] [PATCH 3/3] python: avoid unlikely sign extension bug
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
12:28 p.m.
New subject: [libvirt] [PATCH 3/3] python: avoid unlikely sign extension bug
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:01 p.m.
New subject: [libvirt] [PATCH 3/3] python: avoid unlikely sign extension bug
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:31 p.m.
New subject: [libvirt] [PATCH 4/3] build: allow caching the input to STATIC_ANALYSIS
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
2:26 p.m.
New subject: [libvirt] [PATCH 4/3] build: allow caching the input to STATIC_ANALYSIS
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.
2:06 p.m.
New subject: [libvirt] [PATCH 5/3] rpc: correctly process sasl whitelist globs
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
2:20 p.m.
New subject: [libvirt] [PATCH 5/3] rpc: correctly process sasl whitelist globs
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.
2:21 p.m.
New subject: [libvirt] [PATCH 5/3] rpc: correctly process sasl whitelist globs
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
2:21 p.m.
New subject: [libvirt] [PATCH 6/3] qemu: avoid null deref on block pull error
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
3:57 p.m.
New subject: [libvirt] [PATCH 6/3] qemu: avoid null deref on block pull error
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.
2:38 p.m.
New subject: [libvirt] [PATCH 7/3] rpc: avoid crash on error
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
3:55 p.m.
New subject: [libvirt] [PATCH 7/3] rpc: avoid crash on error
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;
2:50 p.m.
New subject: [libvirt] [PATCH 8/3] conf: avoid memory leak
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
3:53 p.m.
New subject: [libvirt] [PATCH 8/3] conf: avoid memory leak
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;
}
}
4:07 p.m.
New subject: [libvirt] [PATCH 8/3] conf: avoid memory leak
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:10 p.m.
New subject: [libvirt] [PATCH] qemu: avoid memory leaks
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
3:48 p.m.
New subject: [libvirt] [PATCH] qemu: avoid memory leaks
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);
4:46 p.m.
New subject: [libvirt] [PATCH] qemu: avoid memory leaks
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:56 p.m.
New subject: [libvirt] [PATCH] qemu: avoid memory leaks
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.
3:20 p.m.
New subject: [libvirt] [PATCH 10/3] qemu: remove dead code
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
3:40 p.m.
New subject: [libvirt] [PATCH 10/3] qemu: remove dead code
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.
3:38 p.m.
New subject: [libvirt] [PATCH 11/3] qemu: plug child process leak on domain core dump
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
3:59 p.m.
New subject: [libvirt] [PATCH 11/3] qemu: plug child process leak on domain core dump
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.
4:16 p.m.
New subject: [libvirt] [PATCH 11/3] qemu: plug child process leak on domain core dump
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:46 p.m.
New subject: [libvirt] [PATCHv2 9/3] qemu: avoid memory leaks
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
4:50 p.m.
New subject: [libvirt] [PATCHv2 9/3] qemu: avoid memory leaks
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:33 p.m.
New subject: [libvirt] [PATCHv2 9/3] qemu: avoid memory leaks
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:32 p.m.
New subject: [libvirt] [PATCHv2 9/3] qemu: avoid memory leaks
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);
4:53 p.m.
New subject: [libvirt] [PATCH 12/3] build: silence coverity false positives
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
5:33 p.m.
New subject: [libvirt] [PATCH 12/3] build: silence coverity false positives
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.
4:58 p.m.
New subject: [libvirt] [PATCH 13/3] rpc: avoid null deref
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
5:34 p.m.
New subject: [libvirt] [PATCH 13/3] rpc: avoid null deref
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.
5:04 p.m.
New subject: [libvirt] [PATCH 14/3] rpc: silence coverity false positives
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
5:37 p.m.
New subject: [libvirt] [PATCH 14/3] rpc: silence coverity false positives
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.
5:09 p.m.
New subject: [libvirt] [PATCH 15/3] interface: drop dead code
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
5:38 p.m.
New subject: [libvirt] [PATCH 15/3] interface: drop dead code
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.
5:27 p.m.
New subject: [libvirt] [PATCH 16/3] util: plug memory leak
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
5:47 p.m.
New subject: [libvirt] [PATCH 16/3] util: plug memory leak
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!
5:34 p.m.
New subject: [libvirt] [PATCHv3 9/3] qemu: avoid memory leaks
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
5:44 p.m.
New subject: [libvirt] [PATCHv3 9/3] qemu: avoid memory leaks
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.
5:47 p.m.
New subject: [libvirt] [PATCH 17/3] qemu: silence coverity false positives
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
6 p.m.
New subject: [libvirt] [PATCH 17/3] qemu: silence coverity false positives
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:39 p.m.
New subject: [libvirt] [PATCH 17/3] qemu: silence coverity false positives
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4860
days inactive
4861
days old
48 comments
4 participants
participants (4)
-
Daniel Veillard -
Eric Blake -
Jiri Denemark -
Laine Stump