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

diff --git a/src/inc/profilepriv.h b/src/inc/profilepriv.h
index 91791f5..acec829 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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<WCHAR> 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<ProfToEEInterfaceImpl> 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<EEToProfInterfaceImpl> 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<EEToProfInterfaceImpl> 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 (file)
--- 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,