11:24 a.m.
These stem from me playing with OOM testing. I've discovered more bugs,
but they're mostly in tests themselves, e.g. use without check.
Michal Prívozník (4):
virstorageobj: Don't clear vols if they weren't initialized
virNetServerPreExecRestart: Check for retval of virJSONValueNewArray()
virCommand: Make virCommandPassFDGetFDIndex fail if passed command is
in error state
storagepoolxml2argvtest: Avoid double free
src/conf/virstorageobj.c | 6 ++++--
src/rpc/virnetserver.c | 8 ++++++--
src/util/vircommand.c | 3 +++
tests/storagepoolxml2argvtest.c | 1 -
4 files changed, 13 insertions(+), 5 deletions(-)
--
2.21.0
11:24 a.m.
New subject: [libvirt] [PATCH 1/4] virstorageobj: Don't clear vols if they weren't initialized
If virStoragePoolObjNew() fails to create new volume object list
then virObjectUnref() is called and since refcounter is 1 then
virStoragePoolObjDispose() is called which in turn calls
virStoragePoolObjClearVols() which in turn dereferences
obj->volumes.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/virstorageobj.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 1d6c9d1937..1d5c88f50b 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -365,8 +365,10 @@ virStoragePoolObjDispose(void *opaque)
if (!obj)
return;
- virStoragePoolObjClearVols(obj);
- virObjectUnref(obj->volumes);
+ if (obj->volumes) {
+ virStoragePoolObjClearVols(obj);
+ virObjectUnref(obj->volumes);
+ }
virStoragePoolDefFree(obj->def);
virStoragePoolDefFree(obj->newDef);
--
2.21.0
3:40 p.m.
New subject: [libvirt] [PATCH 1/4] virstorageobj: Don't clear vols if they weren't initialized
On Tue, May 14, 2019 at 11:24:09AM +0200, Michal Privoznik wrote:
If virStoragePoolObjNew() fails to create new volume object list
then virObjectUnref() is called and since refcounter is 1 then
virStoragePoolObjDispose() is called which in turn calls
virStoragePoolObjClearVols() which in turn dereferences
obj->volumes.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/virstorageobj.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 1d6c9d1937..1d5c88f50b 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -365,8 +365,10 @@ virStoragePoolObjDispose(void *opaque)
if (!obj)
return;
- virStoragePoolObjClearVols(obj);
- virObjectUnref(obj->volumes);
+ if (obj->volumes) {
+ virStoragePoolObjClearVols(obj);
+ virObjectUnref(obj->volumes);
I think the check is better suited to live inside virStoragePoolObjClearVols as
there are multiple callers to virStoragePoolObjClearVols, just to be on the
safer side.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
11:24 a.m.
New subject: [libvirt] [PATCH 2/4] virNetServerPreExecRestart: Check for retval of virJSONValueNewArray()
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/rpc/virnetserver.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 83b871764f..4934dba967 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -629,7 +629,9 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv)
goto error;
}
- services = virJSONValueNewArray();
+ if (!(services = virJSONValueNewArray()))
+ goto error;
+
if (virJSONValueObjectAppend(object, "services", services) < 0) {
virJSONValueFree(services);
goto error;
@@ -646,7 +648,9 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv)
}
}
- clients = virJSONValueNewArray();
+ if (!(clients = virJSONValueNewArray()))
+ goto error;
+
if (virJSONValueObjectAppend(object, "clients", clients) < 0) {
virJSONValueFree(clients);
goto error;
--
2.21.0
3:34 p.m.
New subject: [libvirt] [PATCH 2/4] virNetServerPreExecRestart: Check for retval of virJSONValueNewArray()
On Tue, May 14, 2019 at 11:24:10AM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/rpc/virnetserver.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
11:24 a.m.
New subject: [libvirt] [PATCH 3/4] virCommand: Make virCommandPassFDGetFDIndex fail if passed command is in error state
The idea of virCommand* APIs is that a possible error that
occurred while constructing cmd line is kept in virCommand
struct. If that's the case all subsequent calls to virCommand*()
are NO-OPs or they return an error. Well,
virCommandPassFDGetFDIndex() is not honoring that.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/vircommand.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 9e99697c55..8695c98d1b 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -1034,6 +1034,9 @@ virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd)
{
size_t i = 0;
+ if (!cmd || cmd->has_error)
+ return -1;
+
while (i < cmd->npassfd) {
if (cmd->passfd[i].fd == fd)
return i;
--
2.21.0
3:40 p.m.
New subject: [libvirt] [PATCH 3/4] virCommand: Make virCommandPassFDGetFDIndex fail if passed command is in error state
On Tue, May 14, 2019 at 11:24:11AM +0200, Michal Privoznik wrote:
The idea of virCommand* APIs is that a possible error that
occurred while constructing cmd line is kept in virCommand
struct. If that's the case all subsequent calls to virCommand*()
are NO-OPs or they return an error. Well,
virCommandPassFDGetFDIndex() is not honoring that.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
3:44 p.m.
New subject: [libvirt] [PATCH 3/4] virCommand: Make virCommandPassFDGetFDIndex fail if passed command is in error state
On Tue, May 14, 2019 at 11:24:11AM +0200, Michal Privoznik wrote:
The idea of virCommand* APIs is that a possible error that
occurred while constructing cmd line is kept in virCommand
struct. If that's the case all subsequent calls to virCommand*()
are NO-OPs or they return an error. Well,
virCommandPassFDGetFDIndex() is not honoring that.
This is the flaw that caused the crash I reported yesterday when
running qemuxml2argvtest with ENOMEM simulation active.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/vircommand.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 9e99697c55..8695c98d1b 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -1034,6 +1034,9 @@ virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd)
{
size_t i = 0;
+ if (!cmd || cmd->has_error)
+ return -1;
+
while (i < cmd->npassfd) {
if (cmd->passfd[i].fd == fd)
return i;
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 :|
11:24 a.m.
New subject: [libvirt] [PATCH 4/4] storagepoolxml2argvtest: Avoid double free
A double free may occur in testCompareXMLToArgvFiles() when @def
is freed right after virStoragePoolObjNew() failed and the second
time at cleanup label.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/storagepoolxml2argvtest.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c
index b7e32064af..0c01931946 100644
--- a/tests/storagepoolxml2argvtest.c
+++ b/tests/storagepoolxml2argvtest.c
@@ -39,7 +39,6 @@ testCompareXMLToArgvFiles(bool shouldFail,
case VIR_STORAGE_POOL_NETFS:
if (!(pool = virStoragePoolObjNew())) {
VIR_TEST_DEBUG("pool type '%s' alloc pool obj fails\n",
defTypeStr);
- virStoragePoolDefFree(def);
goto cleanup;
}
virStoragePoolObjSetDef(pool, def);
--
2.21.0
3:42 p.m.
New subject: [libvirt] [PATCH 4/4] storagepoolxml2argvtest: Avoid double free
On Tue, May 14, 2019 at 11:24:12AM +0200, Michal Privoznik wrote:
A double free may occur in testCompareXMLToArgvFiles() when @def
is freed right after virStoragePoolObjNew() failed and the second
time at cleanup label.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1955
days inactive
1955
days old
9 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Erik Skultety -
Michal Privoznik