9:25 a.m.
In some places where virDomainObjListForEach() is called the
passed callback calls virDomainObjListRemoveLocked(). Well, this
is unsafe, because the former only grabs a read lock but the
latter modifies the list.
I've identified the following unsafe calls:
- qemuProcessReconnectAll()
- libxlReconnectDomains()
The rest seem to be safe.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/bhyve/bhyve_driver.c | 2 +-
src/bhyve/bhyve_process.c | 2 +-
src/conf/virdomainobjlist.c | 12 ++++++++++--
src/conf/virdomainobjlist.h | 1 +
src/libxl/libxl_driver.c | 11 +++++++----
src/lxc/lxc_process.c | 4 ++--
src/qemu/qemu_process.c | 3 ++-
src/vmware/vmware_driver.c | 2 +-
8 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index e2c1b00080..c52def7607 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -114,7 +114,7 @@ bhyveAutostartDomains(bhyveConnPtr driver)
struct bhyveAutostartData data = { driver, conn };
- virDomainObjListForEach(driver->domains, bhyveAutostartDomain, &data);
+ virDomainObjListForEach(driver->domains, false, bhyveAutostartDomain, &data);
virObjectUnref(conn);
}
diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
index 4dab6e5e54..5fea2eb51c 100644
--- a/src/bhyve/bhyve_process.c
+++ b/src/bhyve/bhyve_process.c
@@ -466,7 +466,7 @@ virBhyveProcessReconnectAll(bhyveConnPtr driver)
data.driver = driver;
data.kd = kd;
- virDomainObjListForEach(driver->domains, virBhyveProcessReconnect, &data);
+ virDomainObjListForEach(driver->domains, false, virBhyveProcessReconnect,
&data);
kvm_close(kd);
}
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 11fd68745b..2eff6768c2 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -818,25 +818,33 @@ virDomainObjListHelper(void *payload,
/**
* virDomainObjListForEach:
* @doms: Pointer to the domain object list
+ * @modify: Whether to lock @doms for modify operation
* @callback: callback to run over each domain on the list
* @opaque: opaque data to pass to @callback
*
* For every domain on the list (@doms) run @callback on it. If
* @callback fails (i.e. returns a negative value), the iteration
- * carries still on until all domains are visited.
+ * carries still on until all domains are visited. Moreover, if
+ * @callback wants to modify the list of domains (@doms) then
+ * @modify must be set to true.
*
* Returns: 0 on success,
* -1 otherwise.
*/
int
virDomainObjListForEach(virDomainObjListPtr doms,
+ bool modify,
virDomainObjListIterator callback,
void *opaque)
{
struct virDomainListIterData data = {
callback, opaque, 0,
};
- virObjectRWLockRead(doms);
+
+ if (modify)
+ virObjectRWLockWrite(doms);
+ else
+ virObjectRWLockRead(doms);
virHashForEach(doms->objs, virDomainObjListHelper, &data);
virObjectRWUnlock(doms);
return data.ret;
diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h
index 7d71bc54d0..da5ec8a57c 100644
--- a/src/conf/virdomainobjlist.h
+++ b/src/conf/virdomainobjlist.h
@@ -91,6 +91,7 @@ typedef int (*virDomainObjListIterator)(virDomainObjPtr dom,
void *opaque);
int virDomainObjListForEach(virDomainObjListPtr doms,
+ bool modify,
virDomainObjListIterator callback,
void *opaque);
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index d0396e4781..5e14aa4fbe 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -497,7 +497,7 @@ libxlReconnectDomain(virDomainObjPtr vm,
static void
libxlReconnectDomains(libxlDriverPrivatePtr driver)
{
- virDomainObjListForEach(driver->domains, libxlReconnectDomain, driver);
+ virDomainObjListForEach(driver->domains, true, libxlReconnectDomain, driver);
}
static int
@@ -800,10 +800,12 @@ libxlStateInitialize(bool privileged,
NULL, NULL) < 0)
goto error;
- virDomainObjListForEach(libxl_driver->domains, libxlAutostartDomain,
+ virDomainObjListForEach(libxl_driver->domains, false,
+ libxlAutostartDomain,
libxl_driver);
- virDomainObjListForEach(libxl_driver->domains, libxlDomainManagedSaveLoad,
+ virDomainObjListForEach(libxl_driver->domains, false,
+ libxlDomainManagedSaveLoad,
libxl_driver);
return VIR_DRV_STATE_INIT_COMPLETE;
@@ -832,7 +834,8 @@ libxlStateReload(void)
libxl_driver->xmlopt,
NULL, libxl_driver);
- virDomainObjListForEach(libxl_driver->domains, libxlAutostartDomain,
+ virDomainObjListForEach(libxl_driver->domains, false,
+ libxlAutostartDomain,
libxl_driver);
virObjectUnref(cfg);
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index cd65e7a0c0..cbdc7b1268 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -1639,7 +1639,7 @@ virLXCProcessAutostartAll(virLXCDriverPtr driver)
struct virLXCProcessAutostartData data = { driver, conn };
- virDomainObjListForEach(driver->domains,
+ virDomainObjListForEach(driver->domains, false,
virLXCProcessAutostartDomain,
&data);
@@ -1760,6 +1760,6 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm,
int virLXCProcessReconnectAll(virLXCDriverPtr driver,
virDomainObjListPtr doms)
{
- virDomainObjListForEach(doms, virLXCProcessReconnectDomain, driver);
+ virDomainObjListForEach(doms, false, virLXCProcessReconnectDomain, driver);
return 0;
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1960f53466..6a261786f9 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8369,7 +8369,8 @@ void
qemuProcessReconnectAll(virQEMUDriverPtr driver)
{
struct qemuProcessReconnectData data = {.driver = driver};
- virDomainObjListForEach(driver->domains, qemuProcessReconnectHelper, &data);
+ virDomainObjListForEach(driver->domains, true,
+ qemuProcessReconnectHelper, &data);
}
diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
index 1bc8a06c39..a87af85916 100644
--- a/src/vmware/vmware_driver.c
+++ b/src/vmware/vmware_driver.c
@@ -990,7 +990,7 @@ static int vmwareDomainObjListUpdateDomain(virDomainObjPtr dom, void
*data)
static void
vmwareDomainObjListUpdateAll(virDomainObjListPtr doms, struct vmware_driver *driver)
{
- virDomainObjListForEach(doms, vmwareDomainObjListUpdateDomain, driver);
+ virDomainObjListForEach(doms, false, vmwareDomainObjListUpdateDomain, driver);
}
static int
--
2.21.0