rpc: Fix connection close callback race condition and memory corruption/crash

The last Viktor's effort to fix the race and memory corruption unfortunately
wasn't complete in the case the close callback was not registered in an
connection. At that time, the trail of event's that I'll describe later could
still happen and corrupt the memory or cause a crash of the client (including
the daemon in case of a p2p migration).

Consider the following prerequisities and trail of events:
Let's have a remote connection to a hypervisor that doesn't have a close
callback registered and the client is using the event loop. The crash happens in
cooperation of 2 threads. Thread E is the event loop and thread W is the worker
that does some stuff. R denotes the remote client.

1.) W - The client finishes everything and sheds the last reference on the client
2.) W - The virObject stuff invokes virConnectDispose that invokes doRemoteClose
3.) W - the remote close method invokes the REMOTE_PROC_CLOSE RPC method.
4.) W - The thread is preempted at this point.
5.) R - The remote side receives the close and closes the socket.
6.) E - poll() wakes up due to the closed socket and invokes the close callback
7.) E - The event loop is preempted right before remoteClientCloseFunc is called
8.) W - The worker now finishes, and frees the conn object.
9.) E - The remoteClientCloseFunc accesses the now-freed conn object in the
        attempt to retrieve pointer for the real close callback.
10.) Kaboom, corrupted memory/segfault.

This patch tries to fix this by introducing a new object that survives the
freeing of the connection object. We can't increase the reference count on the
connection object itself or the connection would never be closed, as the
connection is closed only when the reference count reaches zero.

The new object - virConnectCloseCallbackData - is a lockable object that keeps
the pointers to the real user registered callback and ensures that the
connection callback is either not called if the connection was already freed or
that the connection isn't freed while this is being called.

src/datatypes.c

@@ -37,6 +37,7 @@
 
 
 virClassPtr virConnectClass;
+virClassPtr virConnectCloseCallbackDataClass;
 virClassPtr virDomainClass;
 virClassPtr virDomainSnapshotClass;
 virClassPtr virInterfaceClass;
@@ -49,6 +50,7 @@ virClassPtr virStorageVolClass;
 virClassPtr virStoragePoolClass;
 
 static void virConnectDispose(void *obj);
+static void virConnectCloseCallbackDataDispose(void *obj);
 static void virDomainDispose(void *obj);
 static void virDomainSnapshotDispose(void *obj);
 static void virInterfaceDispose(void *obj);
@@ -63,14 +65,19 @@ static void virStoragePoolDispose(void *obj);
 static int
 virDataTypesOnceInit(void)
 {
-#define DECLARE_CLASS(basename)                                  \
-    if (!(basename ## Class = virClassNew(virClassForObject(),   \
+#define DECLARE_CLASS_COMMON(basename, parent)                   \
+    if (!(basename ## Class = virClassNew(parent,                \
                                           #basename,             \
                                           sizeof(basename),      \
                                           basename ## Dispose))) \
         return -1;
+#define DECLARE_CLASS(basename)                                  \
+    DECLARE_CLASS_COMMON(basename, virClassForObject())
+#define DECLARE_CLASS_LOCKABLE(basename)                         \
+    DECLARE_CLASS_COMMON(basename, virClassForObjectLockable())
 
     DECLARE_CLASS(virConnect);
+    DECLARE_CLASS_LOCKABLE(virConnectCloseCallbackData);
     DECLARE_CLASS(virDomain);
     DECLARE_CLASS(virDomainSnapshot);
     DECLARE_CLASS(virInterface);
@@ -82,6 +89,8 @@ virDataTypesOnceInit(void)
     DECLARE_CLASS(virStorageVol);
     DECLARE_CLASS(virStoragePool);
 
+#undef DECLARE_CLASS_COMMON
+#undef DECLARE_CLASS_LOCKABLE
 #undef DECLARE_CLASS
 
     return 0;
@@ -107,12 +116,17 @@ virGetConnect(void)
     if (!(ret = virObjectNew(virConnectClass)))
         return NULL;
 
-    if (virMutexInit(&ret->lock) < 0) {
-        VIR_FREE(ret);
-        return NULL;
-    }
+    if (!(ret->closeCallback = virObjectNew(virConnectCloseCallbackDataClass)))
+        goto error;
+
+    if (virMutexInit(&ret->lock) < 0)
+        goto error;
 
     return ret;
+
+error:
+    virObjectUnref(ret);
+    return NULL;
 }
 
 /**
@@ -146,18 +160,41 @@ virConnectDispose(void *obj)
 
     virMutexLock(&conn->lock);
 
-    if (conn->closeFreeCallback)
-        conn->closeFreeCallback(conn->closeOpaque);
-
     virResetError(&conn->err);
 
     virURIFree(conn->uri);
 
+    virObjectLock(conn->closeCallback);
+    conn->closeCallback->callback = NULL;
+    virObjectUnlock(conn->closeCallback);
+
+    virObjectUnref(conn->closeCallback);
+
     virMutexUnlock(&conn->lock);
     virMutexDestroy(&conn->lock);
 }
 
 
+/**
+ * virConnectCloseCallbackDataDispose:
+ * @obj: the close callback data to release
+ *
+ * Release resources bound to the connection close callback.
+ */
+static void
+virConnectCloseCallbackDataDispose(void *obj)
+{
+    virConnectCloseCallbackDataPtr cb = obj;
+
+    virObjectLock(cb);
+
+    if (cb->freeCallback)
+        cb->freeCallback(cb->opaque);
+
+    virObjectUnlock(cb);
+}
+
+
 /**
  * virGetDomain:
  * @conn: the hypervisor connection

src/datatypes.h

@@ -93,6 +93,22 @@ extern virClassPtr virStoragePoolClass;
 # define VIR_IS_DOMAIN_SNAPSHOT(obj) \
     (VIR_IS_SNAPSHOT(obj) && VIR_IS_DOMAIN((obj)->domain))
 
+
+typedef struct _virConnectCloseCallbackData virConnectCloseCallbackData;
+typedef virConnectCloseCallbackData *virConnectCloseCallbackDataPtr;
+
+/**
+ * Internal structure holding data related to connection close callbacks.
+ */
+struct _virConnectCloseCallbackData {
+    virObjectLockable parent;
+
+    virConnectPtr conn;
+    virConnectCloseFunc callback;
+    void *opaque;
+    virFreeCallback freeCallback;
+};
+
 /**
  * _virConnect:
  *
@@ -142,11 +158,7 @@ struct _virConnect {
     void *userData;         /* the user data */
 
     /* Per-connection close callback */
-    virConnectCloseFunc closeCallback;
-    void *closeOpaque;
-    virFreeCallback closeFreeCallback;
-    bool closeDispatch;
-    unsigned closeUnregisterCount;
+    virConnectCloseCallbackDataPtr closeCallback;
 };
 
 /**

src/libvirt.c

@@ -20189,24 +20189,27 @@ int virConnectRegisterCloseCallback(virConnectPtr conn,
     virObjectRef(conn);
 
     virMutexLock(&conn->lock);
+    virObjectLock(conn->closeCallback);
 
     virCheckNonNullArgGoto(cb, error);
 
-    if (conn->closeCallback) {
+    if (conn->closeCallback->callback) {
         virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
                         _("A close callback is already registered"));
         goto error;
     }
 
-    conn->closeCallback = cb;
-    conn->closeOpaque = opaque;
-    conn->closeFreeCallback = freecb;
+    conn->closeCallback->callback = cb;
+    conn->closeCallback->opaque = opaque;
+    conn->closeCallback->freeCallback = freecb;
 
+    virObjectUnlock(conn->closeCallback);
     virMutexUnlock(&conn->lock);
 
     return 0;
 
 error:
+    virObjectUnlock(conn->closeCallback);
     virMutexUnlock(&conn->lock);
     virObjectUnref(conn);
     virDispatchError(NULL);
@@ -20240,29 +20243,29 @@ int virConnectUnregisterCloseCallback(virConnectPtr conn,
     }
 
     virMutexLock(&conn->lock);
+    virObjectLock(conn->closeCallback);
 
     virCheckNonNullArgGoto(cb, error);
 
-    if (conn->closeCallback != cb) {
+    if (conn->closeCallback->callback != cb) {
         virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
                         _("A different callback was requested"));
         goto error;
     }
 
-    conn->closeCallback = NULL;
-    conn->closeUnregisterCount++;
-    if (!conn->closeDispatch && conn->closeFreeCallback)
-        conn->closeFreeCallback(conn->closeOpaque);
-    conn->closeFreeCallback = NULL;
-    conn->closeOpaque = NULL;
-
-    virMutexUnlock(&conn->lock);
+    conn->closeCallback->callback = NULL;
+    if (conn->closeCallback->freeCallback)
+        conn->closeCallback->freeCallback(conn->closeCallback->opaque);
+    conn->closeCallback->freeCallback = NULL;
 
     virObjectUnref(conn);
+    virObjectUnlock(conn->closeCallback);
+    virMutexUnlock(&conn->lock);
 
     return 0;
 
 error:
+    virObjectUnlock(conn->closeCallback);
     virMutexUnlock(&conn->lock);
     virDispatchError(NULL);
     return -1;

src/remote/remote_driver.c

@@ -338,31 +338,30 @@ enum virDrvOpenRemoteFlags {
 };
 
 
-static void remoteClientCloseFunc(virNetClientPtr client ATTRIBUTE_UNUSED,
-                                  int reason,
-                                  void *opaque)
+static void
+remoteClientCloseFunc(virNetClientPtr client ATTRIBUTE_UNUSED,
+                      int reason,
+                      void *opaque)
 {
-    virConnectPtr conn = opaque;
+    virConnectCloseCallbackDataPtr cbdata = opaque;
 
-    virMutexLock(&conn->lock);
-    if (conn->closeCallback) {
-        virConnectCloseFunc closeCallback = conn->closeCallback;
-        void *closeOpaque = conn->closeOpaque;
-        virFreeCallback closeFreeCallback = conn->closeFreeCallback;
-        unsigned closeUnregisterCount = conn->closeUnregisterCount;
+    virObjectLock(cbdata);
 
-        VIR_DEBUG("Triggering connection close callback %p reason=%d",
-                  conn->closeCallback, reason);
-        conn->closeDispatch = true;
-        virMutexUnlock(&conn->lock);
-        closeCallback(conn, reason, closeOpaque);
-        virMutexLock(&conn->lock);
-        conn->closeDispatch = false;
-        if (conn->closeUnregisterCount != closeUnregisterCount &&
-            closeFreeCallback)
-            closeFreeCallback(closeOpaque);
+    if (cbdata->callback) {
+        VIR_DEBUG("Triggering connection close callback %p reason=%d, opaque=%p",
+                  cbdata->callback, reason, cbdata->opaque);
+        cbdata->callback(cbdata->conn, reason, cbdata->opaque);
+
+        if (cbdata->freeCallback)
+            cbdata->freeCallback(cbdata->opaque);
+        cbdata->callback = NULL;
+        cbdata->freeCallback = NULL;
     }
-    virMutexUnlock(&conn->lock);
+    virObjectUnlock(cbdata);
+
+    /* free the connection reference that comes along with the callback
+     * registration */
+    virObjectUnref(cbdata->conn);
 }
 
 /* helper macro to ease extraction of arguments from the URI */
@@ -765,9 +764,11 @@ doRemoteOpen(virConnectPtr conn,
             goto failed;
     }
 
+    virObjectRef(conn->closeCallback);
+
     virNetClientSetCloseCallback(priv->client,
                                  remoteClientCloseFunc,
-                                 conn, NULL);
+                                 conn->closeCallback, virObjectFreeCallback);
 
     if (!(priv->remoteProgram = virNetClientProgramNew(REMOTE_PROGRAM,
                                                        REMOTE_PROTOCOL_VERSION,
@@ -1031,10 +1032,11 @@ doRemoteClose(virConnectPtr conn, struct private_data *priv)
     virObjectUnref(priv->tls);
     priv->tls = NULL;
 #endif
+
     virNetClientSetCloseCallback(priv->client,
                                  NULL,
-                                 NULL,
-                                 NULL);
+                                 conn->closeCallback, virObjectFreeCallback);
+
     virNetClientClose(priv->client);
     virObjectUnref(priv->client);
     priv->client = NULL;