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.

diff --git a/src/datatypes.c b/src/datatypes.c
index b04e10055e59bf95cda009e7139b6d69350f1b34..b21daac2fb5911943055fd8171aac0efa610e77e 100644 (file)
--- a/src/datatypes.c
+++ b/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

diff --git a/src/datatypes.h b/src/datatypes.h
index a1dfc1ebcfc64e509e370e2b1d393b6d4e58a28e..e5ecf183688d7978c4438fcf0a68a710770d5a7c 100644 (file)
--- a/src/datatypes.h
+++ b/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;
 };
 
 /**

diff --git a/src/libvirt.c b/src/libvirt.c
index e9aff8aa48a2b48fe879c7a47384ec69a5306a87..cef9f1f1f4217ac0762c3cb7d35d871c3fde0f13 100644 (file)
--- a/src/libvirt.c
+++ b/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;

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 885120e1533478fe92936dcaf328878bb658aeed..18c086b3c5cfc264e210170e6f16fa5e5094d512 100644 (file)
--- a/src/remote/remote_driver.c
+++ b/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;