9:30 a.m.
Patches 1, 3, & 4 show up periodically in my Coverity runs, but not
always. Usually only when some sort of self inflicted build error only
gets a partial build followed by a complete analysis phase.
Patch 2 is related to a Coverity error seen in private integration
testing as well. Although it could be deemed Coverity noise - it's
just easier to adjust our code to use the right type than try to
generate a reproducer or figure out exactly what the error is.
Patch 5 is based on some investigation I started in July to remove
some sa_asserts. I could split into two patches if really desired,
but it was just easier to keep as one.
John Ferlan (5):
qemu: Check virGetLastError return value for migration finish failure
lxc: Avoid Coverity SIZEOF_MISMATCH
virfile: Avoid Coverity IDENTICAL_BRANCHES error
util: Avoid Coverity FORWARD_NULL
conf: Remove need for a couple of sa_asserts
src/conf/domain_conf.c | 9 +++++----
src/libvirt-domain.c | 3 ++-
src/lxc/lxc_container.c | 4 ++--
src/qemu/qemu_migration.c | 3 ++-
src/util/virdbus.c | 4 ++++
src/util/virfile.c | 3 +--
6 files changed, 16 insertions(+), 10 deletions(-)
--
2.1.0
9:30 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: Check virGetLastError return value for migration finish failure
Commit id '2e7cea243' added a check for an error from Finish instead
of 'unexpected error'; however, if for some reason there wasn't an
error, then virGetLastError could return NULL resulting in the
NULL pointer deref to err->domain.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libvirt-domain.c | 3 ++-
src/qemu/qemu_migration.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index cbf08fc..964a4d7 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3195,7 +3195,8 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
orig_err->domain == VIR_FROM_QEMU &&
orig_err->code == VIR_ERR_OPERATION_FAILED) {
virErrorPtr err = virGetLastError();
- if (err->domain == VIR_FROM_QEMU &&
+ if (err &&
+ err->domain == VIR_FROM_QEMU &&
err->code != VIR_ERR_MIGRATE_FINISH_OK) {
virFreeError(orig_err);
orig_err = NULL;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index ff89ab5..d50d367 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -5023,7 +5023,8 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver,
orig_err->domain == VIR_FROM_QEMU &&
orig_err->code == VIR_ERR_OPERATION_FAILED) {
virErrorPtr err = virGetLastError();
- if (err->domain == VIR_FROM_QEMU &&
+ if (err &&
+ err->domain == VIR_FROM_QEMU &&
err->code != VIR_ERR_MIGRATE_FINISH_OK) {
virFreeError(orig_err);
orig_err = NULL;
--
2.1.0
12:36 p.m.
New subject: [libvirt] [PATCH 1/5] qemu: Check virGetLastError return value for migration finish failure
On 09/04/2015 10:30 AM, John Ferlan wrote:
Commit id '2e7cea243' added a check for an error from Finish
instead
of 'unexpected error'; however, if for some reason there wasn't an
error, then virGetLastError could return NULL resulting in the
NULL pointer deref to err->domain.
ACK.
9:30 a.m.
New subject: [libvirt] [PATCH 2/5] lxc: Avoid Coverity SIZEOF_MISMATCH
Commit id '692e9fac7' used virProcessSetNamespaces instead of inlining
the similar functionality; however, Coverity notes that the function
prototype expects a size_t value and not an enum and complains. So,
just pass as a size_t to avoid the noise.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/lxc/lxc_container.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index a433552..062da68 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -2150,8 +2150,8 @@ static int lxcContainerDropCapabilities(virDomainDefPtr def
ATTRIBUTE_UNUSED,
*/
static int lxcAttachNS(int *ns_fd)
{
- if (ns_fd &&
- virProcessSetNamespaces(VIR_LXC_DOMAIN_NAMESPACE_LAST, ns_fd) < 0)
+ size_t nfdlist = VIR_LXC_DOMAIN_NAMESPACE_LAST;
+ if (ns_fd && virProcessSetNamespaces(nfdlist, ns_fd) < 0)
return -1;
return 0;
}
--
2.1.0
12:41 p.m.
New subject: [libvirt] [PATCH 2/5] lxc: Avoid Coverity SIZEOF_MISMATCH
On 09/04/2015 10:30 AM, John Ferlan wrote:
Commit id '692e9fac7' used virProcessSetNamespaces instead of
inlining
the similar functionality; however, Coverity notes that the function
prototype expects a size_t value and not an enum and complains. So,
just pass as a size_t to avoid the noise.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/lxc/lxc_container.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
This file needs the copyright date updated to include 2015 (no I don't
go around checking for that; I have an emacs hook that complains about
it any time I save a file.)
index a433552..062da68 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -2150,8 +2150,8 @@ static int lxcContainerDropCapabilities(virDomainDefPtr def
ATTRIBUTE_UNUSED,
*/
static int lxcAttachNS(int *ns_fd)
{
- if (ns_fd &&
- virProcessSetNamespaces(VIR_LXC_DOMAIN_NAMESPACE_LAST, ns_fd) < 0)
+ size_t nfdlist = VIR_LXC_DOMAIN_NAMESPACE_LAST;
+ if (ns_fd && virProcessSetNamespaces(nfdlist, ns_fd) < 0)
Doesn't it work if you just add a typecast in the call:
virProcessSetNamespaces((size_t)VIR_LXC_DOMAIN_NAMESPACE_LAST, ns_fd)
?
Assuming there's a reason for the way you did it vs. this, ACK with the
copyright updated.
1:43 p.m.
New subject: [libvirt] [PATCH 2/5] lxc: Avoid Coverity SIZEOF_MISMATCH
On 09/04/2015 01:41 PM, Laine Stump wrote:
On 09/04/2015 10:30 AM, John Ferlan wrote:
> Commit id '692e9fac7' used virProcessSetNamespaces instead of inlining
> the similar functionality; however, Coverity notes that the function
> prototype expects a size_t value and not an enum and complains. So,
> just pass as a size_t to avoid the noise.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/lxc/lxc_container.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
This file needs the copyright date updated to include 2015 (no I don't
go around checking for that; I have an emacs hook that complains about
it any time I save a file.)
> index a433552..062da68 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -2150,8 +2150,8 @@ static int
> lxcContainerDropCapabilities(virDomainDefPtr def ATTRIBUTE_UNUSED,
> */
> static int lxcAttachNS(int *ns_fd)
> {
> - if (ns_fd &&
> - virProcessSetNamespaces(VIR_LXC_DOMAIN_NAMESPACE_LAST, ns_fd)
> < 0)
> + size_t nfdlist = VIR_LXC_DOMAIN_NAMESPACE_LAST;
> + if (ns_fd && virProcessSetNamespaces(nfdlist, ns_fd) < 0)
Doesn't it work if you just add a typecast in the call:
virProcessSetNamespaces((size_t)VIR_LXC_DOMAIN_NAMESPACE_LAST, ns_fd)
?
Yes typecasting works - some like it, some don't - so I played it safe
with a new variable.
John
Assuming there's a reason for the way you did it vs. this, ACK
with the
copyright updated.
9:31 a.m.
New subject: [libvirt] [PATCH 3/5] virfile: Avoid Coverity IDENTICAL_BRANCHES error
In virFileNBDDeviceFindUnused if virFileNBDDeviceIsBusy returns 0,
then both branches jumped to cleanup, so just use ignore_value
since the function returns NULL or some memory and the caller
handles the error.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virfile.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index e3c00ef..408d2d9 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -797,8 +797,7 @@ virFileNBDDeviceFindUnused(void)
if (rv < 0)
goto cleanup;
if (rv == 0) {
- if (virAsprintf(&ret, "/dev/%s", de->d_name) < 0)
- goto cleanup;
+ ignore_value(virAsprintf(&ret, "/dev/%s", de->d_name));
goto cleanup;
}
}
--
2.1.0
12:43 p.m.
New subject: [libvirt] [PATCH 3/5] virfile: Avoid Coverity IDENTICAL_BRANCHES error
On 09/04/2015 10:31 AM, John Ferlan wrote:
In virFileNBDDeviceFindUnused if virFileNBDDeviceIsBusy returns 0,
then both branches jumped to cleanup, so just use ignore_value
since the function returns NULL or some memory and the caller
handles the error.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virfile.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index e3c00ef..408d2d9 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -797,8 +797,7 @@ virFileNBDDeviceFindUnused(void)
if (rv < 0)
goto cleanup;
if (rv == 0) {
- if (virAsprintf(&ret, "/dev/%s", de->d_name) < 0)
- goto cleanup;
+ ignore_value(virAsprintf(&ret, "/dev/%s",
de->d_name));
goto cleanup;
}
}
ACK.
9:31 a.m.
New subject: [libvirt] [PATCH 4/5] util: Avoid Coverity FORWARD_NULL
Coverity claims it could be possible to call virDBusTypeStackFree with
*stack == NULL and although the two API's that call it don't appear to
allow that - I suppose it's better to be safe than sorry
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virdbus.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index 1cf1eef..78fb795 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -544,6 +544,10 @@ static void virDBusTypeStackFree(virDBusTypeStack **stack,
size_t *nstack)
{
size_t i;
+
+ if (!*stack)
+ return;
+
/* The iter in the first level of the stack is the
* root iter which must not be freed
*/
--
2.1.0
12:46 p.m.
New subject: [libvirt] [PATCH 4/5] util: Avoid Coverity FORWARD_NULL
On 09/04/2015 10:31 AM, John Ferlan wrote:
Coverity claims it could be possible to call virDBusTypeStackFree
with
*stack == NULL and although the two API's that call it don't appear to
allow that - I suppose it's better to be safe than sorry
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virdbus.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index 1cf1eef..78fb795 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -544,6 +544,10 @@ static void virDBusTypeStackFree(virDBusTypeStack **stack,
size_t *nstack)
{
size_t i;
+
+ if (!*stack)
+ return;
+
/* The iter in the first level of the stack is the
* root iter which must not be freed
*/
ACK.
9:31 a.m.
New subject: [libvirt] [PATCH 5/5] conf: Remove need for a couple of sa_asserts
Remove the need for a couple of sa_asserts.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f95190f..6df1618 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -24067,10 +24067,11 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
unsigned int flags)
{
struct virDomainListData data = { NULL, 0 };
+ ssize_t hash_size;
virObjectLock(domlist);
- sa_assert(domlist->objs);
- if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) {
+ if ((hash_size = virHashSize(domlist->objs)) < 0 ||
+ (VIR_ALLOC_N(data.vms, hash_size) < 0)) {
virObjectUnlock(domlist);
return -1;
}
@@ -24132,8 +24133,8 @@ virDomainObjListConvert(virDomainObjListPtr domlist,
}
virObjectUnlock(domlist);
- sa_assert(*vms);
- virDomainObjListFilter(vms, nvms, conn, filter, flags);
+ if (*vms)
+ virDomainObjListFilter(vms, nvms, conn, filter, flags);
return 0;
--
2.1.0
12:49 p.m.
New subject: [libvirt] [PATCH 5/5] conf: Remove need for a couple of sa_asserts
On 09/04/2015 10:31 AM, John Ferlan wrote:
Remove the need for a couple of sa_asserts.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f95190f..6df1618 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -24067,10 +24067,11 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
unsigned int flags)
{
struct virDomainListData data = { NULL, 0 };
+ ssize_t hash_size;
virObjectLock(domlist);
- sa_assert(domlist->objs);
- if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) {
+ if ((hash_size = virHashSize(domlist->objs)) < 0 ||
+ (VIR_ALLOC_N(data.vms, hash_size) < 0)) {
virObjectUnlock(domlist);
return -1;
}
@@ -24132,8 +24133,8 @@ virDomainObjListConvert(virDomainObjListPtr domlist,
}
virObjectUnlock(domlist);
- sa_assert(*vms);
- virDomainObjListFilter(vms, nvms, conn, filter, flags);
+ if (*vms)
+ virDomainObjListFilter(vms, nvms, conn, filter, flags);
return 0;
ACK.
3367
days inactive
3367
days old
11 comments
2 participants
participants (2)
-
John Ferlan -
Laine Stump