From: VMware, Inc <> Date: Mon, 20 Dec 2010 22:15:43 +0000 (-0800) Subject: Fix Guest Hgfs server interface for multiple callers X-Git-Tag: 2010.12.19-339835~16 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=88cb790575c8cfef6cc5291b4c0b90ec21e3ceaa;p=thirdparty%2Fopen-vm-tools.git Fix Guest Hgfs server interface for multiple callers Code does not quite cope with multiple callers for the same process which leads to memory leaks. The guest channel uses two globals: a channel data object and a server data object both are shared for all registered connections that use the internal HGFS RPC. The channel data and server data objects are reference counted, initialized on the first reference and torndown on the final reference removal. At some point in the future if a client requires its own private callback which will call HGFS server, then the channel code will allocate a new channel which will in turn get its own private HGFS server session. Currently this is not supported. Signed-off-by: Marcelo Vanzin --- diff --git a/open-vm-tools/lib/hgfsServerManagerGuest/hgfsChannelGuest.c b/open-vm-tools/lib/hgfsServerManagerGuest/hgfsChannelGuest.c index f7af6dbc7..ff90162d5 100644 --- a/open-vm-tools/lib/hgfsServerManagerGuest/hgfsChannelGuest.c +++ b/open-vm-tools/lib/hgfsServerManagerGuest/hgfsChannelGuest.c @@ -17,7 +17,7 @@ *********************************************************/ /* - * hgfsChannel.c -- + * hgfsChannelGuest.c -- * * Channel abstraction for the HGFS server. * @@ -41,29 +41,221 @@ #include "hgfsServer.h" #include "hgfsServerManager.h" +/* + * HGFS server connection channel and state object usage. + * + * Currently, all plugins can share this same HGFS server channel and state. + * This allows us to use a common channel so it is only initialized + * once, by the first loaded plugin which requires an HGFS channel, and torn + * down when the final plugin that uses the HGFS server is unloaded. + * + * Currently, the plugins are loaded (and unloaded) in any particular order, + * and those operations are serialized. (For example the HGFS server plugin + * maybe the first plugin loaded that uses this channel, but is not the final + * plugin to be unloaded that uses the channel. This also may change in the + * future, so no dependencies can be made on order of loading and unloading + * of plugins.) + * Furthermore, multiple plugins use the HGFS channel and server and some plugins + * have multiple connections. Some plugins also create and teardown connections + * during general mutlithreaded operation of the tools processes. + * + * In order to support the above, we must track how many users of the shared + * connection there are. This allows us to tear down the shared connection + * when the final plugin that is using it is unloaded, and when no + * channels are in use the HGFS server state can be torn down. + */ -/* Transport channels context. Static. */ +/* + * The HGFS server state. + * + * This object is initiliazed once only and is shared across all + * connections, shared or private. + * Each new channel connection will reference the server and so the HGFS + * server is initialized when the first new channel is being created. Each + * new channel just increments the reference of server state object. + * When the final channel is torn down the final HGFS server reference is + * also removed and the HGFS server exit is called and this object is torn down. + */ +typedef struct HgfsChannelServerData { + HgfsServerSessionCallbacks *serverCBTable; /* HGFS server entry points. */ + Atomic_uint32 refCount; /* Server data reference count. */ +} HgfsChannelServerData; + +/* + * Transport channels context. + * + * Multiple callers share this same channel currently as only one + * transport channel is required. Therefore, the channel is referenced + * for each client that it is returned to (a usage count). + */ typedef struct HgfsChannelData { - const char *name; /* Channel name. */ - HgfsGuestChannelCBTable *ops; /* Channel operations. */ - uint32 state; /* Channel state (see flags below). */ - struct HgfsGuestConn *connection; /* Opaque server connection */ + const char *name; /* Channel name. */ + HgfsGuestChannelCBTable *ops; /* Channel operations. */ + uint32 state; /* Channel state (see flags below). */ + struct HgfsGuestConn *connection; /* Opaque server connection */ + HgfsChannelServerData *serverInfo; /* HGFS server entry points. */ + Atomic_uint32 refCount; /* Channel reference count. */ } HgfsChannelData; -#define HGFS_CHANNEL_STATE_INIT (1 << 0) -#define HGFS_CHANNEL_STATE_CBINIT (1 << 1) - -typedef uint32 HgfsChannelMgrState; /* Channel state (see flags below). */ - -#define HGFS_CHANNELMGR_STATE_SERVERINIT (1 << 0) -#define HGFS_CHANNELMGR_STATE_CHANINIT (1 << 1) +#define HGFS_CHANNEL_STATE_INIT (1 << 0) +#define HGFS_CHANNEL_STATE_CBINIT (1 << 1) /* Static channel registration - assumes only one for now. */ static HgfsChannelData gHgfsChannels[] = { - { "guest", &gGuestBackdoorOps, 0, NULL }, + { "guest", &gGuestBackdoorOps, 0, NULL, NULL, {0} }, }; -static HgfsChannelMgrState gHgfsChannelsMgrState = 0; +/* HGFS server info state. Referenced by each separate channel that uses it. */ +static HgfsChannelServerData gHgfsChannelServerInfo = { NULL, {0} }; + +static void HgfsChannelTeardownChannel(HgfsChannelData *channel); +static void HgfsChannelTeardownServer(HgfsChannelServerData *serverInfo); +static void HgfsChannelExitChannel(HgfsChannelData *channel); + + +/* + *---------------------------------------------------------------------------- + * + * HGFS SERVER DATA FUNCTIONS + * + *---------------------------------------------------------------------------- + */ + + +/* + *---------------------------------------------------------------------------- + * + * HgfsChannelGetServer -- + * + * Increment the server data reference count. + * + * Results: + * The value of the reference count before the increment. + * + * Side effects: + * None. + * + *---------------------------------------------------------------------------- + */ + +static uint32 +HgfsChannelGetServer(HgfsChannelServerData *serverInfo) // IN/OUT: ref count +{ + ASSERT(NULL != serverInfo); + return Atomic_FetchAndInc(&serverInfo->refCount); +} + + +/* + *---------------------------------------------------------------------------- + * + * HgfsChannelPutServer -- + * + * Decrement server data reference count. + * + * Teardown the server data object if removed the final reference. + * + * Results: + * None. + * + * Side effects: + * None. + * + *---------------------------------------------------------------------------- + */ + +static void +HgfsChannelPutServer(HgfsChannelServerData *serverInfo) // IN/OUT: ref count +{ + ASSERT(NULL != serverInfo); + if (Atomic_FetchAndDec(&serverInfo->refCount) == 1) { + HgfsChannelTeardownServer(serverInfo); + } +} + + +/* + *---------------------------------------------------------------------------- + * + * HgfsChannelInitServer -- + * + * Initialize HGFS server and save the state. + * + * Results: + * TRUE if success, FALSE otherwise. + * + * Side effects: + * None. + * + *---------------------------------------------------------------------------- + */ + +static Bool +HgfsChannelInitServer(HgfsChannelServerData *serverInfo) // IN/OUT: ref count +{ + Bool result; + + ASSERT(NULL == serverInfo->serverCBTable); + + Debug("%s: Initialize Hgfs server.\n", __FUNCTION__); + /* If we have a new connection initialize the server session. */ + result = HgfsServer_InitState(&serverInfo->serverCBTable, NULL); + if (!result) { + Debug("%s: Could not init Hgfs server.\n", __FUNCTION__); + } + return result; +} + + +/* + *---------------------------------------------------------------------------- + * + * HgfsChannelExitServer -- + * + * Reset the HGFS server and destroy the state. + * + * Results: + * None. + * + * Side effects: + * None. + * + *---------------------------------------------------------------------------- + */ + +static void +HgfsChannelExitServer(HgfsChannelServerData *serverInfo) // IN/OUT: ref count +{ + if (NULL != serverInfo->serverCBTable) { + Debug("%s: Teardown Hgfs server.\n", __FUNCTION__); + HgfsServer_ExitState(); + serverInfo->serverCBTable = NULL; + } +} + + +/* + *---------------------------------------------------------------------------- + * + * HgfsChannelTeardownServer -- + * + * Teardown the HGFS server state for all connections. + * + * Results: + * None. + * + * Side effects: + * None. + * + *---------------------------------------------------------------------------- + */ + +static void +HgfsChannelTeardownServer(HgfsChannelServerData *serverInfo) // IN/OUT: connection manager object +{ + HgfsChannelExitServer(serverInfo); +} + /* *---------------------------------------------------------------------------- @@ -73,12 +265,65 @@ static HgfsChannelMgrState gHgfsChannelsMgrState = 0; *---------------------------------------------------------------------------- */ + + /* + *---------------------------------------------------------------------------- + * + * HgfsChannelGetChannel -- + * + * Increment channel data reference count. + * + * Results: + * The value of the reference count before the increment. + * + * Side effects: + * None. + * + *---------------------------------------------------------------------------- + */ + +uint32 +HgfsChannelGetChannel(HgfsChannelData *channel) // IN/OUT: ref count +{ + ASSERT(NULL != channel); + return Atomic_FetchAndInc(&channel->refCount); +} + + +/* + *---------------------------------------------------------------------------- + * + * HgfsChannelPutChannel -- + * + * Decrement channel reference count. + * + * Teardown channel object if removed the final reference. + * + * Results: + * None. + * + * Side effects: + * None. + * + *---------------------------------------------------------------------------- + */ + +static void +HgfsChannelPutChannel(HgfsChannelData *channel) // IN/OUT: ref count +{ + ASSERT(NULL != channel); + if (Atomic_FetchAndDec(&channel->refCount) == 1) { + HgfsChannelTeardownChannel(channel); + } +} + + /* *----------------------------------------------------------------------------- * * HgfsChannelInitChannel -- * - * Initializes a channel. + * Initializes a channel by initializing the HGFS server state. * * Results: * TRUE if the channel initialized, FALSE otherwise. @@ -90,10 +335,39 @@ static HgfsChannelMgrState gHgfsChannelsMgrState = 0; */ static Bool -HgfsChannelInitChannel(HgfsChannelData *channel) // IN/OUT: channel object +HgfsChannelInitChannel(HgfsChannelData *channel, // IN/OUT: channel object + HgfsChannelServerData *serverInfo) // IN/OUT: server info { - channel->state = HGFS_CHANNEL_STATE_INIT; - return TRUE; + Bool result = TRUE; + uint32 serverInfoCount; + + channel->state = 0; + /* + * Reference the HGFS server as it will be used by the new channel. + * The HGFS server should only be initialized once, i.e. on the first + * caller instance, otherwise only reference the server info for + * the new channel. + */ + serverInfoCount = HgfsChannelGetServer(serverInfo); + /* Referenced the server, save it for dereferencing. */ + channel->serverInfo = serverInfo; + if (0 == serverInfoCount) { + /* The HGFS server has not been initialized, do it now. */ + result = HgfsChannelInitServer(channel->serverInfo); + if (!result) { + Debug("%s: Could not init Hgfs server.\n", __FUNCTION__); + goto exit; + } + } + + channel->state |= HGFS_CHANNEL_STATE_INIT; + +exit: + if (!result) { + HgfsChannelExitChannel(channel); + } + Debug("%s: Init channel return %d.\n", __FUNCTION__, result); + return result; } @@ -102,7 +376,7 @@ HgfsChannelInitChannel(HgfsChannelData *channel) // IN/OUT: channel object * * HgfsChannelExitChannel -- * - * Teardown the channel. + * Teardown the channel and teardown the HGFS server. * * Results: * None. @@ -116,9 +390,13 @@ HgfsChannelInitChannel(HgfsChannelData *channel) // IN/OUT: channel object static void HgfsChannelExitChannel(HgfsChannelData *channel) // IN/OUT: channel object { - if (channel->state & HGFS_CHANNEL_STATE_INIT) { - channel->state = 0; - } + if (NULL != channel->serverInfo) { + /* Remove the reference for the HGFS server info. */ + HgfsChannelPutServer(channel->serverInfo); + channel->serverInfo = NULL; + } + channel->state = 0; + Debug("%s: Exit channel returns.\n", __FUNCTION__); } @@ -139,14 +417,17 @@ HgfsChannelExitChannel(HgfsChannelData *channel) // IN/OUT: channel object */ static Bool -HgfsChannelActivateChannel(HgfsChannelData *channel, // IN/OUT: channel object - HgfsServerSessionCallbacks *serverCBTable, // IN: server callbacks - HgfsServerMgrData *mgrData) // IN: mgrData +HgfsChannelActivateChannel(HgfsChannelData *channel, // IN/OUT: channel object + void *rpc, // IN: Rpc channel + void *rpcCallback) // IN: Rpc callback { Bool success = FALSE; struct HgfsGuestConn *connData = NULL; - if (channel->ops->init(serverCBTable, mgrData->rpc, mgrData->rpcCallback, &connData)) { + if (channel->ops->init(channel->serverInfo->serverCBTable, + rpc, + rpcCallback, + &connData)) { channel->state |= HGFS_CHANNEL_STATE_CBINIT; channel->connection = connData; success = TRUE; @@ -172,13 +453,11 @@ HgfsChannelActivateChannel(HgfsChannelData *channel, // IN/OUT: */ static void -HgfsChannelDeactivateChannel(HgfsChannelData *channel, // IN/OUT: channel object - HgfsServerMgrData *mgrData) // IN: mgr handle +HgfsChannelDeactivateChannel(HgfsChannelData *channel) // IN/OUT: channel object { channel->ops->exit(channel->connection); channel->state &= ~HGFS_CHANNEL_STATE_CBINIT; channel->connection = NULL; - mgrData->connection = NULL; } @@ -201,8 +480,8 @@ HgfsChannelDeactivateChannel(HgfsChannelData *channel, // IN/OUT: channel obje static Bool HgfsChannelIsChannelActive(HgfsChannelData *channel) // IN/OUT: channel object { - return (Bool)((channel->state & HGFS_CHANNEL_STATE_INIT) && - (channel->state & HGFS_CHANNEL_STATE_CBINIT)); + return (0 != (channel->state & HGFS_CHANNEL_STATE_INIT) && + 0 != (channel->state & HGFS_CHANNEL_STATE_CBINIT)); } @@ -237,6 +516,32 @@ HgfsChannelReceive(HgfsChannelData *channel, // IN/OUT: channel object } +/* + *---------------------------------------------------------------------------- + * + * HgfsChannelTeardownChannel -- + * + * Teardown the channel for HGFS. + * + * Results: + * None. + * + * Side effects: + * None. + * + *---------------------------------------------------------------------------- + */ + +static void +HgfsChannelTeardownChannel(HgfsChannelData *channel) // IN/OUT: connection manager object +{ + if (HgfsChannelIsChannelActive(channel)) { + HgfsChannelDeactivateChannel(channel); + } + HgfsChannelExitChannel(channel); +} + + /* *---------------------------------------------------------------------------- * @@ -269,35 +574,55 @@ HgfsChannelReceive(HgfsChannelData *channel, // IN/OUT: channel object Bool HgfsChannelGuest_Init(HgfsServerMgrData *mgrData) // IN/OUT: connection manager object { - HgfsServerSessionCallbacks *serverCBTable = NULL; // References a read-only global Bool success = FALSE; + HgfsChannelData *channel = &gHgfsChannels[0]; // Shared channel (internal RPC) + uint32 channelRefCount; ASSERT(NULL != mgrData); ASSERT(NULL == mgrData->connection); + /* Currently, the RPC override is not implemented. */ + ASSERT(NULL == mgrData->rpc); + ASSERT(NULL == mgrData->rpcCallback); + ASSERT(NULL != mgrData->appName); - gHgfsChannelsMgrState = 0; + Debug("%s: app %s rpc = %p rpc cb = %p.\n", __FUNCTION__, + mgrData->appName, mgrData->rpc, mgrData->rpcCallback); - /* If we have a new connection initialize the server session. */ - if (!HgfsServer_InitState(&serverCBTable, NULL)) { - Debug("%s: Could not init Hgfs server.\n", __FUNCTION__); - goto exit; - } - gHgfsChannelsMgrState |= HGFS_CHANNELMGR_STATE_SERVERINIT; + if (NULL != mgrData->rpc || NULL != mgrData->rpcCallback) { + /* + * XXX - Would malloc a new channel here and activate + * with the required RPC. + */ - /* Initialize channels objects. */ - if (!HgfsChannelInitChannel(&gHgfsChannels[0])) { - Debug("%s: Could not init channel.\n", __FUNCTION__); + Debug("%s: Guest channel RPC override not supported.\n", __FUNCTION__); goto exit; } - gHgfsChannelsMgrState |= HGFS_CHANNELMGR_STATE_CHANINIT; - /* Call the channels initializers. */ - if (!HgfsChannelActivateChannel(&gHgfsChannels[0], serverCBTable, mgrData)) { - Debug("%s: Could not activate channel.\n", __FUNCTION__); - goto exit; + /* + * Reference the channel. Initialize only for the first + * caller instance, otherwise only reference the channel for + * return to the caller. + */ + channelRefCount = HgfsChannelGetChannel(channel); + /* We have referenced the channel, save it for later dereference. */ + mgrData->connection = channel; + if (0 == channelRefCount) { + + /* Initialize channels objects. */ + if (!HgfsChannelInitChannel(channel, &gHgfsChannelServerInfo)) { + Debug("%s: Could not init channel.\n", __FUNCTION__); + goto exit; + } + + /* Call the channels initializers. */ + if (!HgfsChannelActivateChannel(channel, + mgrData->rpc, + mgrData->rpcCallback)) { + Debug("%s: Could not activate channel.\n", __FUNCTION__); + goto exit; + } } - mgrData->connection = &gHgfsChannels[0]; success = TRUE; exit: @@ -313,15 +638,14 @@ exit: * * HgfsChannelGuest_Exit -- * - * Close the channel for HGFS. - * - * Close open sessions and close the channels. + * Dereference the channel which for the final reference will + * close the channel for HGFS. * * Results: * None. * * Side effects: - * Closes the worker group and all the channels. + * None. * *---------------------------------------------------------------------------- */ @@ -331,29 +655,17 @@ HgfsChannelGuest_Exit(HgfsServerMgrData *mgrData) // IN/OUT: connection manager { HgfsChannelData *channel; - ASSERT(mgrData != NULL); + ASSERT(NULL != mgrData); + ASSERT(NULL != mgrData->appName); channel = mgrData->connection; - Debug("%s: Channel Exit.\n", __FUNCTION__); + Debug("%s: app %s rpc = %p rpc cb = %p chn = %p.\n", __FUNCTION__, + mgrData->appName, mgrData->rpc, mgrData->rpcCallback, channel); if (NULL != channel) { - if (HgfsChannelIsChannelActive(channel)) { - HgfsChannelDeactivateChannel(channel, mgrData); - mgrData->connection = NULL; - } - } else { - channel = &gHgfsChannels[0]; - } - - if (gHgfsChannelsMgrState & HGFS_CHANNELMGR_STATE_CHANINIT) { - HgfsChannelExitChannel(channel); - gHgfsChannelsMgrState &= ~HGFS_CHANNELMGR_STATE_CHANINIT; - } - - if (gHgfsChannelsMgrState & HGFS_CHANNELMGR_STATE_SERVERINIT) { - HgfsServer_ExitState(); - gHgfsChannelsMgrState &= ~HGFS_CHANNELMGR_STATE_SERVERINIT; + HgfsChannelPutChannel(channel); + mgrData->connection = NULL; } } @@ -385,12 +697,13 @@ HgfsChannelGuest_Receive(HgfsServerMgrData *mgrData, // IN/OUT : conn manager HgfsChannelData *channel = NULL; Bool result = FALSE; - ASSERT(mgrData != NULL); - ASSERT(mgrData->connection != NULL); + ASSERT(NULL != mgrData); + ASSERT(NULL != mgrData->connection); + ASSERT(NULL != mgrData->appName); channel = mgrData->connection; - Debug("%s: Channel receive request.\n", __FUNCTION__); + Debug("%s: %s Channel receive request.\n", __FUNCTION__, mgrData->appName); if (HgfsChannelIsChannelActive(channel)) { result = HgfsChannelReceive(channel, @@ -400,5 +713,7 @@ HgfsChannelGuest_Receive(HgfsServerMgrData *mgrData, // IN/OUT : conn manager packetOutSize); } + Debug("%s: Channel receive returns %#x.\n", __FUNCTION__, result); + return result; }