[libvirt] [PATCH 0/8] Coverity related patches
Some related to "new-ish" changes that have caused Coverity to discover new issues and some from older changes from a pile of 50 or so that are not essentially "false positives". John Ferlan (8): vbox: Fix possible NULL deref conf: Remove ATTRIBUTE_NONNULL for virDomainQemuMonitorEventNew tests: Fix memory leak in mymain lxc: Remove unnecessary comment tests: Remove _NULLABLE in virNetDevExists mock qemu: Fix possible NULL deref in qemuDomainSaveImageStartVM tests: Add return value check in checkUserInfo tests: Fix logic to not have possible NULL deref src/conf/domain_event.c | 6 +++++- src/conf/domain_event.h | 2 +- src/lxc/lxc_container.c | 5 ----- src/qemu/qemu_driver.c | 2 +- src/vbox/vbox_common.c | 4 ++-- tests/qemuagenttest.c | 4 +++- tests/qemuhotplugtest.c | 3 +-- tests/qemuxml2argvmock.c | 2 +- tests/virbitmaptest.c | 6 ++++-- 9 files changed, 18 insertions(+), 16 deletions(-) -- 2.20.1
The @valueTypeUtf8 references need to use the STREQ_NULLABLE since they're variantly filled in by @valueTypeUtf16. Found by Coverity. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vbox/vbox_common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 5486e5ff3b..cdbec15dae 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3509,13 +3509,13 @@ vboxDumpDisplay(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) VBOX_UTF8_FREE(valueDisplayUtf8); } - if (STREQ(valueTypeUtf8, "sdl")) { + if (STREQ_NULLABLE(valueTypeUtf8, "sdl")) { graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_SDL; graphics->data.sdl.display = valueDisplayUtf8; valueDisplayUtf8 = NULL; } - if (STREQ(valueTypeUtf8, "gui")) { + if (STREQ_NULLABLE(valueTypeUtf8, "gui")) { graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; graphics->data.desktop.display = valueDisplayUtf8; valueDisplayUtf8 = NULL; -- 2.20.1
Commit 17561eb36 modified the logic to check "if (!event)" for an attribute that was not supposed to be passed as NULL. This causes the static checker/Coverity build to fail. Since the check is made, alter the header. Also add an error message since returning -1 without some sort of error message as previously would have happened with the failed VIR_STRDUP so that the eventual error doesn't get the default for some reason message. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_event.c | 6 +++++- src/conf/domain_event.h | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 644f6eb595..900d8f745e 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -1935,8 +1935,12 @@ virDomainQemuMonitorEventNew(int id, return NULL; /* event is mandatory, details are optional */ - if (!event) + if (!event) { + virReportError(VIR_ERR_INVALID_ARG, + _("unexpected event=NULL name=%s uuid=%s details=%s"), + name, uuidstr, NULLSTR(details)); goto error; + } ev->event = g_strdup(event); ev->seconds = seconds; diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index d1cfb81d62..0a4bce3d04 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -321,4 +321,4 @@ virDomainQemuMonitorEventNew(int id, long long seconds, unsigned int micros, const char *details) - ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); -- 2.20.1
On Sun, Nov 03, 2019 at 08:53:34AM -0500, John Ferlan wrote:
Commit 17561eb36 modified the logic to check "if (!event)" for an attribute that was not supposed to be passed as NULL. This causes the static checker/Coverity build to fail. Since the check is made, alter the header.
Oops, I forgot to push this already acked patch that removed the condition completely: https://www.redhat.com/archives/libvir-list/2019-October/msg01505.html Jano
Also add an error message since returning -1 without some sort of error message as previously would have happened with the failed VIR_STRDUP so that the eventual error doesn't get the default for some reason message.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_event.c | 6 +++++- src/conf/domain_event.h | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-)
Commit 944a35d7f0 added @fakerootdir; however, there are multiple paths out of mymain that didn't free the memory - so just use the g_autofree to resolve the potential leak. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemuhotplugtest.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 4ff2b38c83..ebf4582ac1 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -595,7 +595,7 @@ mymain(void) int ret = 0; struct qemuHotplugTestData data = {0}; struct testQemuHotplugCpuParams cpudata; - char *fakerootdir; + g_autofree char *fakerootdir = NULL; fakerootdir = g_strdup(FAKEROOTDIRTEMPLATE); @@ -875,7 +875,6 @@ mymain(void) if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakerootdir); - VIR_FREE(fakerootdir); qemuTestDriverFree(&driver); virObjectUnref(data.vm); -- 2.20.1
Commit 66e2adb2ba moved the code and the coverity comment which now was useless since the context was in lxcContainerSetupPivotRoot. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_container.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index a18b214a25..1fb9049c96 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1658,11 +1658,6 @@ static int lxcContainerUnmountForSharedRoot(const char *stateDir, /* Some versions of Linux kernel don't let you overmount * the selinux filesystem, so make sure we kill it first */ - /* Filed coverity bug for false positive 'USE_AFTER_FREE' due to swap - * of root->src with root->dst and the VIR_FREE(root->src) prior to the - * reset of root->src in lxcContainerPrepareRoot() - */ - /* coverity[deref_arg] */ if (lxcContainerUnmountSubtree(SELINUX_MOUNT, false) < 0) goto cleanup; #endif -- 2.20.1
The @ifname is listed as an ATTRIBUTE_NONNULL(1) parameter, so checking for _NULLABLE causes a coverity build failure - remove that and if it's NULL for the test let's fail miserably. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemuxml2argvmock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 914d2bcf9f..8143de1618 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -169,7 +169,7 @@ virNetDevSetMAC(const char *ifname G_GNUC_UNUSED, int virNetDevExists(const char *ifname) { - return STREQ_NULLABLE(ifname, "mytap0"); + return STREQ(ifname, "mytap0"); } -- 2.20.1
Commit 075523438 added a direct reference to @cookie even though it may be NULL as shown by a comment a few lines previous - so add the check here as well. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d17c18705b..56fcba8b2c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6831,7 +6831,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, qemuDomainFixupCPUs(vm, &cookie->cpu) < 0) goto cleanup; - if (!cookie->slirpHelper) + if (cookie && !cookie->slirpHelper) priv->disableSlirp = true; if (qemuProcessStart(conn, driver, vm, cookie ? cookie->cpu : NULL, -- 2.20.1
Commit 1c8113f9c added the call to virTypedParamsGetString without a return value check which caused Coverity to complain especially since other checks for the same function are made. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemuagenttest.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index cef9ae5fee..ae55086d17 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -1127,7 +1127,9 @@ checkUserInfo(virTypedParameterPtr params, snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "user.%zu.domain", nth); - virTypedParamsGetString(params, nparams, param_name, &domain); + if (virTypedParamsGetString(params, nparams, param_name, &domain) < 0) + return -1; + if (STRNEQ_NULLABLE(expDomain, domain)) { virReportError(VIR_ERR_INTERNAL_ERROR, "Expected domain '%s', got '%s'", -- 2.20.1
It's possible that virBitmapNewString returns NULL with an error string (and not an allocation failure that would abort); however, if virBitmapToString is called with a NULL @bitmap, then it will fail in an ugly manner. So rather than have if (!map && !str) logic, split the checks for each variable. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/virbitmaptest.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index 545e9272df..2808d9c880 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -682,9 +682,11 @@ test13(const void *opaque G_GNUC_UNUSED) for (i = 0; i < G_N_ELEMENTS(strings); i++) { map = virBitmapNewString(strings[i]); - str = virBitmapToString(map, false, true); + if (!map) + goto cleanup; - if (!map || !str) + str = virBitmapToString(map, false, true); + if (!str) goto cleanup; if (STRNEQ(strings[i], str)) { -- 2.20.1
On Sun, Nov 03, 2019 at 08:53:32AM -0500, John Ferlan wrote:
Some related to "new-ish" changes that have caused Coverity to discover new issues and some from older changes from a pile of 50 or so that are not essentially "false positives".
John Ferlan (8): vbox: Fix possible NULL deref conf: Remove ATTRIBUTE_NONNULL for virDomainQemuMonitorEventNew tests: Fix memory leak in mymain lxc: Remove unnecessary comment tests: Remove _NULLABLE in virNetDevExists mock qemu: Fix possible NULL deref in qemuDomainSaveImageStartVM tests: Add return value check in checkUserInfo tests: Fix logic to not have possible NULL deref
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrangé -
John Ferlan -
Ján Tomko