diff options
author | Juan Hoyos <juan.hoyos@microsoft.com> | 2019-06-03 15:47:19 -0700 |
---|---|---|
committer | John Salem <josalem@microsoft.com> | 2019-06-03 15:47:18 -0700 |
commit | 7ec87b0097fdd4400a8632a2eae56612914579ef (patch) | |
tree | be83ff50e9c5f344dd9a3f9d2c25fbd7827765f2 | |
parent | 877efe9aea4565f366e8489f90558b35f24737b1 (diff) | |
download | coreclr-7ec87b0097fdd4400a8632a2eae56612914579ef.tar.gz coreclr-7ec87b0097fdd4400a8632a2eae56612914579ef.tar.bz2 coreclr-7ec87b0097fdd4400a8632a2eae56612914579ef.zip |
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
-rw-r--r-- | src/vm/eventpipe.cpp | 2 | ||||
-rw-r--r-- | src/vm/eventpipebuffermanager.cpp | 2 | ||||
-rw-r--r-- | src/vm/eventpipeconfiguration.cpp | 4 | ||||
-rw-r--r-- | src/vm/eventpipesession.cpp | 19 | ||||
-rw-r--r-- | src/vm/eventpipesession.h | 1 | ||||
-rw-r--r-- | src/vm/eventpipesessionprovider.cpp | 12 | ||||
-rw-r--r-- | tests/issues.targets | 12 |
7 files changed, 33 insertions, 19 deletions
diff --git a/src/vm/eventpipe.cpp b/src/vm/eventpipe.cpp index dea3b1e493..72a6c13943 100644 --- 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 index b7351613d5..c3540fc14d 100644 --- 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 index bee0a08ae2..cd4824e591 100644 --- 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 index af1a5af212..e06a29a164 100644 --- 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 index bea5f9b2c3..528f1dd8d7 100644 --- a/src/vm/eventpipesession.h +++ b/src/vm/eventpipesession.h @@ -89,6 +89,7 @@ public: uint32_t numProviders, bool rundownEnabled = false); ~EventPipeSession(); + void Close(); EventPipeSessionID GetId() const { diff --git a/src/vm/eventpipesessionprovider.cpp b/src/vm/eventpipesessionprovider.cpp index 6550d9c49c..6a24ef7d95 100644 --- 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 index d926ccd1dd..5a39d9025c 100644 --- 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 --> |