The function updates vm->newDef and frees previous pesistent definition
which can be used by in different thread by some API that unlocks domain
to communicate to qemu. So we have an access to freed memory in that
thread. Let's acquire job condition in case of updating exiting domain.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---

diff from v1 [1]:
- pay a bit of attention to the problem

Reproducer in the Michal's reply to the [1].

[1] https://www.redhat.com/archives/libvir-list/2019-May/msg00804.html

 src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 42b1ce2..2dd4442 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7620,6 +7620,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
     virDomainDefPtr def = NULL;
     virDomainDefPtr oldDef = NULL;
     virDomainObjPtr vm = NULL;
+    virDomainObjPtr exvm = NULL;
     virDomainPtr dom = NULL;
     virObjectEventPtr event = NULL;
     virQEMUDriverConfigPtr cfg;
@@ -7647,10 +7648,34 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
     if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
         goto cleanup;
 
+    if ((exvm = virDomainObjListFindByUUID(driver->domains, def->uuid))) {
+        if (qemuDomainObjBeginJob(driver, exvm, QEMU_JOB_MODIFY) < 0) {
+            virDomainObjEndAPI(&exvm);
+            goto cleanup;
+        }
+        virObjectUnlock(exvm);
+    }
+
     if (!(vm = virDomainObjListAdd(driver->domains, def,
                                    driver->xmlopt,
-                                   0, &oldDef)))
+                                   0, &oldDef))) {
+        if (exvm)
+            virObjectLock(exvm);
         goto cleanup;
+    }
+
+    if (exvm) {
+        if (vm != exvm) {
+            virReportError(VIR_ERR_OPERATION_FAILED,
+                           _("domain '%s' is removed"), def->name);
+            virObjectLock(exvm);
+            goto cleanup;
+        }
+
+        /* we don't need extra reference for same domain */
+        virObjectUnref(exvm);
+    }
+
     def = NULL;
 
     vm->persistent = 1;
@@ -7670,7 +7695,10 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
             /* Brand new domain. Remove it */
             VIR_INFO("Deleting domain '%s'", vm->def->name);
             vm->persistent = 0;
-            qemuDomainRemoveInactiveJob(driver, vm);
+            if (exvm)
+                qemuDomainRemoveInactive(driver, vm);
+            else
+                qemuDomainRemoveInactiveJob(driver, vm);
         }
         goto cleanup;
     }
@@ -7685,8 +7713,13 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
     dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
 
  cleanup:
+    if (exvm)
+        qemuDomainObjEndJob(driver, exvm);
+
     virDomainDefFree(oldDef);
     virDomainDefFree(def);
+    if (exvm && exvm != vm)
+        virDomainObjEndAPI(&exvm);
     virDomainObjEndAPI(&vm);
     virObjectEventStateQueue(driver->domainEventState, event);
     virObjectUnref(caps);
-- 
1.8.3.1

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

On 5/28/19 6:11 PM, Nikolay Shirokovskiy wrote:
> The function updates vm->newDef and frees previous pesistent definition
> which can be used by in different thread by some API that unlocks domain
> to communicate to qemu. So we have an access to freed memory in that
> thread. Let's acquire job condition in case of updating exiting domain.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
> 
> diff from v1 [1]:
> - pay a bit of attention to the problem
> 
> Reproducer in the Michal's reply to the [1].
> 
> [1] https://www.redhat.com/archives/libvir-list/2019-May/msg00804.html
> 
>   src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++++++++++++++--
>   1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 42b1ce2..2dd4442 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7620,6 +7620,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
>       virDomainDefPtr def = NULL;
>       virDomainDefPtr oldDef = NULL;
>       virDomainObjPtr vm = NULL;
> +    virDomainObjPtr exvm = NULL;
>       virDomainPtr dom = NULL;
>       virObjectEventPtr event = NULL;
>       virQEMUDriverConfigPtr cfg;
> @@ -7647,10 +7648,34 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
>       if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
>           goto cleanup;
>   
> +    if ((exvm = virDomainObjListFindByUUID(driver->domains, def->uuid))) {
> +        if (qemuDomainObjBeginJob(driver, exvm, QEMU_JOB_MODIFY) < 0) {
> +            virDomainObjEndAPI(&exvm);
> +            goto cleanup;
> +        }
> +        virObjectUnlock(exvm);
> +    }
> +
>       if (!(vm = virDomainObjListAdd(driver->domains, def,
>                                      driver->xmlopt,
> -                                   0, &oldDef)))
> +                                   0, &oldDef))) {
> +        if (exvm)
> +            virObjectLock(exvm);
>           goto cleanup;
> +    }
> +

Almost. There are other places where virDomainObjListAdd() is called and 
they require the same threatment. I'm posting a different approach.

Michal

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