2:42 p.m.
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
2:42 p.m.
New subject: [libvirt] [PATCH 01/12] build: detect Coverity 5.3.0
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
8:25 a.m.
New subject: [libvirt] [PATCH 01/12] build: detect Coverity 5.3.0
2011/6/6 Eric Blake <eblake(a)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
2:42 p.m.
New subject: [libvirt] [PATCH 02/12] storage: avoid mishandling backing store > 2GB
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
8:34 a.m.
New subject: [libvirt] [PATCH 02/12] storage: avoid mishandling backing store > 2GB
2011/6/6 Eric Blake <eblake(a)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
6:17 a.m.
New subject: [libvirt] [PATCH 02/12] storage: avoid mishandling backing store > 2GB
On 06/07/2011 07:34 AM, Matthias Bolte wrote:
2011/6/6 Eric Blake <eblake(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:05 a.m.
New subject: [libvirt] [PATCH 02/12] storage: avoid mishandling backing store > 2GB
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/
1 p.m.
New subject: [libvirt] [PATCH 02/12] storage: avoid mishandling backing store > 2GB
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:42 p.m.
New subject: [libvirt] [PATCH 03/12] build: silence coverity false positive
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
8:31 a.m.
New subject: [libvirt] [PATCH 03/12] build: silence coverity false positive
2011/6/6 Eric Blake <eblake(a)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
6:29 a.m.
New subject: [libvirt] [PATCH 03/12] build: silence coverity false positive
On 06/07/2011 07:31 AM, Matthias Bolte wrote:
2011/6/6 Eric Blake <eblake(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:42 p.m.
New subject: [libvirt] [PATCH 04/12] python: avoid unlikely sign extension bug
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
8:32 a.m.
New subject: [libvirt] [PATCH 04/12] python: avoid unlikely sign extension bug
2011/6/6 Eric Blake <eblake(a)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
2:42 p.m.
New subject: [libvirt] [PATCH 05/12] debug: avoid null dereference on uuid lookup api
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
8:37 a.m.
New subject: [libvirt] [PATCH 05/12] debug: avoid null dereference on uuid lookup api
2011/6/6 Eric Blake <eblake(a)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
2:42 p.m.
New subject: [libvirt] [PATCH 06/12] uuid: annotate non-null requirements
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
8:39 a.m.
New subject: [libvirt] [PATCH 06/12] uuid: annotate non-null requirements
2011/6/6 Eric Blake <eblake(a)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
2:42 p.m.
New subject: [libvirt] [PATCH 07/12] qemu: reorder checks for safety
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
8:38 a.m.
New subject: [libvirt] [PATCH 07/12] qemu: reorder checks for safety
2011/6/6 Eric Blake <eblake(a)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
2:43 p.m.
New subject: [libvirt] [PATCH 08/12] secret: drop dead code
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
8:41 a.m.
New subject: [libvirt] [PATCH 08/12] secret: drop dead code
2011/6/6 Eric Blake <eblake(a)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
2:43 p.m.
New subject: [libvirt] [PATCH 09/12] esx: avoid dead code
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(a)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
9:01 a.m.
New subject: [libvirt] [PATCH 09/12] esx: avoid dead code
2011/6/6 Eric Blake <eblake(a)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(a)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
6:43 a.m.
New subject: [libvirt] [PATCH 09/12] esx: avoid dead code
On 06/07/2011 08:01 AM, Matthias Bolte wrote:
2011/6/6 Eric Blake <eblake(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:43 p.m.
New subject: [libvirt] [PATCH 10/12] build: silence coverity false positives
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
8:46 a.m.
New subject: [libvirt] [PATCH 10/12] build: silence coverity false positives
2011/6/6 Eric Blake <eblake(a)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
2:48 p.m.
New subject: [libvirt] [PATCH 11/12] qemu: add missing break statement
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
8:48 a.m.
New subject: [libvirt] [PATCH 11/12] qemu: add missing break statement
2011/6/6 Eric Blake <eblake(a)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
2:48 p.m.
New subject: [libvirt] [PATCH 12/12] build: break some long lines
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
8:49 a.m.
New subject: [libvirt] [PATCH 12/12] build: break some long lines
2011/6/6 Eric Blake <eblake(a)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
4:06 p.m.
New subject: [libvirt] [PATCH 13/12] daemon: plug memory leak
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
8:50 a.m.
New subject: [libvirt] [PATCH 13/12] daemon: plug memory leak
2011/6/6 Eric Blake <eblake(a)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
4912
days inactive
4919
days old
31 comments
3 participants
participants (3)
-
Eric Blake -
Matthias Bolte -
Richard W.M. Jones