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 <[email protected]> Signed-off-by: Michal Privoznik <[email protected]>
open-power-host-os · Jun 13, 2016 · 9ac9450 · 9ac9450
1 parent fc1b242
commit 9ac9450
Show file tree

Hide file tree

Showing 7 changed files with 87 additions and 112 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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)