Fix use after free AV in EventPipe (#24924)

* Reenable tests turned off in #24772 * Disable event pipe tests for investigation * Fix logic for deletion * In case of error - we need to take the lock because disabling the event pipe session * Intentionally leaking the EventPipeSession to mitigate a known race condition for now * Fix for EventListener lock order and throwing * Fix purposeful leak to still close the session
dotnet · Jun 3, 2019 · 7ec87b0 · 7ec87b0
1 parent 877efe9
commit 7ec87b0
Show file tree

Hide file tree

Showing 7 changed files with 33 additions and 19 deletions.
diff --git a/src/vm/eventpipe.cpp b/src/vm/eventpipe.cpp
@@ -457,7 +457,7 @@ void EventPipe::DisableInternal(EventPipeSessionID id, EventPipeProviderCallback
 EventPipeSession *EventPipe::GetSession(EventPipeSessionID id)
 {
     LIMITED_METHOD_CONTRACT;
-    _ASSERTE(IsLockOwnedByCurrentThread());
+    CrstHolder _crst(GetLock());
 
     if (s_pSessions == nullptr)
         return nullptr;

diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp
@@ -616,7 +616,7 @@ EventPipeEventInstance* EventPipeBufferManager::GetNextEvent()
 {
     CONTRACTL
     {
-        NOTHROW;
+        THROWS;
         GC_NOTRIGGER;
         MODE_ANY;
         PRECONDITION(!EventPipe::IsLockOwnedByCurrentThread());

diff --git a/src/vm/eventpipeconfiguration.cpp b/src/vm/eventpipeconfiguration.cpp
@@ -335,7 +335,9 @@ void EventPipeConfiguration::DeleteSession(EventPipeSession *pSession)
     {
         // Reset the mask of active sessions.
         m_activeSessions &= ~pSession->GetId();
-        delete pSession;
+        pSession->Close();
+        // TODO: Re-enable this after fixing the underlying race condition
+        // delete pSession;
     }
 }
 

diff --git a/src/vm/eventpipesession.cpp b/src/vm/eventpipesession.cpp
@@ -62,6 +62,21 @@ EventPipeSession::EventPipeSession(
     QueryPerformanceCounter(&m_sessionStartTimeStamp);
 }
 
+void EventPipeSession::Close()
+{
+    CONTRACTL
+    {
+        NOTHROW;
+        GC_TRIGGERS;
+        MODE_PREEMPTIVE;
+    }
+    CONTRACTL_END;
+
+    // FIXME: **ONLY** closes the stream. This explicitly **LEAKS** the
+    // provider list and buffer manager.
+    delete m_pFile;
+}
+
 EventPipeSession::~EventPipeSession()
 {
     CONTRACTL
@@ -182,7 +197,9 @@ DWORD WINAPI EventPipeSession::ThreadProc(void *args)
             pEventPipeSession->SetThreadShutdownEvent();
 
             if (!fSuccess)
-                pEventPipeSession->Disable();
+            {
+                EventPipe::RunWithCallbackPostponed([pEventPipeSession](EventPipeProviderCallbackDataQueue *pEventPipeProviderCallbackDataQueue){pEventPipeSession->Disable();});
+            }
         }
         EX_CATCH
         {

diff --git a/src/vm/eventpipesession.h b/src/vm/eventpipesession.h
@@ -89,6 +89,7 @@ class EventPipeSession
         uint32_t numProviders,
         bool rundownEnabled = false);
     ~EventPipeSession();
+    void Close();
 
     EventPipeSessionID GetId() const
     {

diff --git a/src/vm/eventpipesessionprovider.cpp b/src/vm/eventpipesessionprovider.cpp
@@ -188,18 +188,12 @@ void EventPipeSessionProviderList::Clear()
 {
     if (m_pProviders != NULL)
     {
-        SListElem<EventPipeSessionProvider *> *pElem = m_pProviders->GetHead();
-        while (pElem != NULL)
+        while (!m_pProviders->IsEmpty())
         {
+            SListElem<EventPipeSessionProvider*> *pElem = m_pProviders->RemoveHead();
             EventPipeSessionProvider *pProvider = pElem->GetValue();
             delete pProvider;
-
-            SListElem<EventPipeSessionProvider *> *pCurElem = pElem;
-            pElem = m_pProviders->GetNext(pElem);
-            delete pCurElem;
-
-            // Remove deleted node.
-            m_pProviders->RemoveHead();
+            delete pElem;
         }
     }
 

diff --git a/tests/issues.targets b/tests/issues.targets
@@ -2,6 +2,12 @@
 <Project DefaultTargets = "GetListOfTestCmds" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
     <!-- All OS/Arch common excludes -->
     <ItemGroup Condition="'$(XunitTestBinBase)' != ''">
+        <ExcludeList Include="$(XunitTestBinBase)/tracing/eventsource/**/*">	
+            <Issue>24839</Issue>	
+        </ExcludeList>	
+        <ExcludeList Include="$(XunitTestBinBase)/tracing/tracevalidation/**/*">	
+            <Issue>24839</Issue>	
+        </ExcludeList>
         <ExcludeList Include="$(XunitTestBinBase)/baseservices/threading/generics/threadstart/GThread23/*">
             <Issue>19339</Issue>
         </ExcludeList>
@@ -518,12 +524,6 @@
         <ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/GitHub_19601/Github_19601/*">
             <Issue>Needs Triage</Issue>
         </ExcludeList>
-        <ExcludeList Include="$(XunitTestBinBase)/tracing/eventsource/**/*">
-            <Issue>Failing on alpine, being tracked by #24772</Issue>
-        </ExcludeList>
-        <ExcludeList Include="$(XunitTestBinBase)/tracing/tracevalidation/**/*">
-            <Issue>Failing on alpine, being tracked by #24772</Issue>
-        </ExcludeList>
     </ItemGroup>
 
     <!-- Unix arm64 specific -->