From: VMware, Inc <> Date: Tue, 29 Mar 2011 20:10:58 +0000 (-0700) Subject: Fix a crash when calling RpcIn_stop. X-Git-Tag: 2011.03.28-387002~42 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=be58b1f375d38788b71d5665d0c122f031b0ef7c;p=thirdparty%2Fopen-vm-tools.git Fix a crash when calling RpcIn_stop. If an RpcInLoop callback calls RpcIn_stop, then the state of the RpcIn struct becomes corrupt (we have a timer but no channel), leading to a crash the next time the RpcInLoop runs. One way this can happen on Windows VMs is if we pump the main thread's message queue from inside an RPC handler, since we will call RpcIn_stop in response to a WM_WTSSESSION_CHANGE message. To fix this, we set an inLoop var in the RpcIn struct at the start of the RpcInLoop function. RpcIn_stop checks the var, and if it is true, it sets a shouldStop to TRUE instead of stopping the channel itself. When we eventually exit the RpcInLoop function, we check shouldStop and stop the RPC channel if it is true. This obviously only works if RpcIn_stop and RpcInLoop are called on the same thread, but RpcIn is not thread-safe to being with, so there didn't seem to be any point in trying to fix this problem for that case. Signed-off-by: Marcelo Vanzin --- diff --git a/open-vm-tools/lib/include/rpcin.h b/open-vm-tools/lib/include/rpcin.h index 0d749c294..0fac5d706 100644 --- a/open-vm-tools/lib/include/rpcin.h +++ b/open-vm-tools/lib/include/rpcin.h @@ -83,8 +83,7 @@ unsigned int RpcIn_SetRetVals(char const **result, size_t *resultLen, #endif /* } */ void RpcIn_Destruct(RpcIn *in); -Bool RpcIn_stop(RpcIn *in); - +void RpcIn_stop(RpcIn *in); #ifdef __cplusplus } // extern "C" diff --git a/open-vm-tools/lib/rpcIn/rpcin.c b/open-vm-tools/lib/rpcIn/rpcin.c index 71b356e8a..9a62a310c 100644 --- a/open-vm-tools/lib/rpcIn/rpcin.c +++ b/open-vm-tools/lib/rpcIn/rpcin.c @@ -112,16 +112,28 @@ struct RpcIn { /* The size of the result */ size_t last_resultLen; + + /* + * It's possible for a callback dispatched by RpcInLoop to call RpcIn_stop. + * When this happens, we corrupt the state of the RpcIn struct, resulting in + * a crash the next time RpcInLoop is called. To prevent corruption of the + * RpcIn struct, we check inLoop when RpcIn_stop is called, and if it is + * true, we set shouldStop to TRUE instead of actually stopping the + * channel. When RpcInLoop exits, it will stop the channel if shouldStop is + * TRUE. + */ + Bool inLoop; // RpcInLoop is running. + Bool shouldStop; // Stop the channel the next time RpcInLoop exits. }; -/* +/* * The following functions are only needed in the non-glib version of the * library. The glib version of the library only deals with the transport * aspects of the code - RPC dispatching and other RPC-layer concerns are * handled by the rpcChannel abstraction library, or by the application. */ - + #if !defined(VMTOOLS_USE_GLIB) /* @@ -447,29 +459,23 @@ RpcInSend(RpcIn *in) // IN /* *----------------------------------------------------------------------------- * - * RpcIn_stop -- + * RpcInStop -- * - * Stop the background loop that receives RPC from VMware + * Stop the RPC channel. * * Results: - * TRUE on success - * FALSE on failure + * None * - * Side-effects: - * Try to send the last result and to close the channel + * Side effects: + * Sends the last result back to the host. * *----------------------------------------------------------------------------- */ -Bool -RpcIn_stop(RpcIn *in) // IN +static void +RpcInStop(RpcIn *in) // IN { - Bool status; - ASSERT(in); - - status = TRUE; - if (in->nextEvent) { /* The loop is started. Stop it */ #if defined(VMTOOLS_USE_GLIB) @@ -482,26 +488,46 @@ RpcIn_stop(RpcIn *in) // IN if (in->channel) { /* The channel is open */ - if (in->mustSend) { /* There is a final result to send back. Try to send it */ - if (RpcInSend(in) == FALSE) { - status = FALSE; - } - + RpcInSend(in); ASSERT(in->mustSend == FALSE); } /* Try to close the channel */ if (Message_Close(in->channel) == FALSE) { Debug("RpcIn: couldn't close channel\n"); - status = FALSE; } in->channel = NULL; } +} - return status; + +/* + *----------------------------------------------------------------------------- + * + * RpcIn_stop -- + * + * Stop the RPC channel. + * + * Results: + * None + * + * Side-effects: + * Sends the last result to the host, if one exists. + * + *----------------------------------------------------------------------------- + */ + +void +RpcIn_stop(RpcIn *in) // IN +{ + if (in->inLoop) { + in->shouldStop = TRUE; + } else { + RpcInStop(in); + } } @@ -510,17 +536,16 @@ RpcIn_stop(RpcIn *in) // IN * * RpcInLoop -- * - * The background loop that receives RPC from VMware + * Receives an RPC from the host. * - * Result - * Always FALSE for the glib implementation (to force unregistration of the - * timer; this function will re-schedule the callback). - * TRUE on success - * FALSE on failure (never happens in this implementation) + * Result: + * For the Event Manager implementation, always TRUE. * - * Side-effects - * May call the error routine in which case the loop is - * stopped. + * For the glib implementation, returns FALSE if the timer was rescheduled + * so that g_main_loop will unregister the old timer, or TRUE otherwise. + * + * Side-effects: + * Stops the RPC channel on error. * *----------------------------------------------------------------------------- */ @@ -542,12 +567,16 @@ RpcInLoop(void *clientData) // IN in = (RpcIn *)clientData; ASSERT(in); - /* The event has fired: it is no longer valid */ + in->inLoop = TRUE; + + /* The event has fired: it is no longer valid. */ ASSERT(in->nextEvent); current = in->delay; - /* This is very important: this is the only way to signal the existence - of this guest application to VMware */ + /* + * This is very important: this is the only way to signal the existence of + * this guest application to VMware. + */ ASSERT(in->channel); ASSERT(in->mustSend); if (RpcInSend(in) == FALSE) { @@ -623,7 +652,7 @@ RpcInLoop(void *clientData) // IN memcpy(in->last_result, statusStr, statusLen); memcpy(in->last_result + statusLen, result, resultLen); in->last_resultLen = statusLen + resultLen; - + if (freeResult) { free(result); } @@ -664,21 +693,31 @@ RpcInLoop(void *clientData) // IN ASSERT(in->mustSend == FALSE); in->mustSend = TRUE; + if (!in->shouldStop) { #if defined(VMTOOLS_USE_GLIB) - resched = (in->delay != current); - if (resched) { - g_source_unref(in->nextEvent); - RPCIN_SCHED_EVENT(in, VMTools_CreateTimer(in->delay * 10)); - } + resched = (in->delay != current); + if (resched) { + g_source_unref(in->nextEvent); + RPCIN_SCHED_EVENT(in, VMTools_CreateTimer(in->delay * 10)); + } #else - resched = TRUE; /* Avoid unused warning. */ - in->nextEvent = EventManager_Add(gTimerEventQueue, in->delay, RpcInLoop, in); + resched = TRUE; /* Avoid unused warning. */ + in->nextEvent = EventManager_Add(gTimerEventQueue, in->delay, RpcInLoop, in); #endif - if (in->nextEvent == NULL) { - errmsg = "RpcIn: Unable to run the loop"; - goto error; + if (in->nextEvent == NULL) { + errmsg = "RpcIn: Unable to run the loop"; + goto error; + } } +exit: + if (in->shouldStop) { + RpcInStop(in); + in->shouldStop = FALSE; + } + + in->inLoop = FALSE; + #if defined(VMTOOLS_USE_GLIB) return !resched; #else @@ -686,23 +725,10 @@ RpcInLoop(void *clientData) // IN #endif error: - /* The event has fired and will not be rescheduled: it is no longer valid */ - if (in->nextEvent) { -#if defined(VMTOOLS_USE_GLIB) - g_source_unref(in->nextEvent); -#endif - in->nextEvent = NULL; - } - RpcIn_stop(in); - /* Call the error routine */ (*in->errorFunc)(in->errorData, errmsg); - -#if defined(VMTOOLS_USE_GLIB) - return FALSE; -#else - return TRUE; -#endif + in->shouldStop = TRUE; + goto exit; } @@ -782,8 +808,7 @@ RpcIn_start(RpcIn *in, // IN return TRUE; error: - RpcIn_stop(in); - + RpcInStop(in); return FALSE; } diff --git a/open-vm-tools/toolbox/toolbox-gtk.c b/open-vm-tools/toolbox/toolbox-gtk.c index e9afa903e..6f5716ee8 100644 --- a/open-vm-tools/toolbox/toolbox-gtk.c +++ b/open-vm-tools/toolbox/toolbox-gtk.c @@ -162,10 +162,7 @@ static void ToolsMainCleanupRpc(void) { if (gRpcInCtlPanel) { - if (!RpcIn_stop(gRpcInCtlPanel)) { - Debug("Failed to stop RpcIn loop\n"); - } - + RpcIn_stop(gRpcInCtlPanel); RpcIn_Destruct(gRpcInCtlPanel); gRpcInCtlPanel = NULL; }