9:38 a.m.
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
9:38 a.m.
New subject: [libvirt PATCH 01/11] domain_conf: remove unused rc variable
Leftover after commit <479a8c1fa1e0f58d3165c0446cd1abd72160256e>.
Signed-off-by: Pavel Hrdina <phrdina(a)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
9:47 a.m.
New subject: [libvirt PATCH 01/11] domain_conf: remove unused rc variable
On Mon, Nov 16, 2020 at 16:38:48 +0100, Pavel Hrdina wrote:
Leftover after commit
<479a8c1fa1e0f58d3165c0446cd1abd72160256e>.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/conf/domain_conf.c | 4 ----
1 file changed, 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
9:38 a.m.
New subject: [libvirt PATCH 02/11] domain_conf: fix NULL dereference on error in virDomainObjCopyPersistentDef
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(a)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
9:49 a.m.
New subject: [libvirt PATCH 02/11] domain_conf: fix NULL dereference on error in virDomainObjCopyPersistentDef
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(a)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(a)redhat.com>
9:38 a.m.
New subject: [libvirt PATCH 03/11] hyperv_wmi: remove unreachable cleanup code
In the cleanup section @data will always be NULL.
Signed-off-by: Pavel Hrdina <phrdina(a)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
9:51 a.m.
New subject: [libvirt PATCH 03/11] hyperv_wmi: remove unreachable cleanup code
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(a)redhat.com>
---
src/hyperv/hyperv_wmi.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
9:38 a.m.
New subject: [libvirt PATCH 04/11] interface_backend_udev: refactor udevListInterfacesByStatus
Commit <2f3b7a5555c4cf4127ff3f8e00746eafcc91432c> replaced VIR_STRDUP
by g_strdup which made the error: path mostly useless.
Signed-off-by: Pavel Hrdina <phrdina(a)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
9:55 a.m.
New subject: [libvirt PATCH 04/11] interface_backend_udev: refactor udevListInterfacesByStatus
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(a)redhat.com>
---
src/interface/interface_backend_udev.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
9:38 a.m.
New subject: [libvirt PATCH 05/11] qemu_command: fix FD usage in qemuBuildInterfaceCommandLine
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(a)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
10:18 a.m.
New subject: [libvirt PATCH 05/11] qemu_command: fix FD usage in qemuBuildInterfaceCommandLine
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(a)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
10:42 a.m.
New subject: [libvirt PATCH 05/11] qemu_command: fix FD usage in qemuBuildInterfaceCommandLine
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(a)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
9:38 a.m.
New subject: [libvirt PATCH 06/11] qemu_monitor_json: explicitly ignore return values
Signed-off-by: Pavel Hrdina <phrdina(a)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
9:58 a.m.
New subject: [libvirt PATCH 06/11] qemu_monitor_json: explicitly ignore return values
On Mon, Nov 16, 2020 at 16:38:53 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)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.
10:09 a.m.
New subject: [libvirt PATCH 06/11] qemu_monitor_json: explicitly ignore return values
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(a)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
10:20 a.m.
New subject: [libvirt PATCH 06/11] qemu_monitor_json: explicitly ignore return values
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(a)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.
11:04 a.m.
New subject: [libvirt PATCH 06/11] qemu_monitor_json: explicitly ignore return values
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(a)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
1:38 a.m.
New subject: [libvirt PATCH 06/11] qemu_monitor_json: explicitly ignore return values
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(a)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.
9:38 a.m.
New subject: [libvirt PATCH 07/11] qemu_process: no need to check for NULL
Signed-off-by: Pavel Hrdina <phrdina(a)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
10:01 a.m.
New subject: [libvirt PATCH 07/11] qemu_process: no need to check for NULL
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(a)redhat.com>
---
src/qemu/qemu_process.c | 30 +++++++++++++-----------------
1 file changed, 13 insertions(+), 17 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
9:38 a.m.
New subject: [libvirt PATCH 08/11] remote_driver: remove unreachable cleanup code
In the cleanup section @info_ret will always be NULL.
Signed-off-by: Pavel Hrdina <phrdina(a)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
10:05 a.m.
New subject: [libvirt PATCH 08/11] remote_driver: remove unreachable cleanup code
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(a)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(a)redhat.com>
9:38 a.m.
New subject: [libvirt PATCH 09/11] virdevmapper: fix stat comparison in virDMSanitizepath
Introduced by commit <22494556542c676d1b9e7f1c1f2ea13ac17e1e3e>.
Signed-off-by: Pavel Hrdina <phrdina(a)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
10:08 a.m.
New subject: [libvirt PATCH 09/11] virdevmapper: fix stat comparison in virDMSanitizepath
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(a)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(a)redhat.com>
9:38 a.m.
New subject: [libvirt PATCH 10/11] vbox_common: unlock vbox_driver_lock before return
Signed-off-by: Pavel Hrdina <phrdina(a)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
10:09 a.m.
New subject: [libvirt PATCH 10/11] vbox_common: unlock vbox_driver_lock before return
On Mon, Nov 16, 2020 at 16:38:57 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)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(a)redhat.com>
9:38 a.m.
New subject: [libvirt PATCH 11/11] testutils: call va_end before return
Signed-off-by: Pavel Hrdina <phrdina(a)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
10:10 a.m.
New subject: [libvirt PATCH 11/11] testutils: call va_end before return
On Mon, Nov 16, 2020 at 16:38:58 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
tests/testutils.c | 1 +
1 file changed, 1 insertion(+)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
1457
days inactive
1459
days old
27 comments
2 participants
participants (2)
-
Pavel Hrdina -
Peter Krempa