[libvirt] [PATCH 0/5] Resolve some Coverity related errors
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
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@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
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.
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@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
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@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.
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@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.
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@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
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@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.
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@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
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@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.
Remove the need for a couple of sa_asserts. Signed-off-by: John Ferlan <jferlan@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
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@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.
participants (2)
-
John Ferlan -
Laine Stump