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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Sep 06, 2019 at 04:25:17PM +0200, Michal Privoznik wrote:
>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@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(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list