From 8ad126e695e5cef5da9d62ccfde7338317041e84 Mon Sep 17 00:00:00 2001 From: Peter Krempa Date: Fri, 29 Mar 2013 18:21:19 +0100 Subject: [PATCH] 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 | 55 +++++++++++++++++++++++++++++++------- src/datatypes.h | 22 +++++++++++---- src/libvirt.c | 29 +++++++++++--------- src/remote/remote_driver.c | 50 +++++++++++++++++----------------- 4 files changed, 105 insertions(+), 51 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index b04e10055e..b21daac2fb 100644 --- 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 a1dfc1ebcf..e5ecf18368 100644 --- 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 e9aff8aa48..cef9f1f1f4 100644 --- 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 885120e153..18c086b3c5 100644 --- 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;