From: John Ferlan <jferlan@redhat.com>
Date: Sat, 21 Jan 2017 17:59:14 +0000 (-0500)
Subject: util: Fix domain object leaks on closecallbacks
X-Git-Tag: CVE-2017-2635~190
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=48ad600916bc239fcb549b3d5dc4a1a7d49ba6c3;p=thirdparty%2Flibvirt.git

util: Fix domain object leaks on closecallbacks

Originally/discovered proposed by "Wang King <king.wang@huawei.com>"

When the virCloseCallbacksSet is first called, it increments the refcnt
on the domain object to ensure it doesn't get deleted before the callback
is called. The refcnt would be decremented in virCloseCallbacksUnset once
the entry is removed from the closeCallbacks has table.

When (mostly) normal shutdown occurs, the qemuProcessStop will end up
calling qemuProcessAutoDestroyRemove and will remove the callback from
the list and hash table normally and decrement the refcnt.

However, when qemuConnectClose calls virCloseCallbacksRun, it will scan
the (locked) closeCallbacks list for matching domain and callback function.
If an entry is found, it will be removed from the closeCallbacks list and
placed into a lookaside list to be processed when the closeCallbacks lock
is dropped. The callback function (e.g. qemuProcessAutoDestroy) is called
and will run qemuProcessStop. That code will fail to find the callback
in the list when qemuProcessAutoDestroyRemove is called and thus not decrement
the domain refcnt. Instead since the entry isn't found the code will just
return (mostly) harmlessly.

This patch will resolve the issue by taking another ref during the
search UUID process during virCloseCallackRun, decrementing the refcnt
taken by virCloseCallbacksSet, calling the callback routine and returning
overwriting the vm (since it could return NULL). Finally, it will call the
virDomainObjEndAPI to lower the refcnt and remove the lock taken during
the search UUID processing. This may cause the vm to be destroyed.
---

diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c
index 2f430cfabc..4db50e8b67 100644
--- a/src/util/virclosecallbacks.c
+++ b/src/util/virclosecallbacks.c
@@ -346,17 +346,24 @@ virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks,
     for (i = 0; i < list->nentries; i++) {
         virDomainObjPtr vm;
 
-        if (!(vm = virDomainObjListFindByUUID(domains,
-                                              list->entries[i].uuid))) {
+        /* Grab a ref and lock to the vm */
+        if (!(vm = virDomainObjListFindByUUIDRef(domains,
+                                                 list->entries[i].uuid))) {
             char uuidstr[VIR_UUID_STRING_BUFLEN];
             virUUIDFormat(list->entries[i].uuid, uuidstr);
             VIR_DEBUG("No domain object with UUID %s", uuidstr);
             continue;
         }
 
-        vm = list->entries[i].callback(vm, conn, opaque);
-        if (vm)
-            virObjectUnlock(vm);
+        /* Remove the ref taken out during virCloseCallbacksSet since
+         * we're about to call the callback function and we have another
+         * ref anyway (so it cannot be deleted).
+         *
+         * Call the callback function, ignoring the return since it might be
+         * NULL. Once we're done with the object, then end the API usage. */
+        virObjectUnref(vm);
+        ignore_value(list->entries[i].callback(vm, conn, opaque));
+        virDomainObjEndAPI(&vm);
     }
     VIR_FREE(list->entries);
     VIR_FREE(list);