From 1893a9c21d21a860094155b330efca9e598581d0 Mon Sep 17 00:00:00 2001 From: David Mason Date: Wed, 3 Jul 2019 17:15:47 -0700 Subject: [PATCH] Add check to prevent attaching a profiler when one is already present (#25520) * Don't load multiple profilers * add comment * Move check to LoadProfiler, and eliminate race condition between attach profiler and startup profiled --- src/inc/profilepriv.h | 1 + src/vm/profilerdiagnosticprotocolhelper.cpp | 2 +- src/vm/profilinghelper.cpp | 211 +++++++++++++++++----------- src/vm/profilinghelper.h | 20 +++ 4 files changed, 149 insertions(+), 85 deletions(-) diff --git a/src/inc/profilepriv.h b/src/inc/profilepriv.h index 91791f5..acec829 100644 --- a/src/inc/profilepriv.h +++ b/src/inc/profilepriv.h @@ -40,6 +40,7 @@ enum ProfilerStatus kProfStatusInitializingForStartupLoad = 2, // Prof ready for (or in) its Initialize callback kProfStatusInitializingForAttachLoad = 3, // Prof ready for (or in) its InitializeForAttach callback kProfStatusActive = 4, // Prof completed initialization and is actively running + kProfStatusPreInitialize = 5, // Prof is in LoadProfiler, but initialization has yet to occur }; class CurrentProfilerStatus diff --git a/src/vm/profilerdiagnosticprotocolhelper.cpp b/src/vm/profilerdiagnosticprotocolhelper.cpp index 3f96912..e269504 100644 --- a/src/vm/profilerdiagnosticprotocolhelper.cpp +++ b/src/vm/profilerdiagnosticprotocolhelper.cpp @@ -102,7 +102,7 @@ void ProfilerDiagnosticProtocolHelper::AttachProfiler(DiagnosticsIpc::IpcMessage hr = CORPROF_E_RUNTIME_UNINITIALIZED; goto ErrExit; } - + // Certain actions are only allowable during attach, and this flag is how we track it. ClrFlsSetThreadType(ThreadType_ProfAPI_Attach); diff --git a/src/vm/profilinghelper.cpp b/src/vm/profilinghelper.cpp index e6d5fb4..1ef25b4 100644 --- a/src/vm/profilinghelper.cpp +++ b/src/vm/profilinghelper.cpp @@ -232,8 +232,7 @@ void CurrentProfilerStatus::Set(ProfilerStatus newProfStatus) break; case kProfStatusNone: - _ASSERTE((newProfStatus == kProfStatusInitializingForStartupLoad) || - (newProfStatus == kProfStatusInitializingForAttachLoad)); + _ASSERTE(newProfStatus == kProfStatusPreInitialize); break; case kProfStatusDetaching: @@ -250,6 +249,12 @@ void CurrentProfilerStatus::Set(ProfilerStatus newProfStatus) _ASSERTE((newProfStatus == kProfStatusNone) || (newProfStatus == kProfStatusDetaching)); break; + + case kProfStatusPreInitialize: + _ASSERTE((newProfStatus == kProfStatusNone) || + (newProfStatus == kProfStatusInitializingForStartupLoad) || + (newProfStatus == kProfStatusInitializingForAttachLoad)); + break; } m_profStatus = newProfStatus; @@ -818,38 +823,13 @@ HRESULT ProfilingAPIUtility::PerformDeferredInit() return S_OK; } -// ---------------------------------------------------------------------------- -// ProfilingAPIUtility::LoadProfiler -// -// Description: -// Outermost common code for loading the profiler DLL. Both startup and attach code -// paths use this. -// -// Arguments: -// * loadType - Startup load or attach load? -// * pClsid - Profiler's CLSID -// * wszClsid - Profiler's CLSID (or progid) in string form, for event log messages -// * wszProfilerDLL - Profiler's DLL path -// * pvClientData - For attach loads, this is the client data the trigger wants to -// pass to the profiler DLL -// * cbClientData - For attach loads, size of client data in bytes -// * dwConcurrentGCWaitTimeoutInMs - Time out for wait operation on concurrent GC. Attach scenario only -// -// Return Value: -// HRESULT indicating success or failure of the load -// -// Notes: -// * On failure, this function or a callee will have logged an event -// - // static -HRESULT ProfilingAPIUtility::LoadProfiler( - LoadType loadType, - const CLSID * pClsid, - LPCWSTR wszClsid, +HRESULT ProfilingAPIUtility::DoPreInitialization( + EEToProfInterfaceImpl *pEEProf, + const CLSID *pClsid, + LPCWSTR wszClsid, LPCWSTR wszProfilerDLL, - LPVOID pvClientData, - UINT cbClientData, + LoadType loadType, DWORD dwConcurrentGCWaitTimeoutInMs) { CONTRACTL @@ -862,25 +842,14 @@ HRESULT ProfilingAPIUtility::LoadProfiler( CAN_TAKE_LOCK; MODE_ANY; + PRECONDITION(pEEProf != NULL); + PRECONDITION(pClsid != NULL); + PRECONDITION(wszClsid != NULL); + PRECONDITION(wszProfilerDLL != NULL); } CONTRACTL_END; - if (g_fEEShutDown) - { - return CORPROF_E_RUNTIME_UNINITIALIZED; - } - - enum ProfilerCompatibilityFlag - { - // Default: disable V2 profiler - kDisableV2Profiler = 0x0, - - // Enable V2 profilers - kEnableV2Profiler = 0x1, - - // Disable Profiling - kPreventLoad = 0x2, - }; + _ASSERTE(s_csStatus != NULL); ProfilerCompatibilityFlag profilerCompatibilityFlag = kDisableV2Profiler; NewArrayHolder wszProfilerCompatibilitySetting(NULL); @@ -914,33 +883,8 @@ HRESULT ProfilingAPIUtility::LoadProfiler( } } - HRESULT hr; - - hr = PerformDeferredInit(); - if (FAILED(hr)) - { - LOG(( - LF_CORPROF, - LL_ERROR, - "**PROF: ProfilingAPIUtility::PerformDeferredInit failed. hr=0x%x.\n", - hr)); - LogProfError(IDS_E_PROF_INTERNAL_INIT, wszClsid, hr); - return hr; - } - - // Valid loadType? - _ASSERTE((loadType == kStartupLoad) || (loadType == kAttachLoad)); - - // If a nonzero client data size is reported, there'd better be client data! - _ASSERTE((cbClientData == 0) || (pvClientData != NULL)); - - // Client data is currently only specified on attach - _ASSERTE((pvClientData == NULL) || (loadType == kAttachLoad)); - - // Don't be telling me to load a profiler if there already is one. - _ASSERTE(g_profControlBlock.curProfStatus.Get() == kProfStatusNone); + HRESULT hr = S_OK; - // Create the ProfToEE interface to provide to the profiling services NewHolder pProfEE(new (nothrow) ProfToEEInterfaceImpl()); if (pProfEE == NULL) { @@ -961,15 +905,6 @@ HRESULT ProfilingAPIUtility::LoadProfiler( // Provide the newly created and inited interface LOG((LF_CORPROF, LL_INFO10, "**PROF: Profiling code being provided with EE interface.\n")); - // Create a new EEToProf object - NewHolder pEEProf(new (nothrow) EEToProfInterfaceImpl()); - if (pEEProf == NULL) - { - LOG((LF_CORPROF, LL_ERROR, "**PROF: Unable to allocate EEToProfInterfaceImpl.\n")); - LogProfError(IDS_E_PROF_INTERNAL_INIT, wszClsid, E_OUTOFMEMORY); - return E_OUTOFMEMORY; - } - #ifdef FEATURE_PROFAPI_ATTACH_DETACH // We're about to load the profiler, so first make sure we successfully create the // DetachThread and abort the load of the profiler if we can't. This ensures we don't @@ -1035,7 +970,115 @@ HRESULT ProfilingAPIUtility::LoadProfiler( wszClsid); } - _ASSERTE(s_csStatus != NULL); + return hr; +} + +// ---------------------------------------------------------------------------- +// ProfilingAPIUtility::LoadProfiler +// +// Description: +// Outermost common code for loading the profiler DLL. Both startup and attach code +// paths use this. +// +// Arguments: +// * loadType - Startup load or attach load? +// * pClsid - Profiler's CLSID +// * wszClsid - Profiler's CLSID (or progid) in string form, for event log messages +// * wszProfilerDLL - Profiler's DLL path +// * pvClientData - For attach loads, this is the client data the trigger wants to +// pass to the profiler DLL +// * cbClientData - For attach loads, size of client data in bytes +// * dwConcurrentGCWaitTimeoutInMs - Time out for wait operation on concurrent GC. Attach scenario only +// +// Return Value: +// HRESULT indicating success or failure of the load +// +// Notes: +// * On failure, this function or a callee will have logged an event +// + +// static +HRESULT ProfilingAPIUtility::LoadProfiler( + LoadType loadType, + const CLSID * pClsid, + LPCWSTR wszClsid, + LPCWSTR wszProfilerDLL, + LPVOID pvClientData, + UINT cbClientData, + DWORD dwConcurrentGCWaitTimeoutInMs) +{ + CONTRACTL + { + THROWS; + GC_TRIGGERS; + + // This causes events to be logged, which loads resource strings, + // which takes locks. + CAN_TAKE_LOCK; + + MODE_ANY; + } + CONTRACTL_END; + + if (g_fEEShutDown) + { + return CORPROF_E_RUNTIME_UNINITIALIZED; + } + + // Valid loadType? + _ASSERTE((loadType == kStartupLoad) || (loadType == kAttachLoad)); + + // If a nonzero client data size is reported, there'd better be client data! + _ASSERTE((cbClientData == 0) || (pvClientData != NULL)); + + // Client data is currently only specified on attach + _ASSERTE((pvClientData == NULL) || (loadType == kAttachLoad)); + + HRESULT hr = PerformDeferredInit(); + if (FAILED(hr)) + { + LOG(( + LF_CORPROF, + LL_ERROR, + "**PROF: ProfilingAPIUtility::PerformDeferredInit failed. hr=0x%x.\n", + hr)); + LogProfError(IDS_E_PROF_INTERNAL_INIT, wszClsid, hr); + return hr; + } + + { + // To prevent race conditions we need to signal that a load is already happening. + // The diagnostics server is single threaded, but it can potentially be + // racing with the startup path, or theoretically in the future it could be + // racing with another attach request if the diagnostic server becomes + // multithreaded. + CRITSEC_Holder csh(s_csStatus); + + if (g_profControlBlock.curProfStatus.Get() != kProfStatusNone) + { + return CORPROF_E_PROFILER_ALREADY_ACTIVE; + } + + g_profControlBlock.curProfStatus.Set(kProfStatusPreInitialize); + } + + NewHolder pEEProf(new (nothrow) EEToProfInterfaceImpl()); + if (pEEProf == NULL) + { + LOG((LF_CORPROF, LL_ERROR, "**PROF: Unable to allocate EEToProfInterfaceImpl.\n")); + LogProfError(IDS_E_PROF_INTERNAL_INIT, wszClsid, E_OUTOFMEMORY); + return E_OUTOFMEMORY; + } + + // Create the ProfToEE interface to provide to the profiling services + hr = DoPreInitialization(pEEProf, pClsid, wszClsid, wszProfilerDLL, loadType, dwConcurrentGCWaitTimeoutInMs); + if (FAILED(hr)) + { + CRITSEC_Holder csh(s_csStatus); + g_profControlBlock.curProfStatus.Set(kProfStatusNone); + return hr; + } + { // All modification of the profiler's status and // g_profControlBlock.pProfInterface need to be serialized against each other, diff --git a/src/vm/profilinghelper.h b/src/vm/profilinghelper.h index 287c616..2f01740 100644 --- a/src/vm/profilinghelper.h +++ b/src/vm/profilinghelper.h @@ -45,6 +45,19 @@ class SidBuffer; // class ProfilingAPIUtility { +private: + enum ProfilerCompatibilityFlag + { + // Default: disable V2 profiler + kDisableV2Profiler = 0x0, + + // Enable V2 profilers + kEnableV2Profiler = 0x1, + + // Disable Profiling + kPreventLoad = 0x2, + }; + public: static HRESULT InitializeProfiling(); static HRESULT LoadProfilerForAttach( @@ -127,6 +140,13 @@ private: ProfilingAPIUtility() {} static HRESULT PerformDeferredInit(); + static HRESULT ProfilingAPIUtility::DoPreInitialization( + EEToProfInterfaceImpl *pEEProf, + const CLSID *pClsid, + LPCWSTR wszClsid, + LPCWSTR wszProfilerDLL, + LoadType loadType, + DWORD dwConcurrentGCWaitTimeoutInMs); static HRESULT LoadProfiler( LoadType loadType, const CLSID * pClsid, -- 2.7.4