[PATCH] qemuDomainBlockStatsFlags: Guard disk lookup with a domain job

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

When there are two concurrent threads, we may dereference a NULL
pointer, even though it has been checked before:

1. Thread1: starts executing qemuDomainBlockStatsFlags() with nparams != 0.
            It finds given disk and successfully pass check for disk->info.alias
            not being NULL.
2. Thread2: starts executing qemuDomainDetachDeviceFlags() on the very same
            disk as Thread1 is working on.
3. Thread1: gets to qemuDomainObjBeginJob() where it sets a job on a
            domain.
4. Thread2: also tries to set a job. However, we are not guaranteed which
            thread wins. So assume it's Thread2 who can continue.
5. Thread2: does the actual detach and frees disk->info.alias
6. Thread2: quits the job
7. Thread1: now successfully acquires the job, and accesses a NULL pointer.
---
 src/qemu/qemu_driver.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 32b0522..f4bbd74 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8496,17 +8496,20 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
     if (!(vm = qemuDomObjFromDomain(dom)))
         goto cleanup;
 
+    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+        goto cleanup;
+
     if (!virDomainObjIsActive(vm)) {
         virReportError(VIR_ERR_OPERATION_INVALID,
                        "%s", _("domain is not running"));
-        goto cleanup;
+        goto endjob;
     }
 
     if (*nparams != 0) {
         if ((i = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
             virReportError(VIR_ERR_INVALID_ARG,
                            _("invalid path: %s"), path);
-            goto cleanup;
+            goto endjob;
         }
         disk = vm->def->disks[i];
 
@@ -8514,22 +8517,13 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
              virReportError(VIR_ERR_INTERNAL_ERROR,
                             _("missing disk device alias name for %s"),
                             disk->dst);
-             goto cleanup;
+             goto endjob;
         }
     }
 
     priv = vm->privateData;
     VIR_DEBUG("priv=%p, params=%p, flags=%x", priv, params, flags);
 
-    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
-        goto cleanup;
-
-    if (!virDomainObjIsActive(vm)) {
-        virReportError(VIR_ERR_OPERATION_INVALID,
-                       "%s", _("domain is not running"));
-        goto endjob;
-    }
-
     qemuDomainObjEnterMonitor(driver, vm);
     tmp = *nparams;
     ret = qemuMonitorGetBlockStatsParamsNumber(priv->mon, nparams);
-- 
1.8.1.5

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