[libvirt PATCH 00/11] fix various issues
Issues reported by coverity. Pavel Hrdina (11): domain_conf: remove unused rc variable domain_conf: fix NULL dereference on error in virDomainObjCopyPersistentDef hyperv_wmi: remove unreachable cleanup code interface_backend_udev: refactor udevListInterfacesByStatus qemu_command: fix FD usage in qemuBuildInterfaceCommandLine qemu_monitor_json: explicitly ignore return values qemu_process: no need to check for NULL remote_driver: remove unreachable cleanup code virdevmapper: fix stat comparison in virDMSanitizepath vbox_common: unlock vbox_driver_lock before return testutils: call va_end before return src/conf/domain_conf.c | 10 +++++---- src/hyperv/hyperv_wmi.c | 12 ++--------- src/interface/interface_backend_udev.c | 13 ++--------- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_monitor_json.c | 4 ++-- src/qemu/qemu_process.c | 30 +++++++++++--------------- src/remote/remote_driver.c | 18 ++++------------ src/util/virdevmapper.c | 2 +- src/vbox/vbox_common.c | 3 +++ tests/testutils.c | 1 + 10 files changed, 35 insertions(+), 60 deletions(-) -- 2.26.2
Leftover after commit <479a8c1fa1e0f58d3165c0446cd1abd72160256e>. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9199771dc0..5c30227212 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27074,7 +27074,6 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferEscapeString(buf, "<model type='%s'/>\n", virDomainNetGetModelString(def)); if (virDomainNetIsVirtioModel(def)) { - int rc = 0; g_autofree char *str = NULL; g_autofree char *gueststr = NULL; g_autofree char *hoststr = NULL; @@ -27099,9 +27098,6 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</driver>\n"); } - - if (rc < 0) - return -1; } } if (def->backend.tap || def->backend.vhost) { -- 2.26.2
On Mon, Nov 16, 2020 at 16:38:48 +0100, Pavel Hrdina wrote:
Leftover after commit <479a8c1fa1e0f58d3165c0446cd1abd72160256e>.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 4 ---- 1 file changed, 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The issue was introduced together with the function itself by commit <da1eba6bc8f58bfce34136710d1979a3a44adb17>. Calling `virDomainObjGetPersistentDef` may return NULL which is later passed to `virDomainDefFormat` where the `def` attribute is marked as NONNULL and later in `virDomainDefFormatInternalSetRootName` it is actually defererenced without any other check. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5c30227212..eaad72ad0a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30933,6 +30933,12 @@ virDomainObjCopyPersistentDef(virDomainObjPtr dom, virDomainDefPtr cur; cur = virDomainObjGetPersistentDef(xmlopt, dom, parseOpaque); + if (!cur) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Get persistent config failed")); + return NULL; + } + return virDomainDefCopy(cur, xmlopt, parseOpaque, false); } -- 2.26.2
On Mon, Nov 16, 2020 at 16:38:49 +0100, Pavel Hrdina wrote:
The issue was introduced together with the function itself by commit <da1eba6bc8f58bfce34136710d1979a3a44adb17>. Calling `virDomainObjGetPersistentDef` may return NULL which is later passed to `virDomainDefFormat` where the `def` attribute is marked as NONNULL and later in `virDomainDefFormatInternalSetRootName` it is actually defererenced without any other check.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5c30227212..eaad72ad0a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30933,6 +30933,12 @@ virDomainObjCopyPersistentDef(virDomainObjPtr dom, virDomainDefPtr cur;
cur = virDomainObjGetPersistentDef(xmlopt, dom, parseOpaque); + if (!cur) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Get persistent config failed"));
"failed to get persistent definition object" Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In the cleanup section @data will always be NULL. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/hyperv/hyperv_wmi.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index 866b347bc2..efd0659051 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -942,7 +942,6 @@ hypervEnumAndPull(hypervPrivate *priv, hypervWqlQueryPtr wqlQuery, hypervObject *head = NULL; hypervObject *tail = NULL; WsXmlNodeH node = NULL; - XML_TYPE_PTR data = NULL; hypervObject *object; query_string = virBufferContentAndReset(wqlQuery->query); @@ -983,6 +982,8 @@ hypervEnumAndPull(hypervPrivate *priv, hypervWqlQueryPtr wqlQuery, response = NULL; while (enumContext != NULL && *enumContext != '\0') { + XML_TYPE_PTR data = NULL; + response = wsmc_action_pull(priv->client, wmiInfo->resourceUri, options, filter, enumContext); @@ -1030,8 +1031,6 @@ hypervEnumAndPull(hypervPrivate *priv, hypervWqlQueryPtr wqlQuery, object->info = wmiInfo; object->data = data; - data = NULL; - if (head == NULL) { head = object; } else { @@ -1059,13 +1058,6 @@ hypervEnumAndPull(hypervPrivate *priv, hypervWqlQueryPtr wqlQuery, if (filter != NULL) filter_destroy(filter); - if (data != NULL) { - if (ws_serializer_free_mem(serializerContext, data, - wmiInfo->serializerInfo) < 0) { - VIR_ERROR(_("Could not free deserialized data")); - } - } - VIR_FREE(query_string); ws_xml_destroy_doc(response); VIR_FREE(enumContext); -- 2.26.2
On Mon, Nov 16, 2020 at 16:38:50 +0100, Pavel Hrdina wrote:
In the cleanup section @data will always be NULL.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/hyperv/hyperv_wmi.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Commit <2f3b7a5555c4cf4127ff3f8e00746eafcc91432c> replaced VIR_STRDUP by g_strdup which made the error: path mostly useless. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/interface/interface_backend_udev.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 4e0a80765c..ce007fd29e 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -204,7 +204,8 @@ udevListInterfacesByStatus(virConnectPtr conn, virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to get list of %s interfaces on host"), virUdevStatusString(status)); - goto error; + udev_unref(udev); + return -1; } /* Do the scan to load up the enumeration */ @@ -239,16 +240,6 @@ udevListInterfacesByStatus(virConnectPtr conn, udev_unref(udev); return count; - - error: - if (enumerate) - udev_enumerate_unref(enumerate); - udev_unref(udev); - - for (names_len = 0; names_len < count; names_len++) - VIR_FREE(names[names_len]); - - return -1; } static int -- 2.26.2
On Mon, Nov 16, 2020 at 16:38:51 +0100, Pavel Hrdina wrote:
Commit <2f3b7a5555c4cf4127ff3f8e00746eafcc91432c> replaced VIR_STRDUP by g_strdup which made the error: path mostly useless.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/interface/interface_backend_udev.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
If virCommandPassFD() is called with VIR_COMMAND_PASS_FD_CLOSE_PARENT the passed FD is closed and should not be used after that call. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0eec35da16..0580feb475 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8142,7 +8142,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, g_autofree char *fdset = NULL; g_autofree char *addfdarg = NULL; - virCommandPassFD(cmd, vdpafd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandPassFD(cmd, vdpafd, 0); fdset = qemuVirCommandGetFDSet(cmd, vdpafd); if (!fdset) goto cleanup; -- 2.26.2
On Mon, Nov 16, 2020 at 16:38:52 +0100, Pavel Hrdina wrote:
If virCommandPassFD() is called with VIR_COMMAND_PASS_FD_CLOSE_PARENT the passed FD is closed and should not be used after that call.
This description doesn't seem to make sense. The filedescriptor itself isn't really used in the caller. The only thing that is used is the number of the filedescriptor which is used to format the string being passed to qemu which is should be correct.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0eec35da16..0580feb475 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8142,7 +8142,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, g_autofree char *fdset = NULL; g_autofree char *addfdarg = NULL;
- virCommandPassFD(cmd, vdpafd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandPassFD(cmd, vdpafd, 0); fdset = qemuVirCommandGetFDSet(cmd, vdpafd);
A proper fix will be to move 'vdpafd = -1;' here or copy it's value somewhere to prevent the double close ...
if (!fdset) goto cleanup;
... on this condition.
-- 2.26.2
On Mon, Nov 16, 2020 at 05:18:51PM +0100, Peter Krempa wrote:
On Mon, Nov 16, 2020 at 16:38:52 +0100, Pavel Hrdina wrote:
If virCommandPassFD() is called with VIR_COMMAND_PASS_FD_CLOSE_PARENT the passed FD is closed and should not be used after that call.
This description doesn't seem to make sense. The filedescriptor itself isn't really used in the caller. The only thing that is used is the number of the filedescriptor which is used to format the string being passed to qemu which is should be correct.
Right, the description was not accurate. By "should not be used" I meant that we close it again in the cleanup section.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0eec35da16..0580feb475 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8142,7 +8142,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, g_autofree char *fdset = NULL; g_autofree char *addfdarg = NULL;
- virCommandPassFD(cmd, vdpafd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandPassFD(cmd, vdpafd, 0); fdset = qemuVirCommandGetFDSet(cmd, vdpafd);
A proper fix will be to move 'vdpafd = -1;' here or copy it's value somewhere to prevent the double close ...
Moving the 'vdpafd = -1;' here would introduce a different bug because 'vdpafd' is used after that condition as well. So I guess the only correct solution is to copy the value to a different variable. Thanks for the review, I'll send a v2. Pavel
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_monitor_json.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 723bdb4426..51730a5fe3 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1370,8 +1370,8 @@ qemuMonitorJSONHandleMemoryFailure(qemuMonitorPtr mon, } if (flagsjson) { - virJSONValueObjectGetBoolean(flagsjson, "action-required", &ar); - virJSONValueObjectGetBoolean(flagsjson, "recursive", &recursive); + ignore_value(virJSONValueObjectGetBoolean(flagsjson, "action-required", &ar)); + ignore_value(virJSONValueObjectGetBoolean(flagsjson, "recursive", &recursive)); } mf.recipient = recipient; -- 2.26.2
On Mon, Nov 16, 2020 at 16:38:53 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_monitor_json.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
I presume this silences a coverity moan about "most of the callers check the value". NACK in that case.
On Mon, Nov 16, 2020 at 04:58:56PM +0100, Peter Krempa wrote:
On Mon, Nov 16, 2020 at 16:38:53 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_monitor_json.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
I presume this silences a coverity moan about "most of the callers check the value". NACK in that case.
Yes it does. In that case we should remove ignore_value() from the remaining calls to that function that are not checked. Pavel
On Mon, Nov 16, 2020 at 17:09:17 +0100, Pavel Hrdina wrote:
On Mon, Nov 16, 2020 at 04:58:56PM +0100, Peter Krempa wrote:
On Mon, Nov 16, 2020 at 16:38:53 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_monitor_json.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
I presume this silences a coverity moan about "most of the callers check the value". NACK in that case.
Yes it does. In that case we should remove ignore_value() from the remaining calls to that function that are not checked.
Either that, or for a full fix add G_GNUC_UNUSED to the function header. Without that nothing would be really fixed and noting will be prevented in the future.
On Mon, Nov 16, 2020 at 05:20:00PM +0100, Peter Krempa wrote:
On Mon, Nov 16, 2020 at 17:09:17 +0100, Pavel Hrdina wrote:
On Mon, Nov 16, 2020 at 04:58:56PM +0100, Peter Krempa wrote:
On Mon, Nov 16, 2020 at 16:38:53 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_monitor_json.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
I presume this silences a coverity moan about "most of the callers check the value". NACK in that case.
Yes it does. In that case we should remove ignore_value() from the remaining calls to that function that are not checked.
Either that, or for a full fix add G_GNUC_UNUSED to the function header. Without that nothing would be really fixed and noting will be prevented in the future.
I guess you mean using G_GNUC_WARN_UNUSED_RESULT. There are other cases where coverity complains that the return value is not checked but probably not all cases, only where the majority is checked. I'll ignore these issues for now. Pavel
On Mon, Nov 16, 2020 at 18:04:38 +0100, Pavel Hrdina wrote:
On Mon, Nov 16, 2020 at 05:20:00PM +0100, Peter Krempa wrote:
On Mon, Nov 16, 2020 at 17:09:17 +0100, Pavel Hrdina wrote:
On Mon, Nov 16, 2020 at 04:58:56PM +0100, Peter Krempa wrote:
On Mon, Nov 16, 2020 at 16:38:53 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_monitor_json.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
I presume this silences a coverity moan about "most of the callers check the value". NACK in that case.
Yes it does. In that case we should remove ignore_value() from the remaining calls to that function that are not checked.
Either that, or for a full fix add G_GNUC_UNUSED to the function header. Without that nothing would be really fixed and noting will be prevented in the future.
I guess you mean using G_GNUC_WARN_UNUSED_RESULT.
Yes, that one, sorry.
There are other cases where coverity complains that the return value is not checked but probably not all cases, only where the majority is checked.
Yeah, this heuristic is of questionable value. I can see that it might find "some" cases of problems, but the annoying part is that it can appear randomly after adding a totally irrelevant piece of code. I'd specifically not address this kind of problems unless there's a real bug.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_process.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e2a3d87527..579b3c3713 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1105,24 +1105,20 @@ qemuProcessHandleGraphics(qemuMonitorPtr mon G_GNUC_UNUSED, return 0; error: - if (localAddr) { - VIR_FREE(localAddr->service); - VIR_FREE(localAddr->node); - VIR_FREE(localAddr); - } - if (remoteAddr) { - VIR_FREE(remoteAddr->service); - VIR_FREE(remoteAddr->node); - VIR_FREE(remoteAddr); - } - if (subject) { - for (i = 0; i < subject->nidentity; i++) { - VIR_FREE(subject->identities[i].type); - VIR_FREE(subject->identities[i].name); - } - VIR_FREE(subject->identities); - VIR_FREE(subject); + VIR_FREE(localAddr->service); + VIR_FREE(localAddr->node); + VIR_FREE(localAddr); + + VIR_FREE(remoteAddr->service); + VIR_FREE(remoteAddr->node); + VIR_FREE(remoteAddr); + + for (i = 0; i < subject->nidentity; i++) { + VIR_FREE(subject->identities[i].type); + VIR_FREE(subject->identities[i].name); } + VIR_FREE(subject->identities); + VIR_FREE(subject); return -1; } -- 2.26.2
On Mon, Nov 16, 2020 at 16:38:54 +0100, Pavel Hrdina wrote: Mention the function name that is being fixed either instead of "qemu_process: " in the summary or in the comment.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_process.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In the cleanup section @info_ret will always be NULL. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/remote/remote_driver.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9cd2fd36ae..dd5e8eeed2 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2248,7 +2248,6 @@ remoteDomainGetIOThreadInfo(virDomainPtr dom, remote_domain_get_iothread_info_args args; remote_domain_get_iothread_info_ret ret; remote_domain_iothread_info *src; - virDomainIOThreadInfoPtr *info_ret = NULL; remoteDriverLock(priv); @@ -2273,6 +2272,8 @@ remoteDomainGetIOThreadInfo(virDomainPtr dom, } if (info) { + virDomainIOThreadInfoPtr *info_ret = NULL; + if (!ret.info.info_len) { *info = NULL; rv = ret.ret; @@ -2293,17 +2294,11 @@ remoteDomainGetIOThreadInfo(virDomainPtr dom, info_ret[i]->cpumaplen = src->cpumap.cpumap_len; } *info = info_ret; - info_ret = NULL; } rv = ret.ret; cleanup: - if (info_ret) { - for (i = 0; i < ret.info.info_len; i++) - virDomainIOThreadInfoFree(info_ret[i]); - VIR_FREE(info_ret); - } xdr_free((xdrproc_t)xdr_remote_domain_get_iothread_info_ret, (char *) &ret); @@ -7638,7 +7633,6 @@ remoteDomainGetFSInfo(virDomainPtr dom, remote_domain_get_fsinfo_args args; remote_domain_get_fsinfo_ret ret; remote_domain_fsinfo *src; - virDomainFSInfoPtr *info_ret = NULL; remoteDriverLock(priv); @@ -7661,6 +7655,8 @@ remoteDomainGetFSInfo(virDomainPtr dom, } if (info) { + virDomainFSInfoPtr *info_ret = NULL; + if (!ret.info.info_len) { *info = NULL; rv = ret.ret; @@ -7690,17 +7686,11 @@ remoteDomainGetFSInfo(virDomainPtr dom, } *info = info_ret; - info_ret = NULL; } rv = ret.ret; cleanup: - if (info_ret) { - for (i = 0; i < ret.info.info_len; i++) - virDomainFSInfoFree(info_ret[i]); - VIR_FREE(info_ret); - } xdr_free((xdrproc_t)xdr_remote_domain_get_fsinfo_ret, (char *) &ret); -- 2.26.2
On Mon, Nov 16, 2020 at 16:38:55 +0100, Pavel Hrdina wrote:
In the cleanup section @info_ret will always be NULL.
The two instances of 'info_ret' are actually completely distinct variables of distinct types. That's quite sneaky and the commit message is deceiving.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/remote/remote_driver.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)
If you split the commit: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Introduced by commit <22494556542c676d1b9e7f1c1f2ea13ac17e1e3e>. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/virdevmapper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index 6c39a2a44d..c4719d0670 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -204,7 +204,7 @@ virDMSanitizepath(const char *path) g_autofree char *tmp = g_strdup_printf(DEV_DM_DIR "/%s", ent->d_name); if (stat(tmp, &sb[1]) == 0 && - sb[0].st_rdev == sb[0].st_rdev) { + sb[0].st_rdev == sb[1].st_rdev) { return g_steal_pointer(&tmp); } } -- 2.26.2
On Mon, Nov 16, 2020 at 16:38:56 +0100, Pavel Hrdina wrote:
Introduced by commit <22494556542c676d1b9e7f1c1f2ea13ac17e1e3e>.
This is a real bug and the commit message neglects to mention what the implications are. This basically returns the first entry of /dev/mapper/ if the previous conditions don't match. That seems serious. Especially since the original commit fixes a CVE!
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/virdevmapper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index 6c39a2a44d..c4719d0670 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -204,7 +204,7 @@ virDMSanitizepath(const char *path) g_autofree char *tmp = g_strdup_printf(DEV_DM_DIR "/%s", ent->d_name);
if (stat(tmp, &sb[1]) == 0 && - sb[0].st_rdev == sb[0].st_rdev) { + sb[0].st_rdev == sb[1].st_rdev) { return g_steal_pointer(&tmp);
If you improve the commit message: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/vbox/vbox_common.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 26168c3c5b..fc897735b0 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -245,6 +245,9 @@ vboxGetDriverConnection(void) if (!vbox_driver) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to create vbox driver object.")); + + virMutexUnlock(&vbox_driver_lock); + return NULL; } } -- 2.26.2
On Mon, Nov 16, 2020 at 16:38:57 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/vbox/vbox_common.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 26168c3c5b..fc897735b0 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -245,6 +245,9 @@ vboxGetDriverConnection(void) if (!vbox_driver) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to create vbox driver object.")); + + virMutexUnlock(&vbox_driver_lock); +
Mention the function name somewshere. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/testutils.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/testutils.c b/tests/testutils.c index 2e586e98bf..7ecf7923b8 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -771,6 +771,7 @@ int virTestMain(int argc, while ((lib = va_arg(ap, const char *))) { if (!virFileIsExecutable(lib)) { perror(lib); + va_end(ap); return EXIT_FAILURE; } -- 2.26.2
On Mon, Nov 16, 2020 at 16:38:58 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/testutils.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Pavel Hrdina -
Peter Krempa