libxl: fix vm lock overwritten bug

In libxl driver we do virObjectRef in libxlDomainObjBeginJob,
If virCondWaitUntil failed, it goes to error, do virObjectUnref,
There's a chance that someone undefine the vm at the same time,
and refs unref to zero, vm is freed in libxlDomainObjBeginJob.
But the vm outside function is not Null, we do virObjectUnlock(vm).
That's how we overwrite the vm memory after it's freed. I fix it.

Signed-off-by: Wang Yufei <james.wangyufei@huawei.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 27590c873fa7f86eec77f6ddd5a6d53b50b41d14..485671e74b53bd3ff7d9a1ee10e148742bdcafb4 100644 (file)
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -112,24 +112,45 @@ static int virDomainObjListSearchID(const void *payload,
     return want;
 }
 
-
-virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms,
-                                         int id)
+static virDomainObjPtr
+virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
+                                 int id,
+                                 bool ref)
 {
     virDomainObjPtr obj;
     virObjectLock(doms);
     obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id);
+    if (ref) {
+        virObjectRef(obj);
+        virObjectUnlock(doms);
+    }
     if (obj) {
         virObjectLock(obj);
         if (obj->removing) {
             virObjectUnlock(obj);
+            if (ref)
+                virObjectUnref(obj);
             obj = NULL;
         }
     }
-    virObjectUnlock(doms);
+    if (!ref)
+        virObjectUnlock(doms);
     return obj;
 }
 
+virDomainObjPtr
+virDomainObjListFindByID(virDomainObjListPtr doms,
+                         int id)
+{
+    return virDomainObjListFindByIDInternal(doms, id, false);
+}
+
+virDomainObjPtr
+virDomainObjListFindByIDRef(virDomainObjListPtr doms,
+                            int id)
+{
+    return virDomainObjListFindByIDInternal(doms, id, true);
+}
 
 static virDomainObjPtr
 virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,

diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h
index c0f856c6e7aacc432c9a8bce29c53437488d5cf1..3c59bd39fed4571854a95b5c4e2b40a7e57d51e8 100644 (file)
--- a/src/conf/virdomainobjlist.h
+++ b/src/conf/virdomainobjlist.h
@@ -34,6 +34,8 @@ virDomainObjListPtr virDomainObjListNew(void);
 
 virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms,
                                          int id);
+virDomainObjPtr virDomainObjListFindByIDRef(virDomainObjListPtr doms,
+                                            int id);
 virDomainObjPtr virDomainObjListFindByUUID(virDomainObjListPtr doms,
                                            const unsigned char *uuid);
 virDomainObjPtr virDomainObjListFindByUUIDRef(virDomainObjListPtr doms,

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 85b9cd14ae7cec00065e264c415b4ce45ca35870..b42e2cdcb5256233391a0941539c4ad55ad6f719 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -888,6 +888,7 @@ virDomainObjListCollect;
 virDomainObjListConvert;
 virDomainObjListExport;
 virDomainObjListFindByID;
+virDomainObjListFindByIDRef;
 virDomainObjListFindByName;
 virDomainObjListFindByUUID;
 virDomainObjListFindByUUIDRef;

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 4a21f68e2251d67fe290053771b4fab9f37cc91a..ec1ff51c6ed301e70cc23b44962c47a67337bbaa 100644 (file)
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -123,8 +123,6 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
         return -1;
     then = now + LIBXL_JOB_WAIT_TIME;
 
-    virObjectRef(obj);
-
     while (priv->job.active) {
         VIR_DEBUG("Wait normal job condition for starting job: %s",
                   libxlDomainJobTypeToString(job));
@@ -157,7 +155,6 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
         virReportSystemError(errno,
                              "%s", _("cannot acquire job mutex"));
 
-    virObjectUnref(obj);
     return -1;
 }
 
@@ -171,7 +168,7 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
  * non-zero, false if the reference count has dropped to zero
  * and obj is disposed.
  */
-bool
+void
 libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
                      virDomainObjPtr obj)
 {
@@ -183,8 +180,6 @@ libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
 
     libxlDomainObjResetJob(priv);
     virCondSignal(&priv->job.cond);
-
-    return virObjectUnref(obj);
 }
 
 int
@@ -532,12 +527,10 @@ libxlDomainShutdownThread(void *opaque)
     }
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     if (dom_event)
         libxlDomainEventQueue(driver, dom_event);
     libxl_event_free(cfg->ctx, ev);
@@ -570,7 +563,7 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
     if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND)
         goto error;
 
-    vm = virDomainObjListFindByID(driver->domains, event->domid);
+    vm = virDomainObjListFindByIDRef(driver->domains, event->domid);
     if (!vm) {
         VIR_INFO("Received event for unknown domain ID %d", event->domid);
         goto error;
@@ -605,8 +598,7 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
     /* Cast away any const */
     libxl_event_free(cfg->ctx, (libxl_event *)event);
     virObjectUnref(cfg);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     VIR_FREE(shutdown_info);
 }
 

diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
index c53adaa1e29ef4f2ad931ccf1b2c791afd8d89eb..af11a2c83d464b2f2ec1ce7688aed0526074af4f 100644 (file)
--- a/src/libxl/libxl_domain.h
+++ b/src/libxl/libxl_domain.h
@@ -85,10 +85,9 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver,
                        enum libxlDomainJob job)
     ATTRIBUTE_RETURN_CHECK;
 
-bool
+void
 libxlDomainObjEndJob(libxlDriverPrivatePtr driver,
-                     virDomainObjPtr obj)
-    ATTRIBUTE_RETURN_CHECK;
+                     virDomainObjPtr obj);
 
 int
 libxlDomainJobUpdateTime(struct libxlDomainJobObj *job)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 2ab08ac76c376fdae9fe97936662bca61cbf4ba0..79f96dc32af5faa2b64adf620c251ec777390e7e 100644 (file)
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -290,7 +290,7 @@ libxlDomObjFromDomain(virDomainPtr dom)
     libxlDriverPrivatePtr driver = dom->conn->privateData;
     char uuidstr[VIR_UUID_STRING_BUFLEN];
 
-    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
+    vm = virDomainObjListFindByUUIDRef(driver->domains, dom->uuid);
     if (!vm) {
         virUUIDFormat(dom->uuid, uuidstr);
         virReportError(VIR_ERR_NO_DOMAIN,
@@ -309,13 +309,12 @@ libxlAutostartDomain(virDomainObjPtr vm,
     libxlDriverPrivatePtr driver = opaque;
     int ret = -1;
 
+    virObjectRef(vm);
     virObjectLock(vm);
     virResetLastError();
 
-    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
-        virObjectUnlock(vm);
-        return ret;
-    }
+    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+        goto cleanup;
 
     if (vm->autostart && !virDomainObjIsActive(vm) &&
         libxlDomainStartNew(driver, vm, false) < 0) {
@@ -328,8 +327,9 @@ libxlAutostartDomain(virDomainObjPtr vm,
     ret = 0;
 
  endjob:
-    if (libxlDomainObjEndJob(driver, vm))
-        virObjectUnlock(vm);
+    libxlDomainObjEndJob(driver, vm);
+ cleanup:
+    virDomainObjEndAPI(&vm);
 
     return ret;
 }
@@ -983,6 +983,7 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
                                    VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
                                    NULL)))
         goto cleanup;
+    virObjectRef(vm);
     def = NULL;
 
     if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
@@ -1008,13 +1009,11 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
         dom->id = vm->def->id;
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
     virDomainDefFree(def);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return dom;
 }
@@ -1141,12 +1140,10 @@ libxlDomainSuspend(virDomainPtr dom)
     ret = 0;
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     if (event)
         libxlDomainEventQueue(driver, event);
     virObjectUnref(cfg);
@@ -1200,12 +1197,10 @@ libxlDomainResume(virDomainPtr dom)
     ret = 0;
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     if (event)
         libxlDomainEventQueue(driver, event);
     virObjectUnref(cfg);
@@ -1373,12 +1368,10 @@ libxlDomainDestroyFlags(virDomainPtr dom,
     ret = 0;
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     if (event)
         libxlDomainEventQueue(driver, event);
     virObjectUnref(cfg);
@@ -1517,12 +1510,10 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
     ret = 0;
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -1746,16 +1737,14 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml,
     ret = 0;
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
     if (remove_dom && vm) {
         virDomainObjListRemove(driver->domains, vm);
         vm = NULL;
     }
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -1802,7 +1791,7 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
                                    VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
                                    NULL)))
         goto cleanup;
-
+    virObjectRef(vm);
     def = NULL;
 
     if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
@@ -1819,15 +1808,13 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
     if (ret < 0 && !vm->persistent)
         virDomainObjListRemove(driver->domains, vm);
 
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
     if (VIR_CLOSE(fd) < 0)
         virReportSystemError(errno, "%s", _("cannot close file"));
     virDomainDefFree(def);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -1923,16 +1910,14 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags)
     }
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
     if (remove_dom && vm) {
         virDomainObjListRemove(driver->domains, vm);
         vm = NULL;
     }
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     if (event)
         libxlDomainEventQueue(driver, event);
     virObjectUnref(cfg);
@@ -1986,16 +1971,14 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags)
     ret = 0;
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
     if (remove_dom && vm) {
         virDomainObjListRemove(driver->domains, vm);
         vm = NULL;
     }
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     VIR_FREE(name);
     return ret;
 }
@@ -2212,14 +2195,12 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
     }
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
     VIR_FREE(bitmask);
-     if (vm)
-        virObjectUnlock(vm);
-     virObjectUnref(cfg);
+    virDomainObjEndAPI(&vm);
+    virObjectUnref(cfg);
     return ret;
 }
 
@@ -2357,12 +2338,10 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu,
     }
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virBitmapFree(pcpumap);
     virObjectUnref(cfg);
     return ret;
@@ -2685,12 +2664,10 @@ libxlDomainCreateWithFlags(virDomainPtr dom,
     dom->id = vm->def->id;
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -3695,14 +3672,12 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
     }
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
     virDomainDefFree(vmdef);
     virDomainDeviceDefFree(dev);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -3788,14 +3763,12 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
     }
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
     virDomainDefFree(vmdef);
     virDomainDeviceDefFree(dev);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -4076,14 +4049,12 @@ libxlDomainSetAutostart(virDomainPtr dom, int autostart)
     ret = 0;
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
     VIR_FREE(configFile);
     VIR_FREE(autostartLink);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -4288,12 +4259,10 @@ libxlDomainSetSchedulerParametersFlags(virDomainPtr dom,
     ret = 0;
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -4609,12 +4578,10 @@ libxlDomainInterfaceStats(virDomainPtr dom,
                        _("'%s' is not a known interface"), path);
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -4791,13 +4758,11 @@ libxlDomainMemoryStats(virDomainPtr dom,
     ret = i;
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
     libxl_dominfo_dispose(&d_info);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -5364,8 +5329,7 @@ libxlDomainMigrateFinish3Params(virConnectPtr dconn,
 
     ret = libxlDomainMigrationFinish(dconn, vm, flags, cancelled);
 
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
     virDomainObjEndAPI(&vm);
 

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 539ee1bb23b3b68bf71b98144989bf8c219b5b9f..bf285a4278690e2a02dcae34dc00f314d2cc0366 100644 (file)
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -266,6 +266,7 @@ libxlDoMigrateReceive(void *opaque)
     int ret;
     bool remove_dom = 0;
 
+    virObjectRef(vm);
     virObjectLock(vm);
     if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
         goto cleanup;
@@ -291,16 +292,14 @@ libxlDoMigrateReceive(void *opaque)
     VIR_FORCE_CLOSE(recvfd);
     virObjectUnref(args);
 
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
     if (remove_dom && vm) {
         virDomainObjListRemove(driver->domains, vm);
         vm = NULL;
     }
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
 }
 
 
@@ -437,14 +436,11 @@ libxlDomainMigrationBegin(virConnectPtr conn,
     xml = virDomainDefFormat(def, cfg->caps, VIR_DOMAIN_DEF_FORMAT_SECURE);
 
  endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
+    libxlDomainObjEndJob(driver, vm);
 
  cleanup:
     libxlMigrationCookieFree(mig);
-    if (vm)
-        virObjectUnlock(vm);
-
+    virDomainObjEndAPI(&vm);
     virDomainDefFree(tmpdef);
     virObjectUnref(cfg);
     return xml;