From 6b42792cfee124a742999e698e348e99b382ba16 Mon Sep 17 00:00:00 2001
From: c00207022 <caoxinhua@huawei.com>
Date: Thu, 2 Mar 2017 12:07:27 +0800
Subject: [PATCH] libvirt-remote:fix use-after-free when sending event message

[Changelog]:If there is a process with a client which registers event callbacks,
and it calls libvirt's API which uses the same virConnectPtr in that
callback function. When this process exit abnormally lead to client
disconnect, there is a possibility that the libvirtd's main thread is use
virServerClient to send event just after the virServerClient been freed by job
thread.
Following is backtrace:

#0  0x00007fda223d66d8 in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x7fda24c81b40) at util/virobject.c:169
#1  0x00007fda223d6a1e in virObjectIsClass (anyobj=anyobj@entry=0x7fd9e575b400, klass=<optimized out>) at util/virobject.c:365
#2  0x00007fda223d6a44 in virObjectLock (anyobj=0x7fd9e575b400) at util/virobject.c:317
#3  0x00007fda22507f71 in virNetServerClientSendMessage (client=client@entry=0x7fd9e575b400, msg=msg@entry=0x7fd9ec30de90)
at rpc/virnetserverclient.c:1422
#4  0x00007fda230d714d in remoteDispatchObjectEventSend (client=0x7fd9e575b400, program=0x7fda24c844e0, procnr=348,
proc=0x7fda2310e5e0 <xdr_remote_domain_event_callback_tunable_msg>, data=0x7ffc3857fdb0) at remote.c:3803
#5  0x00007fda230dd71b in remoteRelayDomainEventTunable (conn=<optimized out>, dom=0x7fda27cd7660, params=0x7fda27f3aae0, nparams=1,
opaque=0x7fd9e6c99e00) at remote.c:1033
#6  0x00007fda224484cb in virDomainEventDispatchDefaultFunc (conn=0x7fda27cd0120, event=0x7fda2736ea00,
cb=0x7fda230dd610 <remoteRelayDomainEventTunable>, cbopaque=0x7fd9e6c99e00) at conf/domain_event.c:1910
#7  0x00007fda22446871 in virObjectEventStateDispatchCallbacks (callbacks=<optimized out>, callbacks=<optimized out>, event=0x7fda2736ea00,
state=0x7fda24ca3960) at conf/object_event.c:722
#8  virObjectEventStateQueueDispatch (callbacks=0x7fda24c65800, queue=0x7ffc3857fe90, state=0x7fda24ca3960) at conf/object_event.c:736
#9  virObjectEventStateFlush (state=0x7fda24ca3960) at conf/object_event.c:814
#10 virObjectEventTimer (timer=<optimized out>, opaque=0x7fda24ca3960) at conf/object_event.c:560
#11 0x00007fda223ae8b9 in virEventPollDispatchTimeouts () at util/vireventpoll.c:458
#12 virEventPollRunOnce () at util/vireventpoll.c:654
#13 0x00007fda223ad1d2 in virEventRunDefaultImpl () at util/virevent.c:314
#14 0x00007fda225046cd in virNetDaemonRun (dmn=0x7fda24c775c0) at rpc/virnetdaemon.c:818
#15 0x00007fda230d6351 in main (argc=<optimized out>, argv=<optimized out>) at libvirtd.c:1623

Let's clean all event callback when client close.

Signed-off-by: Caoxinhua <caoxinhua@huawei.com>
---
 daemon/remote.c | 76 +++++++++++++++++++++------------------------------------
 1 file changed, 28 insertions(+), 48 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 23c9de4..fb7cd50 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -101,6 +101,7 @@ static void make_nonnull_node_device(remote_nonnull_node_device *dev_dst, virNod
 static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr secret_src);
 static void make_nonnull_nwfilter(remote_nonnull_nwfilter *net_dst, virNWFilterPtr nwfilter_src);
 static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src);
+static void remoteClientCleanEventCallbacks(struct daemonClientPrivate *priv);
 
 static int
 remoteSerializeDomainDiskErrors(virDomainDiskErrorPtr errors,
@@ -1483,24 +1484,15 @@ void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int r
                                   &msg);
 }
 
-/*
- * You must hold lock for at least the client
- * We don't free stuff here, merely disconnect the client's
- * network socket & resources.
- * We keep the libvirt connection open until any async
- * jobs have finished, then clean it up elsewhere
- */
-void remoteClientFreeFunc(void *data)
+static void remoteClientCleanEventCallbacks(struct daemonClientPrivate *priv)
 {
-    struct daemonClientPrivate *priv = data;
-
     /* Deregister event delivery callback */
-    if (priv->conn) {
-        virIdentityPtr sysident = virIdentityGetSystem();
+    if (priv && priv->conn) {
         size_t i;
-
+        virIdentityPtr sysident = virIdentityGetSystem();
         virIdentitySetCurrent(sysident);
 
+        virMutexLock(&priv->lock);
         for (i = 0; i < priv->ndomainEventCallbacks; i++) {
             int callbackID = priv->domainEventCallbacks[i]->callbackID;
             if (callbackID < 0) {
@@ -1514,6 +1506,7 @@ void remoteClientFreeFunc(void *data)
                 VIR_WARN("unexpected domain event deregister failure");
         }
         VIR_FREE(priv->domainEventCallbacks);
+        priv->ndomainEventCallbacks = 0;
 
         for (i = 0; i < priv->nnetworkEventCallbacks; i++) {
             int callbackID = priv->networkEventCallbacks[i]->callbackID;
@@ -1529,36 +1522,7 @@ void remoteClientFreeFunc(void *data)
                 VIR_WARN("unexpected network event deregister failure");
         }
         VIR_FREE(priv->networkEventCallbacks);
-
-        for (i = 0; i < priv->nstorageEventCallbacks; i++) {
-            int callbackID = priv->storageEventCallbacks[i]->callbackID;
-            if (callbackID < 0) {
-                VIR_WARN("unexpected incomplete storage pool callback %zu", i);
-                continue;
-            }
-            VIR_DEBUG("Deregistering remote storage pool event relay %d",
-                      callbackID);
-            priv->storageEventCallbacks[i]->callbackID = -1;
-            if (virConnectStoragePoolEventDeregisterAny(priv->conn,
-                                                        callbackID) < 0)
-                VIR_WARN("unexpected storage pool event deregister failure");
-        }
-        VIR_FREE(priv->storageEventCallbacks);
-
-        for (i = 0; i < priv->nnodeDeviceEventCallbacks; i++) {
-            int callbackID = priv->nodeDeviceEventCallbacks[i]->callbackID;
-            if (callbackID < 0) {
-                VIR_WARN("unexpected incomplete node device callback %zu", i);
-                continue;
-            }
-            VIR_DEBUG("Deregistering remote node device event relay %d",
-                      callbackID);
-            priv->nodeDeviceEventCallbacks[i]->callbackID = -1;
-            if (virConnectNodeDeviceEventDeregisterAny(priv->conn,
-                                                       callbackID) < 0)
-                VIR_WARN("unexpected node device event deregister failure");
-        }
-        VIR_FREE(priv->nodeDeviceEventCallbacks);
+        priv->nnetworkEventCallbacks = 0;
 
         for (i = 0; i < priv->nqemuEventCallbacks; i++) {
             int callbackID = priv->qemuEventCallbacks[i]->callbackID;
@@ -1574,31 +1538,47 @@ void remoteClientFreeFunc(void *data)
                 VIR_WARN("unexpected qemu monitor event deregister failure");
         }
         VIR_FREE(priv->qemuEventCallbacks);
+        priv->nqemuEventCallbacks = 0;
 
         if (priv->closeRegistered) {
             if (virConnectUnregisterCloseCallback(priv->conn,
                                                   remoteRelayConnectionClosedEvent) < 0)
                 VIR_WARN("unexpected close callback event deregister failure");
         }
-
-        virConnectClose(priv->conn);
-
+        virMutexUnlock(&priv->lock);
         virIdentitySetCurrent(NULL);
         virObjectUnref(sysident);
     }
+}
+
+
+/*
+ * You must hold lock for at least the client
+ * We don't free stuff here, merely disconnect the client's
+ * network socket & resources.
+ * We keep the libvirt connection open until any async
+ * jobs have finished, then clean it up elsewhere
+ */
+void remoteClientFreeFunc(void *data)
+{
+    struct daemonClientPrivate *priv = data;
+    if (!priv || !priv->conn)
+        return;
+
+    remoteClientCleanEventCallbacks(priv);
+    virConnectClose(priv->conn);
 
     VIR_FREE(priv);
 }
 
-
 static void remoteClientCloseFunc(virNetServerClientPtr client)
 {
     struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
 
     daemonRemoveAllClientStreams(priv->streams);
+    remoteClientCleanEventCallbacks(priv);
 }
 
-
 void *remoteClientInitHook(virNetServerClientPtr client,
                            void *opaque ATTRIBUTE_UNUSED)
 {
-- 
1.8.3.1

