From 9562c551f391635ce81869aabc84f894c2846be8 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 15 Apr 2019 19:21:13 -0700 Subject: [release/3.0] Opt COM methods out of the new Windows instance-method handling (#24012) * Opt COM methods out of the new Windows instance-method handling. * Remove calls to run the MarshalStructReturn tests since we're rolling back the calling convention support change. --- src/vm/dllimport.cpp | 5 ++++- src/vm/ilmarshalers.h | 4 +--- 2 files changed, 5 insertions(+), 4 deletions(-) (limited to 'src/vm') diff --git a/src/vm/dllimport.cpp b/src/vm/dllimport.cpp index 924a50fe9d..93d0f79ca0 100644 --- a/src/vm/dllimport.cpp +++ b/src/vm/dllimport.cpp @@ -3956,7 +3956,10 @@ static void CreateNDirectStubWorker(StubState* pss, BOOL fMarshalReturnValueFirst = FALSE; BOOL fReverseWithReturnBufferArg = FALSE; - bool isInstanceMethod = fStubNeedsCOM || fThisCall; + // Only consider ThisCall methods to be instance methods. + // Techinically COM methods are also instance methods, but we don't want to change the behavior of the built-in + // COM abi work because there are many users that rely on the current behavior (for example WPF). + bool isInstanceMethod = fThisCall; // We can only change fMarshalReturnValueFirst to true when we are NOT doing HRESULT-swapping! // When we are HRESULT-swapping, the managed return type is actually the type of the last parameter and not the return type. diff --git a/src/vm/ilmarshalers.h b/src/vm/ilmarshalers.h index 866b3ec63c..750ac3de20 100644 --- a/src/vm/ilmarshalers.h +++ b/src/vm/ilmarshalers.h @@ -583,9 +583,7 @@ public: bool byrefNativeReturn = false; CorElementType typ = ELEMENT_TYPE_VOID; UINT32 nativeSize = 0; - bool nativeMethodIsMemberFunction = (m_pslNDirect->TargetHasThis() && IsCLRToNative(m_dwMarshalFlags)) - || (m_pslNDirect->HasThis() && !IsCLRToNative(m_dwMarshalFlags)) - || ((CorInfoCallConv)m_pslNDirect->GetStubTargetCallingConv() == CORINFO_CALLCONV_THISCALL); + bool nativeMethodIsMemberFunction = (CorInfoCallConv)m_pslNDirect->GetStubTargetCallingConv() == CORINFO_CALLCONV_THISCALL; // we need to convert value type return types to primitives as // JIT does not inline P/Invoke calls that return structures -- cgit v1.2.3 From 7ec87b0097fdd4400a8632a2eae56612914579ef Mon Sep 17 00:00:00 2001 From: Juan Hoyos Date: Mon, 3 Jun 2019 15:47:19 -0700 Subject: 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 --- src/vm/eventpipe.cpp | 2 +- src/vm/eventpipebuffermanager.cpp | 2 +- src/vm/eventpipeconfiguration.cpp | 4 +++- src/vm/eventpipesession.cpp | 19 ++++++++++++++++++- src/vm/eventpipesession.h | 1 + src/vm/eventpipesessionprovider.cpp | 12 +++--------- 6 files changed, 27 insertions(+), 13 deletions(-) (limited to 'src/vm') 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 *pElem = m_pProviders->GetHead(); - while (pElem != NULL) + while (!m_pProviders->IsEmpty()) { + SListElem *pElem = m_pProviders->RemoveHead(); EventPipeSessionProvider *pProvider = pElem->GetValue(); delete pProvider; - - SListElem *pCurElem = pElem; - pElem = m_pProviders->GetNext(pElem); - delete pCurElem; - - // Remove deleted node. - m_pProviders->RemoveHead(); + delete pElem; } } -- cgit v1.2.3