[libvirt] [PATCH 00/12] Coverity cleanups, round 2
Well, I guess I didn't send these in time for 0.9.2. Again, some are bigger in impact than others. Eric Blake (12): build: detect Coverity 5.3.0 storage: avoid mishandling backing store > 2GB build: silence coverity false positive python: avoid unlikely sign extension bug debug: avoid null dereference on uuid lookup api uuid: annotate non-null requirements qemu: reorder checks for safety secret: drop dead code esx: avoid dead code build: silence coverity false positives qemu: add missing break statement build: break some long lines configure.ac | 4 ++- python/libvirt-override.c | 2 +- src/conf/nwfilter_conf.c | 4 +++ src/esx/esx_vi.c | 4 +- src/libvirt.c | 42 ++++++++++++++++++++++++--------------- src/qemu/qemu_cgroup.c | 4 +- src/qemu/qemu_hotplug.c | 47 +++++++++++++++++++++++++++++--------------- src/secret/secret_driver.c | 8 +------ src/util/storage_file.c | 3 +- src/util/util.c | 2 +- src/util/uuid.h | 8 ++++-- tools/virsh.c | 3 ++ 12 files changed, 81 insertions(+), 50 deletions(-) -- 1.7.4.4
Coverity 5.3.0 still outputs lots of COVERITY_* variables, but no longer modifies COVERITY_BUILD_COMMAND in the environment. Pick one that seems likely to stay around. * configure.ac (STATIC_ANALYSIS): Detect newer Coverity. --- configure.ac | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/configure.ac b/configure.ac index 5307a1d..c9f09b9 100644 --- a/configure.ac +++ b/configure.ac @@ -2389,7 +2389,9 @@ 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. -test -n "${CCC_ANALYZER_ANALYSIS+set}$COVERITY_BUILD_COMMAND" && t=1 || t=0 +t=0 +test -n "${CCC_ANALYZER_ANALYSIS+set}" && t=1 +test -n "$COVERITY_BUILD_COMMAND$COVERITY_LD_PRELOAD" && t=1 AC_DEFINE_UNQUOTED([STATIC_ANALYSIS], [$t], [Define to 1 when performing static analysis.]) -- 1.7.4.4
2011/6/6 Eric Blake <eblake@redhat.com>:
Coverity 5.3.0 still outputs lots of COVERITY_* variables, but no longer modifies COVERITY_BUILD_COMMAND in the environment. Pick one that seems likely to stay around.
* configure.ac (STATIC_ANALYSIS): Detect newer Coverity. --- configure.ac | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/configure.ac b/configure.ac index 5307a1d..c9f09b9 100644 --- a/configure.ac +++ b/configure.ac @@ -2389,7 +2389,9 @@ 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. -test -n "${CCC_ANALYZER_ANALYSIS+set}$COVERITY_BUILD_COMMAND" && t=1 || t=0 +t=0 +test -n "${CCC_ANALYZER_ANALYSIS+set}" && t=1 +test -n "$COVERITY_BUILD_COMMAND$COVERITY_LD_PRELOAD" && t=1 AC_DEFINE_UNQUOTED([STATIC_ANALYSIS], [$t], [Define to 1 when performing static analysis.])
ACK. Matthias
Detected by Coverity. The code was doing math on shifted unsigned char (which promotes to int), then promoting that to unsigned long during assignment to size. On 64-bit platforms, this risks sign extending values of size > 2GiB. Bug present since commit 489fd3 (v0.6.0). I'm not sure if a specially-crafted bogus qcow2 image could exploit this, although it's probably not possible, since we were already checking for the computed results being within range of our fixed-size buffer. * src/util/storage_file.c (qcowXGetBackingStore): Avoid sign extension. --- src/util/storage_file.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 6b3b756..6749599 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -27,6 +27,7 @@ #include <sys/stat.h> #include <unistd.h> #include <fcntl.h> +#include <stdint.h> #ifdef __linux__ # if HAVE_LINUX_MAGIC_H # include <linux/magic.h> @@ -274,7 +275,7 @@ qcowXGetBackingStore(char **res, bool isQCow2) { unsigned long long offset; - unsigned long size; + uint32_t size; *res = NULL; if (format) -- 1.7.4.4
2011/6/6 Eric Blake <eblake@redhat.com>:
Detected by Coverity. The code was doing math on shifted unsigned char (which promotes to int), then promoting that to unsigned long during assignment to size. On 64-bit platforms, this risks sign extending values of size > 2GiB. Bug present since commit 489fd3 (v0.6.0).
I'm not sure if a specially-crafted bogus qcow2 image could exploit this, although it's probably not possible, since we were already checking for the computed results being within range of our fixed-size buffer.
* src/util/storage_file.c (qcowXGetBackingStore): Avoid sign extension. --- src/util/storage_file.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 6b3b756..6749599 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -27,6 +27,7 @@ #include <sys/stat.h> #include <unistd.h> #include <fcntl.h> +#include <stdint.h> #ifdef __linux__ # if HAVE_LINUX_MAGIC_H # include <linux/magic.h> @@ -274,7 +275,7 @@ qcowXGetBackingStore(char **res, bool isQCow2) { unsigned long long offset; - unsigned long size; + uint32_t size;
*res = NULL; if (format)
Using unsigned int instead of uint32_t would also work and avoid stdint.h types that the libvirt codebase avoids. At any rate, ACK. Matthias
On 06/07/2011 07:34 AM, Matthias Bolte wrote:
2011/6/6 Eric Blake <eblake@redhat.com>:
Detected by Coverity. The code was doing math on shifted unsigned char (which promotes to int), then promoting that to unsigned long during assignment to size. On 64-bit platforms, this risks sign extending values of size > 2GiB. Bug present since commit 489fd3 (v0.6.0).
+#include <stdint.h> #ifdef __linux__ # if HAVE_LINUX_MAGIC_H # include <linux/magic.h> @@ -274,7 +275,7 @@ qcowXGetBackingStore(char **res, bool isQCow2) { unsigned long long offset; - unsigned long size; + uint32_t size;
*res = NULL; if (format)
Using unsigned int instead of uint32_t would also work and avoid stdint.h types that the libvirt codebase avoids.
Sure, I'll push with that tweak. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Jun 07, 2011 at 03:34:58PM +0200, Matthias Bolte wrote:
Using unsigned int instead of uint32_t would also work and avoid stdint.h types that the libvirt codebase avoids.
Out of interest, why is libvirt avoiding these int types? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/
On 06/08/2011 10:05 AM, Richard W.M. Jones wrote:
On Tue, Jun 07, 2011 at 03:34:58PM +0200, Matthias Bolte wrote:
Using unsigned int instead of uint32_t would also work and avoid stdint.h types that the libvirt codebase avoids.
Out of interest, why is libvirt avoiding these int types?
Actually, I don't see anything wrong with using these types internally (which this file was) where it makes sense; it just becomes a bit more of a maintenance burden to remember to use the correct PRI* macros rather than raw %d and friends when printing such values. But for the public APIs, we've chosen to avoid those types and to instead stick to native int types, so that our public headers do not break in the face of broken <stdint.h> files (it's sad how many platforms where gnulib ends up replacing <stdint.h> because of bugs). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Similar in nature to commit fd21ecfd, which shut up valgrind. sigaction is apparently a nasty interface for code analyzers. * src/util/util.c (virExecWithHook): Initialize entire var, since coverity gripes about the (unused and non-standard) sa_restorer. --- src/util/util.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index e221abe..5672193 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -690,8 +690,8 @@ virExecWithHook(const char *const*argv, * so we need to temporarily block that again */ struct sigaction waxon, waxoff; + memset(&waxoff, 0, sizeof(waxoff)); waxoff.sa_handler = SIG_IGN; - waxoff.sa_flags = 0; sigemptyset(&waxoff.sa_mask); memset(&waxon, 0, sizeof(waxon)); if (sigaction(SIGPIPE, &waxoff, &waxon) < 0) { -- 1.7.4.4
2011/6/6 Eric Blake <eblake@redhat.com>:
Similar in nature to commit fd21ecfd, which shut up valgrind.
sigaction is apparently a nasty interface for code analyzers.
* src/util/util.c (virExecWithHook): Initialize entire var, since coverity gripes about the (unused and non-standard) sa_restorer. --- src/util/util.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index e221abe..5672193 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -690,8 +690,8 @@ virExecWithHook(const char *const*argv, * so we need to temporarily block that again */ struct sigaction waxon, waxoff; + memset(&waxoff, 0, sizeof(waxoff)); waxoff.sa_handler = SIG_IGN; - waxoff.sa_flags = 0; sigemptyset(&waxoff.sa_mask); memset(&waxon, 0, sizeof(waxon)); if (sigaction(SIGPIPE, &waxoff, &waxon) < 0) { -- 1.7.4.4
ACK. Matthias
On 06/07/2011 07:31 AM, Matthias Bolte wrote:
2011/6/6 Eric Blake <eblake@redhat.com>:
Similar in nature to commit fd21ecfd, which shut up valgrind.
sigaction is apparently a nasty interface for code analyzers.
* src/util/util.c (virExecWithHook): Initialize entire var, since coverity gripes about the (unused and non-standard) sa_restorer. --- src/util/util.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
Of course, this code moved to src/util/command.c in the meantime. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Detected by Coverity. cpumap was allocated with a value of (unsigned short)*(int), which is an int computation, and then promotes to size_t. On a 64-bit platform, this fails if bit 32 of the product is set (because of sign extension giving a HUGE value to malloc), even though a naive programmer would assume that since the first value is unsigned, the product is also unsigned and at most 4GB would be allocated. Won't bite in practice (the product should never be that large), but worth using the right types to begin with, so that we are now computing (unsigned short)*(size_t). * python/libvirt-override.c (libvirt_virDomainGetVcpus): 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 763df00..974decb 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -414,7 +414,7 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED, virDomainInfo dominfo; virVcpuInfoPtr cpuinfo = NULL; unsigned char *cpumap = NULL; - int cpumaplen, i; + size_t cpumaplen, i; int i_retval; if (!PyArg_ParseTuple(args, (char *)"O:virDomainGetVcpus", -- 1.7.4.4
2011/6/6 Eric Blake <eblake@redhat.com>:
Detected by Coverity. cpumap was allocated with a value of (unsigned short)*(int), which is an int computation, and then promotes to size_t. On a 64-bit platform, this fails if bit 32 of the product is set (because of sign extension giving a HUGE value to malloc), even though a naive programmer would assume that since the first value is unsigned, the product is also unsigned and at most 4GB would be allocated.
Won't bite in practice (the product should never be that large), but worth using the right types to begin with, so that we are now computing (unsigned short)*(size_t).
* python/libvirt-override.c (libvirt_virDomainGetVcpus): 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 763df00..974decb 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -414,7 +414,7 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED, virDomainInfo dominfo; virVcpuInfoPtr cpuinfo = NULL; unsigned char *cpumap = NULL; - int cpumaplen, i; + size_t cpumaplen, i; int i_retval;
if (!PyArg_ParseTuple(args, (char *)"O:virDomainGetVcpus",
ACK. Matthias
Detected by Coverity. Commit a98d8f0d tried to make uuid debugging more robust, but missed some APIs. And on the APIs that it visited, the mere act of preparing the debug message ends up dereferencing uuid prior to the null check. Which means the APIs which are supposed to gracefully reject NULL arguments now end up with SIGSEGV. * src/libvirt.c (VIR_UUID_DEBUG): New macro. (virDomainLookupByUUID, virDomainLookupByUUIDString) (virNetworkLookupByUUID, virNetworkLookupByUUIDString) (virStoragePoolLookupByUUID, virStoragePoolLookupByUUIDString) (virSecretLookupByUUID, virSecretLookupByUUIDString) (virNWFilterLookupByUUID, virNWFilterLookupByUUIDString): Avoid null dereference. --- src/libvirt.c | 42 ++++++++++++++++++++++++++---------------- 1 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index cbe1926..bb80f3f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -363,6 +363,22 @@ static struct gcry_thread_cbs virTLSThreadImpl = { __VA_ARGS__) /** + * VIR_UUID_DEBUG: + * @conn: connection + * @uuid: possibly null UUID array + */ +#define VIR_UUID_DEBUG(conn, uuid) \ + do { \ + if (uuid) { \ + char _uuidstr[VIR_UUID_STRING_BUFLEN]; \ + virUUIDFormat(uuid, _uuidstr); \ + VIR_DEBUG("conn=%p, uuid=%s", conn, _uuidstr); \ + } else { \ + VIR_DEBUG("conn=%p, uuid=(null)", conn); \ + } \ + } while (0) + +/** * virInitialize: * * Initialize the library. It's better to call this routine at startup @@ -1941,10 +1957,7 @@ error: virDomainPtr virDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(uuid, uuidstr); - - VIR_DEBUG("conn=%p, uuid=%s", conn, uuidstr); + VIR_UUID_DEBUG(conn, uuid); virResetLastError(); @@ -1987,7 +2000,7 @@ virDomainPtr virDomainLookupByUUIDString(virConnectPtr conn, const char *uuidstr) { unsigned char uuid[VIR_UUID_BUFLEN]; - VIR_DEBUG("conn=%p, uuidstr=%s", conn, uuidstr); + VIR_DEBUG("conn=%p, uuidstr=%s", conn, NULLSTR(uuidstr)); virResetLastError(); @@ -7558,10 +7571,7 @@ error: virNetworkPtr virNetworkLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(uuid, uuidstr); - - VIR_DEBUG("conn=%p, uuid=%s", conn, uuidstr); + VIR_UUID_DEBUG(conn, uuid); virResetLastError(); @@ -7604,7 +7614,7 @@ virNetworkPtr virNetworkLookupByUUIDString(virConnectPtr conn, const char *uuidstr) { unsigned char uuid[VIR_UUID_BUFLEN]; - VIR_DEBUG("conn=%p, uuidstr=%s", conn, uuidstr); + VIR_DEBUG("conn=%p, uuidstr=%s", conn, NULLSTR(uuidstr)); virResetLastError(); @@ -9300,7 +9310,7 @@ virStoragePoolPtr virStoragePoolLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { - VIR_DEBUG("conn=%p, uuid=%s", conn, uuid); + VIR_UUID_DEBUG(conn, uuid); virResetLastError(); @@ -9344,7 +9354,7 @@ virStoragePoolLookupByUUIDString(virConnectPtr conn, const char *uuidstr) { unsigned char uuid[VIR_UUID_BUFLEN]; - VIR_DEBUG("conn=%p, uuidstr=%s", conn, uuidstr); + VIR_DEBUG("conn=%p, uuidstr=%s", conn, NULLSTR(uuidstr)); virResetLastError(); @@ -11831,7 +11841,7 @@ error: virSecretPtr virSecretLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { - VIR_DEBUG("conn=%p, uuid=%s", conn, uuid); + VIR_UUID_DEBUG(conn, uuid); virResetLastError(); @@ -11876,7 +11886,7 @@ virSecretPtr virSecretLookupByUUIDString(virConnectPtr conn, const char *uuidstr) { unsigned char uuid[VIR_UUID_BUFLEN]; - VIR_DEBUG("conn=%p, uuidstr=%s", conn, uuidstr); + VIR_DEBUG("conn=%p, uuidstr=%s", conn, NULLSTR(uuidstr)); virResetLastError(); @@ -13487,7 +13497,7 @@ error: virNWFilterPtr virNWFilterLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { - VIR_DEBUG("conn=%p, uuid=%s", conn, uuid); + VIR_UUID_DEBUG(conn, uuid); virResetLastError(); @@ -13530,7 +13540,7 @@ virNWFilterPtr virNWFilterLookupByUUIDString(virConnectPtr conn, const char *uuidstr) { unsigned char uuid[VIR_UUID_BUFLEN]; - VIR_DEBUG("conn=%p, uuidstr=%s", conn, uuidstr); + VIR_DEBUG("conn=%p, uuidstr=%s", conn, NULLSTR(uuidstr)); virResetLastError(); -- 1.7.4.4
2011/6/6 Eric Blake <eblake@redhat.com>:
Detected by Coverity. Commit a98d8f0d tried to make uuid debugging more robust, but missed some APIs. And on the APIs that it visited, the mere act of preparing the debug message ends up dereferencing uuid prior to the null check. Which means the APIs which are supposed to gracefully reject NULL arguments now end up with SIGSEGV.
* src/libvirt.c (VIR_UUID_DEBUG): New macro. (virDomainLookupByUUID, virDomainLookupByUUIDString) (virNetworkLookupByUUID, virNetworkLookupByUUIDString) (virStoragePoolLookupByUUID, virStoragePoolLookupByUUIDString) (virSecretLookupByUUID, virSecretLookupByUUIDString) (virNWFilterLookupByUUID, virNWFilterLookupByUUIDString): Avoid null dereference. --- src/libvirt.c | 42 ++++++++++++++++++++++++++---------------- 1 files changed, 26 insertions(+), 16 deletions(-)
ACK. Matthias
Coverity already saw through a NULL dereference without these annotations, and gcc is still too puny to do good NULL analysis. But clang still benefits (and is easier to run than coverity), not to mention that adding this bit of documentation to the code may help future developers remember the constraints. * src/util/uuid.h (virGetHostUUID, virUUIDFormat): Document restrictions, for improved static analysis. --- src/util/uuid.h | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/util/uuid.h b/src/util/uuid.h index 36abcfc..b5d7878 100644 --- a/src/util/uuid.h +++ b/src/util/uuid.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007 Red Hat, Inc. + * Copyright (C) 2007, 2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -22,8 +22,10 @@ #ifndef __VIR_UUID_H__ # define __VIR_UUID_H__ +# include "internal.h" + int virSetHostUUIDStr(const char *host_uuid); -int virGetHostUUID(unsigned char *host_uuid); +int virGetHostUUID(unsigned char *host_uuid) ATTRIBUTE_NONNULL(1); int virUUIDIsValid(unsigned char *uuid); @@ -33,6 +35,6 @@ int virUUIDParse(const char *uuidstr, unsigned char *uuid); void virUUIDFormat(const unsigned char *uuid, - char *uuidstr); + char *uuidstr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); #endif /* __VIR_UUID_H__ */ -- 1.7.4.4
2011/6/6 Eric Blake <eblake@redhat.com>:
Coverity already saw through a NULL dereference without these annotations, and gcc is still too puny to do good NULL analysis. But clang still benefits (and is easier to run than coverity), not to mention that adding this bit of documentation to the code may help future developers remember the constraints.
* src/util/uuid.h (virGetHostUUID, virUUIDFormat): Document restrictions, for improved static analysis. --- src/util/uuid.h | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
ACK. Matthias
Detected by Coverity. All existing callers happen to be in range, so this isn't too serious. * src/qemu/qemu_cgroup.c (qemuCgroupControllerActive): Check bounds before dereference. --- src/qemu/qemu_cgroup.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index eba1e73..1298924 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -48,10 +48,10 @@ bool qemuCgroupControllerActive(struct qemud_driver *driver, { if (driver->cgroup == NULL) return false; - if (!virCgroupMounted(driver->cgroup, controller)) - return false; if (controller < 0 || controller >= VIR_CGROUP_CONTROLLER_LAST) return false; + if (!virCgroupMounted(driver->cgroup, controller)) + return false; if (driver->cgroupControllers & (1 << controller)) return true; return false; -- 1.7.4.4
2011/6/6 Eric Blake <eblake@redhat.com>:
Detected by Coverity. All existing callers happen to be in range, so this isn't too serious.
* src/qemu/qemu_cgroup.c (qemuCgroupControllerActive): Check bounds before dereference. --- src/qemu/qemu_cgroup.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index eba1e73..1298924 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -48,10 +48,10 @@ bool qemuCgroupControllerActive(struct qemud_driver *driver, { if (driver->cgroup == NULL) return false; - if (!virCgroupMounted(driver->cgroup, controller)) - return false; if (controller < 0 || controller >= VIR_CGROUP_CONTROLLER_LAST) return false; + if (!virCgroupMounted(driver->cgroup, controller)) + return false; if (driver->cgroupControllers & (1 << controller)) return true; return false; -- 1.7.4.4
ACK. Matthias
Detected by Coverity. The only ways to get to the cleanup label were by an early abort (list still NULL) or after successfully transferring list to dest, so there is no list to clean up. * src/secret/secret_driver.c (loadSecrets): Kill dead code. --- src/secret/secret_driver.c | 8 +------- 1 files changed, 1 insertions(+), 7 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 8f5e735..ee3c165 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -1,7 +1,7 @@ /* * secret_driver.c: local driver for secret manipulation API * - * Copyright (C) 2009-2010 Red Hat, Inc. + * Copyright (C) 2009-2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -516,12 +516,6 @@ loadSecrets(virSecretDriverStatePtr driver, ret = 0; cleanup: - while (list != NULL) { - virSecretEntryPtr s; - - s = listUnlink(&list); - secretFree(s); - } if (dir != NULL) closedir(dir); return ret; -- 1.7.4.4
2011/6/6 Eric Blake <eblake@redhat.com>:
Detected by Coverity. The only ways to get to the cleanup label were by an early abort (list still NULL) or after successfully transferring list to dest, so there is no list to clean up.
* src/secret/secret_driver.c (loadSecrets): Kill dead code. --- src/secret/secret_driver.c | 8 +------- 1 files changed, 1 insertions(+), 7 deletions(-)
ACK. Matthias
Detected by Coverity. The beginning of the function already filtered out NULL objectContentList as invalid; more likely, the code was trying to see if esxVI_RetrieveProperties modified *objectContentList. * src/esx/esx_vi.c (esxVI_LookupObjectContentByType): Check correct pointer. --- Matthias, I'm not sure if this is the correct fix, because I don't know how escVI_RetrieveProperties is supposed to work. src/esx/esx_vi.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index c5c38ca..64e5b73 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -2,7 +2,7 @@ /* * esx_vi.c: client for the VMware VI API 2.5 to manage ESX hosts * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2009-2011 Matthias Bolte <matthias.bolte@googlemail.com> * * This library is free software; you can redistribute it and/or @@ -1737,7 +1737,7 @@ esxVI_LookupObjectContentByType(esxVI_Context *ctx, goto cleanup; } - if (objectContentList == NULL) { + if (*objectContentList == NULL) { switch (occurrence) { case esxVI_Occurrence_OptionalItem: case esxVI_Occurrence_OptionalList: -- 1.7.4.4
2011/6/6 Eric Blake <eblake@redhat.com>:
Detected by Coverity. The beginning of the function already filtered out NULL objectContentList as invalid; more likely, the code was trying to see if esxVI_RetrieveProperties modified *objectContentList.
* src/esx/esx_vi.c (esxVI_LookupObjectContentByType): Check correct pointer. ---
Matthias, I'm not sure if this is the correct fix, because I don't know how escVI_RetrieveProperties is supposed to work.
Sometime one got to love static code analysis tools. Yes, your patch is correct and necessary :) esxVI_RetrieveProperties is generated and returns a list of objects that match the given propertyFilterSpec. esxVI_LookupObjectContentByType then tests whether the result corresponds to the expected occurrence and reports an error otherwise. This simplifies the callers of esxVI_LookupObjectContentByType, but due to the missing dereference the check was never performed because the code thought that at least one item was obtained. NULL represents an empty list. Your patch now fixes this and is also a potential segfault fix because callers of esxVI_LookupObjectContentByType that specified "required" occurrence assume *objectContentList to be non-NULL when esxVI_LookupObjectContentByType succeeds.
src/esx/esx_vi.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index c5c38ca..64e5b73 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -2,7 +2,7 @@ /* * esx_vi.c: client for the VMware VI API 2.5 to manage ESX hosts * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2009-2011 Matthias Bolte <matthias.bolte@googlemail.com> * * This library is free software; you can redistribute it and/or @@ -1737,7 +1737,7 @@ esxVI_LookupObjectContentByType(esxVI_Context *ctx, goto cleanup; }
- if (objectContentList == NULL) { + if (*objectContentList == NULL) { switch (occurrence) { case esxVI_Occurrence_OptionalItem: case esxVI_Occurrence_OptionalList:
ACK. Matthias
On 06/07/2011 08:01 AM, Matthias Bolte wrote:
2011/6/6 Eric Blake <eblake@redhat.com>:
Detected by Coverity. The beginning of the function already filtered out NULL objectContentList as invalid; more likely, the code was trying to see if esxVI_RetrieveProperties modified *objectContentList.
* src/esx/esx_vi.c (esxVI_LookupObjectContentByType): Check correct pointer.
Your patch now fixes this and is also a potential segfault fix because callers of esxVI_LookupObjectContentByType that specified "required" occurrence assume *objectContentList to be non-NULL when esxVI_LookupObjectContentByType succeeds.
Thanks for the additional details; I summarized those into the commit message.
ACK.
I've now pushed the entire series. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Coverity complained about these intentional fallthrough cases, but not about other cases that were explicitly marked with nice comments. For some reason, Coverity doesn't seem smart enough to parse the up-front English comment in virsh about intentional fallthrough :) * tools/virsh.c (cmdVolSize): Mark fallthrough in a more typical fashion. * src/conf/nwfilter_conf.c (virNWFilterRuleDefDetailsFormat) (virNWFilterRuleDetailsParse): Mark explicit fallthrough. --- src/conf/nwfilter_conf.c | 4 ++++ tools/virsh.c | 3 +++ 2 files changed, 7 insertions(+), 0 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 40516c7..127d4be 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -1490,6 +1490,7 @@ virNWFilterRuleDetailsParse(xmlNodePtr node, switch (datatype) { case DATATYPE_UINT8_HEX: base = 16; + /* fallthrough */ case DATATYPE_UINT8: if (virStrToLong_ui(prop, NULL, base, &uint_val) >= 0) { if (uint_val <= 0xff) { @@ -1504,6 +1505,7 @@ virNWFilterRuleDetailsParse(xmlNodePtr node, case DATATYPE_UINT16_HEX: base = 16; + /* fallthrough */ case DATATYPE_UINT16: if (virStrToLong_ui(prop, NULL, base, &uint_val) >= 0) { if (uint_val <= 0xffff) { @@ -2728,6 +2730,7 @@ virNWFilterRuleDefDetailsFormat(virBufferPtr buf, case DATATYPE_UINT8_HEX: asHex = true; + /* fallthrough */ case DATATYPE_IPMASK: case DATATYPE_IPV6MASK: /* display all masks in CIDR format */ @@ -2738,6 +2741,7 @@ virNWFilterRuleDefDetailsFormat(virBufferPtr buf, case DATATYPE_UINT16_HEX: asHex = true; + /* fallthrough */ case DATATYPE_UINT16: virBufferAsprintf(buf, asHex ? "0x%x" : "%d", item->u.u16); diff --git a/tools/virsh.c b/tools/virsh.c index d98be1c..a720185 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7027,10 +7027,13 @@ static int cmdVolSize(const char *data, unsigned long long *val) switch (*end) { case 'T': *val *= 1024; + /* fallthrough */ case 'G': *val *= 1024; + /* fallthrough */ case 'M': *val *= 1024; + /* fallthrough */ case 'k': *val *= 1024; break; -- 1.7.4.4
2011/6/6 Eric Blake <eblake@redhat.com>:
Coverity complained about these intentional fallthrough cases, but not about other cases that were explicitly marked with nice comments.
For some reason, Coverity doesn't seem smart enough to parse the up-front English comment in virsh about intentional fallthrough :)
* tools/virsh.c (cmdVolSize): Mark fallthrough in a more typical fashion. * src/conf/nwfilter_conf.c (virNWFilterRuleDefDetailsFormat) (virNWFilterRuleDetailsParse): Mark explicit fallthrough. --- src/conf/nwfilter_conf.c | 4 ++++ tools/virsh.c | 3 +++ 2 files changed, 7 insertions(+), 0 deletions(-)
ACK. Matthias
Detected by Coverity. Bug introduced in commit 9d73efd (v0.8.8). * src/qemu/qemu_hotplug.c (qemuDomainChangeGraphics): Don't report error on success. --- I hate it when my ISP chokes on large patch series. Hopefully I'm threading this resend correctly. src/qemu/qemu_hotplug.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index fe47896..2154c36 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1117,6 +1117,7 @@ qemuDomainChangeGraphics(struct qemud_driver *driver, VIR_DEBUG("Not updating since password didn't change"); ret = 0; } + break; default: qemuReportError(VIR_ERR_INTERNAL_ERROR, -- 1.7.4.4
2011/6/6 Eric Blake <eblake@redhat.com>:
Detected by Coverity. Bug introduced in commit 9d73efd (v0.8.8).
* src/qemu/qemu_hotplug.c (qemuDomainChangeGraphics): Don't report error on success.
ACK. Matthias
As long as I was already touching the function... * src/qemu/qemu_hotplug.c (qemuDomainChangeGraphics): Line wrap. --- src/qemu/qemu_hotplug.c | 46 ++++++++++++++++++++++++++++++---------------- 1 files changed, 30 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2154c36..d33fc87 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1043,12 +1043,14 @@ qemuDomainChangeGraphics(struct qemud_driver *driver, switch (dev->type) { case VIR_DOMAIN_GRAPHICS_TYPE_VNC: if ((olddev->data.vnc.autoport != dev->data.vnc.autoport) || - (!dev->data.vnc.autoport && (olddev->data.vnc.port != dev->data.vnc.port))) { + (!dev->data.vnc.autoport && + (olddev->data.vnc.port != dev->data.vnc.port))) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change port settings on vnc graphics")); return -1; } - if (STRNEQ_NULLABLE(olddev->data.vnc.listenAddr, dev->data.vnc.listenAddr)) { + if (STRNEQ_NULLABLE(olddev->data.vnc.listenAddr, + dev->data.vnc.listenAddr)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change listen address setting on vnc graphics")); return -1; @@ -1063,10 +1065,14 @@ qemuDomainChangeGraphics(struct qemud_driver *driver, * even if new password matches old password */ if (olddev->data.vnc.auth.expires || dev->data.vnc.auth.expires || - STRNEQ_NULLABLE(olddev->data.vnc.auth.passwd, dev->data.vnc.auth.passwd)) { - VIR_DEBUG("Updating password on VNC server %p %p", dev->data.vnc.auth.passwd, driver->vncPassword); - ret = qemuDomainChangeGraphicsPasswords(driver, vm, VIR_DOMAIN_GRAPHICS_TYPE_VNC, - &dev->data.vnc.auth, driver->vncPassword); + STRNEQ_NULLABLE(olddev->data.vnc.auth.passwd, + dev->data.vnc.auth.passwd)) { + VIR_DEBUG("Updating password on VNC server %p %p", + dev->data.vnc.auth.passwd, driver->vncPassword); + ret = qemuDomainChangeGraphicsPasswords(driver, vm, + VIR_DOMAIN_GRAPHICS_TYPE_VNC, + &dev->data.vnc.auth, + driver->vncPassword); /* Steal the new dev's char * reference */ VIR_FREE(olddev->data.vnc.auth.passwd); @@ -1081,18 +1087,22 @@ qemuDomainChangeGraphics(struct qemud_driver *driver, case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: if ((olddev->data.spice.autoport != dev->data.spice.autoport) || - (!dev->data.spice.autoport && (olddev->data.spice.port != dev->data.spice.port)) || - (!dev->data.spice.autoport && (olddev->data.spice.tlsPort != dev->data.spice.tlsPort))) { + (!dev->data.spice.autoport && + (olddev->data.spice.port != dev->data.spice.port)) || + (!dev->data.spice.autoport && + (olddev->data.spice.tlsPort != dev->data.spice.tlsPort))) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change port settings on spice graphics")); return -1; } - if (STRNEQ_NULLABLE(olddev->data.spice.listenAddr, dev->data.spice.listenAddr)) { + if (STRNEQ_NULLABLE(olddev->data.spice.listenAddr, + dev->data.spice.listenAddr)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change listen address setting on spice graphics")); return -1; } - if (STRNEQ_NULLABLE(olddev->data.spice.keymap, dev->data.spice.keymap)) { + if (STRNEQ_NULLABLE(olddev->data.spice.keymap, + dev->data.spice.keymap)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change keymap setting on spice graphics")); return -1; @@ -1102,12 +1112,16 @@ qemuDomainChangeGraphics(struct qemud_driver *driver, * even if new password matches old password */ if (olddev->data.spice.auth.expires || dev->data.spice.auth.expires || - STRNEQ_NULLABLE(olddev->data.spice.auth.passwd, dev->data.spice.auth.passwd)) { - VIR_DEBUG("Updating password on SPICE server %p %p", dev->data.spice.auth.passwd, driver->spicePassword); - ret = qemuDomainChangeGraphicsPasswords(driver, vm, VIR_DOMAIN_GRAPHICS_TYPE_SPICE, - &dev->data.spice.auth, driver->spicePassword); - - /* Steal the new dev's char * reference */ + STRNEQ_NULLABLE(olddev->data.spice.auth.passwd, + dev->data.spice.auth.passwd)) { + VIR_DEBUG("Updating password on SPICE server %p %p", + dev->data.spice.auth.passwd, driver->spicePassword); + ret = qemuDomainChangeGraphicsPasswords(driver, vm, + VIR_DOMAIN_GRAPHICS_TYPE_SPICE, + &dev->data.spice.auth, + driver->spicePassword); + + /* Steal the new dev's char * reference */ VIR_FREE(olddev->data.spice.auth.passwd); olddev->data.spice.auth.passwd = dev->data.spice.auth.passwd; dev->data.spice.auth.passwd = NULL; -- 1.7.4.4
2011/6/6 Eric Blake <eblake@redhat.com>:
As long as I was already touching the function...
* src/qemu/qemu_hotplug.c (qemuDomainChangeGraphics): Line wrap. --- src/qemu/qemu_hotplug.c | 46 ++++++++++++++++++++++++++++++---------------- 1 files changed, 30 insertions(+), 16 deletions(-)
ACK. Matthias
Detected by Coverity. Commit ef21beda was incomplete; it solved a leak one one path, but not on the other. * daemon/libvirtd.c (qemudSetLogging): Avoid leak on success. --- How embarrassing that I was just barely fixing a leak of the same variable in this function, but only got half the job done. daemon/libvirtd.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 728031f..bcaa37b 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -2783,6 +2783,7 @@ qemudSetLogging(struct qemud_server *server, virConfPtr conf, VIR_FREE(userdir); goto out_of_memory; } + VIR_FREE(userdir); } } else { if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0) -- 1.7.4.4
2011/6/6 Eric Blake <eblake@redhat.com>:
Detected by Coverity. Commit ef21beda was incomplete; it solved a leak one one path, but not on the other.
* daemon/libvirtd.c (qemudSetLogging): Avoid leak on success. ---
How embarrassing that I was just barely fixing a leak of the same variable in this function, but only got half the job done.
daemon/libvirtd.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
ACK. Matthias
participants (3)
-
Eric Blake -
Matthias Bolte -
Richard W.M. Jones