From bcc622d46322dcd22b85a1a219faa170364a6626 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 3 Aug 2022 14:04:36 +0200 Subject: [PATCH 1/5] Fix ThreadAbort on Unix and reenable the related tests When the HandleSuspensionForInterruptedThread invokes Thread::HandleThreadAbort and the thread abort exception is thrown from there, runtime fails with unhandled C++ exception because there was a missing guard that catches the PAL_SEHException and invokes DispatchManagedException to propagate it further through managed code. This change fixes that and also addresses an issue with activation injection in Thread::UserAbort in the case of infinite timeout (it would not inject the activation in that case). --- .../Runtime/ControlledExecution.CoreCLR.cs | 5 - src/coreclr/vm/threads.h | 3 +- src/coreclr/vm/threadsuspend.cpp | 337 ++++++++++-------- .../Runtime/ControlledExecutionTests.cs | 10 +- 4 files changed, 187 insertions(+), 168 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs index 3b419f87f8b521..b6c437b351b7b1 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs @@ -42,11 +42,6 @@ public static partial class ControlledExecution [Obsolete(Obsoletions.ControlledExecutionRunMessage, DiagnosticId = Obsoletions.ControlledExecutionRunDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] public static void Run(Action action, CancellationToken cancellationToken) { - if (!OperatingSystem.IsWindows()) - { - throw new PlatformNotSupportedException(); - } - ArgumentNullException.ThrowIfNull(action); // ControlledExecution.Run does not support nested invocations. If there's one already in flight diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 59588b5dc6af9b..e8888207bd5ee4 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -3837,7 +3837,7 @@ class Thread #endif } - void UnmarkRedirectContextInUse(PTR_CONTEXT pCtx) + bool UnmarkRedirectContextInUse(PTR_CONTEXT pCtx) { LIMITED_METHOD_CONTRACT; #ifdef _DEBUG @@ -3848,6 +3848,7 @@ class Thread m_RedirectContextInUse = false; } #endif + return (pCtx == m_pSavedRedirectContext); } #endif //DACCESS_COMPILE diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 558d1775976acf..f3b85bac4e7a47 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -1167,6 +1167,15 @@ void Thread::SetAbortEndTime(ULONGLONG endTime, BOOL fRudeAbort) } +bool UseActivationInjection() +{ +#ifdef TARGET_UNIX + return true; +#else + return Thread::UseSpecialUserModeApc(); +#endif +} + #ifdef _PREFAST_ #pragma warning(push) #pragma warning(disable:21000) // Suppress PREFast warning about overly large function @@ -1227,176 +1236,188 @@ Thread::UserAbort(EEPolicy::ThreadAbortTypes abortType, DWORD timeout) _ASSERTE(this != pCurThread); // Aborting another thread. - if (!UseSpecialUserModeApc()) - { #ifdef _DEBUG - DWORD elapsed_time = 0; + DWORD elapsed_time = 0; #endif - // We do not want this thread to be alerted. - ThreadPreventAsyncHolder preventAsync(pCurThread != NULL); + // We do not want this thread to be alerted. + ThreadPreventAsyncHolder preventAsync(pCurThread != NULL); #ifdef _DEBUG - // If UserAbort times out, put up msgbox once. - BOOL fAlreadyAssert = FALSE; + // If UserAbort times out, put up msgbox once. + BOOL fAlreadyAssert = FALSE; #endif #if !defined(DISABLE_THREADSUSPEND) - DWORD dwSwitchCount = 0; + DWORD dwSwitchCount = 0; #endif // !defined(DISABLE_THREADSUSPEND) - while (true) + while (true) + { + // Lock the thread store + LOG((LF_SYNC, INFO3, "UserAbort obtain lock\n")); + + ULONGLONG abortEndTime = GetAbortEndTime(); + if (abortEndTime != MAXULONGLONG) { - // Lock the thread store - LOG((LF_SYNC, INFO3, "UserAbort obtain lock\n")); + ULONGLONG now_time = CLRGetTickCount64(); - ULONGLONG abortEndTime = GetAbortEndTime(); - if (abortEndTime != MAXULONGLONG) + if (now_time >= abortEndTime) { - ULONGLONG now_time = CLRGetTickCount64(); - - if (now_time >= abortEndTime) - { - // timeout, but no action on timeout. - // Debugger can call this function to abort func-eval with a timeout - return HRESULT_FROM_WIN32(ERROR_TIMEOUT); - } + // timeout, but no action on timeout. + // Debugger can call this function to abort func-eval with a timeout + return HRESULT_FROM_WIN32(ERROR_TIMEOUT); } + } - // Thread abort needs to walk stack to decide if thread abort can proceed. - // It is unsafe to crawl a stack of thread if the thread is OS-suspended which we do during - // thread abort. For example, Thread T1 aborts thread T2. T2 is suspended by T1. Inside SQL - // this means that no thread sharing the same scheduler with T2 can run. If T1 needs a lock which - // is owned by one thread on the scheduler, T1 will wait forever. - // Our solution is to move T2 to a safe point, resume it, and then do stack crawl. + // Thread abort needs to walk stack to decide if thread abort can proceed. + // It is unsafe to crawl a stack of thread if the thread is OS-suspended which we do during + // thread abort. For example, Thread T1 aborts thread T2. T2 is suspended by T1. Inside SQL + // this means that no thread sharing the same scheduler with T2 can run. If T1 needs a lock which + // is owned by one thread on the scheduler, T1 will wait forever. + // Our solution is to move T2 to a safe point, resume it, and then do stack crawl. - // We need to make sure that ThreadStoreLock is released after CheckForAbort. This makes sure - // that ThreadAbort does not race against GC. - class CheckForAbort + // We need to make sure that ThreadStoreLock is released after CheckForAbort. This makes sure + // that ThreadAbort does not race against GC. + class CheckForAbort + { + private: + Thread *m_pThread; + BOOL m_fHoldingThreadStoreLock; + BOOL m_NeedRelease; + public: + CheckForAbort(Thread *pThread, BOOL fHoldingThreadStoreLock) + : m_pThread(pThread), + m_fHoldingThreadStoreLock(fHoldingThreadStoreLock), + m_NeedRelease(FALSE) { - private: - Thread *m_pThread; - BOOL m_fHoldingThreadStoreLock; - BOOL m_NeedRelease; - public: - CheckForAbort(Thread *pThread, BOOL fHoldingThreadStoreLock) - : m_pThread(pThread), - m_fHoldingThreadStoreLock(fHoldingThreadStoreLock), - m_NeedRelease(TRUE) - { - if (!fHoldingThreadStoreLock) - { - ThreadSuspend::LockThreadStore(ThreadSuspend::SUSPEND_OTHER); - } - ThreadStore::ResetStackCrawlEvent(); - - // The thread being aborted may clear the TS_AbortRequested bit and the matching increment - // of g_TrapReturningThreads behind our back. Increment g_TrapReturningThreads here - // to ensure that we stop for the stack crawl even if the TS_AbortRequested bit is cleared. - ThreadStore::TrapReturningThreads(TRUE); - } - void NeedStackCrawl() - { - m_pThread->SetThreadState(Thread::TS_StackCrawlNeeded); - } - ~CheckForAbort() + } + void Activate() + { + m_NeedRelease = TRUE; + if (!m_fHoldingThreadStoreLock) { - Release(); + ThreadSuspend::LockThreadStore(ThreadSuspend::SUSPEND_OTHER); } - void Release() + ThreadStore::ResetStackCrawlEvent(); + + // The thread being aborted may clear the TS_AbortRequested bit and the matching increment + // of g_TrapReturningThreads behind our back. Increment g_TrapReturningThreads here + // to ensure that we stop for the stack crawl even if the TS_AbortRequested bit is cleared. + ThreadStore::TrapReturningThreads(TRUE); + } + void NeedStackCrawl() + { + m_pThread->SetThreadState(Thread::TS_StackCrawlNeeded); + } + ~CheckForAbort() + { + Release(); + } + void Release() + { + if (m_NeedRelease) { - if (m_NeedRelease) + m_NeedRelease = FALSE; + ThreadStore::TrapReturningThreads(FALSE); + ThreadStore::SetStackCrawlEvent(); + m_pThread->ResetThreadState(TS_StackCrawlNeeded); + if (!m_fHoldingThreadStoreLock) { - m_NeedRelease = FALSE; - ThreadStore::TrapReturningThreads(FALSE); - ThreadStore::SetStackCrawlEvent(); - m_pThread->ResetThreadState(TS_StackCrawlNeeded); - if (!m_fHoldingThreadStoreLock) - { - ThreadSuspend::UnlockThreadStore(); - } + ThreadSuspend::UnlockThreadStore(); } } - }; - CheckForAbort checkForAbort(this, fHoldingThreadStoreLock); + } + }; + CheckForAbort checkForAbort(this, fHoldingThreadStoreLock); + if (!UseActivationInjection()) + { + checkForAbort.Activate(); + } - // We own TS lock. The state of the Thread can not be changed. - if (m_State & TS_Unstarted) - { - // This thread is not yet started. + // We own TS lock. The state of the Thread can not be changed. + if (m_State & TS_Unstarted) + { + // This thread is not yet started. #ifdef _DEBUG - m_dwAbortPoint = 2; + m_dwAbortPoint = 2; #endif - return S_OK; - } + return S_OK; + } - if (GetThreadHandle() == INVALID_HANDLE_VALUE && - (m_State & TS_Unstarted) == 0) - { - // The thread is going to die or is already dead. - UnmarkThreadForAbort(); + if (GetThreadHandle() == INVALID_HANDLE_VALUE && + (m_State & TS_Unstarted) == 0) + { + // The thread is going to die or is already dead. + UnmarkThreadForAbort(); #ifdef _DEBUG - m_dwAbortPoint = 3; + m_dwAbortPoint = 3; #endif - return S_OK; - } + return S_OK; + } - // What if someone else has this thread suspended already? It'll depend where the - // thread got suspended. - // - // User Suspend: - // We'll just set the abort bit and hope for the best on the resume. - // - // GC Suspend: - // If it's suspended in jitted code, we'll hijack the IP. - // Consider race w/ GC suspension - // If it's suspended but not in jitted code, we'll get suspended for GC, the GC - // will complete, and then we'll abort the target thread. - // + // What if someone else has this thread suspended already? It'll depend where the + // thread got suspended. + // + // User Suspend: + // We'll just set the abort bit and hope for the best on the resume. + // + // GC Suspend: + // If it's suspended in jitted code, we'll hijack the IP. + // Consider race w/ GC suspension + // If it's suspended but not in jitted code, we'll get suspended for GC, the GC + // will complete, and then we'll abort the target thread. + // - // It's possible that the thread has completed the abort already. - // - if (!(m_State & TS_AbortRequested)) - { + // It's possible that the thread has completed the abort already. + // + if (!(m_State & TS_AbortRequested)) + { #ifdef _DEBUG - m_dwAbortPoint = 4; + m_dwAbortPoint = 4; #endif - return S_OK; - } + return S_OK; + } - // If a thread is Dead or Detached, abort is a NOP. - // - if (m_State & (TS_Dead | TS_Detached | TS_TaskReset)) - { - UnmarkThreadForAbort(); + // If a thread is Dead or Detached, abort is a NOP. + // + if (m_State & (TS_Dead | TS_Detached | TS_TaskReset)) + { + UnmarkThreadForAbort(); #ifdef _DEBUG - m_dwAbortPoint = 5; + m_dwAbortPoint = 5; #endif - return S_OK; - } + return S_OK; + } - // It's possible that some stub notices the AbortRequested bit -- even though we - // haven't done any real magic yet. If the thread has already started it's abort, we're - // done. - // - // Two more cases can be folded in here as well. If the thread is unstarted, it'll - // abort when we start it. - // - // If the thread is user suspended (SyncSuspended) -- we're out of luck. Set the bit and - // hope for the best on resume. - // - if ((m_State & TS_AbortInitiated) && !IsRudeAbort()) - { + // It's possible that some stub notices the AbortRequested bit -- even though we + // haven't done any real magic yet. If the thread has already started it's abort, we're + // done. + // + // Two more cases can be folded in here as well. If the thread is unstarted, it'll + // abort when we start it. + // + // If the thread is user suspended (SyncSuspended) -- we're out of luck. Set the bit and + // hope for the best on resume. + // + if ((m_State & TS_AbortInitiated) && !IsRudeAbort()) + { #ifdef _DEBUG - m_dwAbortPoint = 6; + m_dwAbortPoint = 6; #endif - break; - } + break; + } + if (UseActivationInjection()) + { + InjectActivation(ActivationReason::ThreadAbort); + } + else + { BOOL fOutOfRuntime = FALSE; BOOL fNeedStackCrawl = FALSE; @@ -1595,32 +1616,33 @@ Thread::UserAbort(EEPolicy::ThreadAbortTypes abortType, DWORD timeout) LPrepareRetry: checkForAbort.Release(); + } - // Don't do a Sleep. It's possible that the thread we are trying to abort is - // stuck in unmanaged code trying to get into the apartment that we are supposed - // to be pumping! Instead, ping the current thread's handle. Obviously this - // will time out, but it will pump if we need it to. - if (pCurThread) - { - pCurThread->Join(ABORT_POLL_TIMEOUT, TRUE); - } - else - { - ClrSleepEx(ABORT_POLL_TIMEOUT, FALSE); - } + // Don't do a Sleep. It's possible that the thread we are trying to abort is + // stuck in unmanaged code trying to get into the apartment that we are supposed + // to be pumping! Instead, ping the current thread's handle. Obviously this + // will time out, but it will pump if we need it to. + if (pCurThread) + { + pCurThread->Join(ABORT_POLL_TIMEOUT, TRUE); + } + else + { + ClrSleepEx(ABORT_POLL_TIMEOUT, FALSE); + } #ifdef _DEBUG - elapsed_time += ABORT_POLL_TIMEOUT; - if (g_pConfig->GetGCStressLevel() == 0 && !fAlreadyAssert) - { - _ASSERTE(elapsed_time < ABORT_FAIL_TIMEOUT); - fAlreadyAssert = TRUE; - } + elapsed_time += ABORT_POLL_TIMEOUT; + if (g_pConfig->GetGCStressLevel() == 0 && !fAlreadyAssert) + { + _ASSERTE(elapsed_time < ABORT_FAIL_TIMEOUT); + fAlreadyAssert = TRUE; + } #endif - } // while (true) - } + } // while (true) + if ((GetAbortEndTime() != MAXULONGLONG) && IsAbortRequested()) { while (TRUE) @@ -1630,13 +1652,6 @@ Thread::UserAbort(EEPolicy::ThreadAbortTypes abortType, DWORD timeout) return S_OK; } -#ifdef FEATURE_THREAD_ACTIVATION - if (UseSpecialUserModeApc()) - { - InjectActivation(ActivationReason::ThreadAbort); - } -#endif // FEATURE_THREAD_ACTIVATION - ULONGLONG curTime = CLRGetTickCount64(); if (curTime >= GetAbortEndTime()) { @@ -2479,8 +2494,10 @@ void RedirectedThreadFrame::ExceptionUnwind() Thread* pThread = GetThread(); // Allow future use to avoid repeatedly new'ing - pThread->UnmarkRedirectContextInUse(m_Regs); - m_Regs = NULL; + if (pThread->UnmarkRedirectContextInUse(m_Regs)) + { + m_Regs = NULL; + } } #ifndef TARGET_UNIX @@ -5862,9 +5879,15 @@ void HandleSuspensionForInterruptedThread(CONTEXT *interruptedContext) pThread->PulseGCMode(); - frame.Pop(pThread); + INSTALL_MANAGED_EXCEPTION_DISPATCHER; + INSTALL_UNWIND_AND_CONTINUE_HANDLER; pThread->HandleThreadAbort(); + + UNINSTALL_UNWIND_AND_CONTINUE_HANDLER; + UNINSTALL_MANAGED_EXCEPTION_DISPATCHER; + + frame.Pop(pThread); } else { @@ -5981,7 +6004,7 @@ bool Thread::InjectActivation(ActivationReason reason) _ASSERTE(success); return true; #elif defined(TARGET_UNIX) - _ASSERTE(reason == ActivationReason::SuspendForGC); + _ASSERTE((reason == ActivationReason::SuspendForGC) || (reason == ActivationReason::ThreadAbort)); static ConfigDWORD injectionEnabled; if (injectionEnabled.val(CLRConfig::INTERNAL_ThreadSuspendInjection) == 0) diff --git a/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs b/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs index 2d615b36ce9042..b0dab8191f87e1 100644 --- a/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs +++ b/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs @@ -33,9 +33,9 @@ public void CancelOnTimeout() // Tests that catch blocks are not aborted. The action catches the ThreadAbortException and throws an exception of a different type. [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/72703", TestPlatforms.AnyUnix)] public void CancelOnTimeout_ThrowFromCatch() { + Console.WriteLine("CancelOnTimeout_ThrowFromCatch"); var cts = new CancellationTokenSource(); cts.CancelAfter(200); RunTest(LengthyAction_ThrowFromCatch, cts.Token); @@ -48,9 +48,9 @@ public void CancelOnTimeout_ThrowFromCatch() // Tests that finally blocks are not aborted. The action throws an exception from a finally block. [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/72703", TestPlatforms.AnyUnix)] public void CancelOnTimeout_ThrowFromFinally() { + Console.WriteLine("CancelOnTimeout_ThrowFromFinally"); var cts = new CancellationTokenSource(); cts.CancelAfter(200); RunTest(LengthyAction_ThrowFromFinally, cts.Token); @@ -61,9 +61,9 @@ public void CancelOnTimeout_ThrowFromFinally() // Tests that finally blocks are not aborted. The action throws an exception from a try block. [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/72703", TestPlatforms.AnyUnix)] public void CancelOnTimeout_Finally() { + Console.WriteLine("CancelOnTimeout_Finally"); var cts = new CancellationTokenSource(); cts.CancelAfter(200); RunTest(LengthyAction_Finally, cts.Token); @@ -75,9 +75,9 @@ public void CancelOnTimeout_Finally() // Tests cancellation before calling the Run method [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/72703", TestPlatforms.AnyUnix)] public void CancelBeforeRun() { + Console.WriteLine("CancelBeforeRun"); var cts = new CancellationTokenSource(); cts.Cancel(); Thread.Sleep(100); @@ -88,9 +88,9 @@ public void CancelBeforeRun() // Tests cancellation by the action itself [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/72703", TestPlatforms.AnyUnix)] public void CancelItself() { + Console.WriteLine("CancelItself"); _cts = new CancellationTokenSource(); RunTest(Action_CancelItself, _cts.Token); From e02cef2c2dbdac24e626ae682a382c657ade3647 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 4 Aug 2022 22:32:02 +0200 Subject: [PATCH 2/5] Remove Console.WriteLine calls from the test methods --- .../tests/System/Runtime/ControlledExecutionTests.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs b/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs index b0dab8191f87e1..469e3cfd60b823 100644 --- a/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs +++ b/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs @@ -35,7 +35,6 @@ public void CancelOnTimeout() [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelOnTimeout_ThrowFromCatch() { - Console.WriteLine("CancelOnTimeout_ThrowFromCatch"); var cts = new CancellationTokenSource(); cts.CancelAfter(200); RunTest(LengthyAction_ThrowFromCatch, cts.Token); @@ -50,7 +49,6 @@ public void CancelOnTimeout_ThrowFromCatch() [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelOnTimeout_ThrowFromFinally() { - Console.WriteLine("CancelOnTimeout_ThrowFromFinally"); var cts = new CancellationTokenSource(); cts.CancelAfter(200); RunTest(LengthyAction_ThrowFromFinally, cts.Token); @@ -63,7 +61,6 @@ public void CancelOnTimeout_ThrowFromFinally() [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelOnTimeout_Finally() { - Console.WriteLine("CancelOnTimeout_Finally"); var cts = new CancellationTokenSource(); cts.CancelAfter(200); RunTest(LengthyAction_Finally, cts.Token); @@ -77,7 +74,6 @@ public void CancelOnTimeout_Finally() [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelBeforeRun() { - Console.WriteLine("CancelBeforeRun"); var cts = new CancellationTokenSource(); cts.Cancel(); Thread.Sleep(100); @@ -90,7 +86,6 @@ public void CancelBeforeRun() [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelItself() { - Console.WriteLine("CancelItself"); _cts = new CancellationTokenSource(); RunTest(Action_CancelItself, _cts.Token); From 8de15f86f52986e95a36912bc0574c4a83c031b2 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 5 Aug 2022 00:45:19 +0200 Subject: [PATCH 3/5] Fix build on non-amd64 windows --- src/coreclr/vm/threadsuspend.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index f3b85bac4e7a47..29332af32e493e 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -1412,11 +1412,13 @@ Thread::UserAbort(EEPolicy::ThreadAbortTypes abortType, DWORD timeout) break; } +#ifdef FEATURE_THREAD_ACTIVATION if (UseActivationInjection()) { InjectActivation(ActivationReason::ThreadAbort); } else +#endif // FEATURE_THREAD_ACTIVATION { BOOL fOutOfRuntime = FALSE; BOOL fNeedStackCrawl = FALSE; From 1948901324bac04a891c0849bfcc735d58872200 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 5 Aug 2022 21:59:25 +0200 Subject: [PATCH 4/5] Fix thread abort on Alpine Linux On Alpine Linux, libunwind cannot unwind correctly over signal frames. We have been handling that for hardware exceptions, but with the new possibility of thread abort exception being thrown from the activation handler, we need to apply the same treatment to the activation signal too. We save a location of the windows style context in the activation_injection_handler and a return address in this function for a call to the actual registered handler invocation. Then in PAL_VirtualUnwind, when we detect that we are trying to unwind from this location, we use the saved context instead of calling libunwind. --- src/coreclr/pal/src/exception/seh-unwind.cpp | 15 ++++++++ src/coreclr/pal/src/exception/seh.cpp | 1 + src/coreclr/pal/src/exception/signal.cpp | 38 ++++++++++++++++++-- src/coreclr/pal/src/include/pal/seh.hpp | 4 ++- 4 files changed, 55 insertions(+), 3 deletions(-) diff --git a/src/coreclr/pal/src/exception/seh-unwind.cpp b/src/coreclr/pal/src/exception/seh-unwind.cpp index 9fe6c61d0f8286..f718be6af54c27 100644 --- a/src/coreclr/pal/src/exception/seh-unwind.cpp +++ b/src/coreclr/pal/src/exception/seh-unwind.cpp @@ -558,6 +558,9 @@ void GetContextPointers(unw_cursor_t *cursor, unw_context_t *unwContext, KNONVOL // Frame pointer relative offset of a local containing a pointer to the windows style context of a location // where a hardware exception occurred. int g_hardware_exception_context_locvar_offset = 0; +// Frame pointer relative offset of a local containing a pointer to the windows style context of a location +// where an activation signal interrupted the thread. +int g_inject_activation_context_locvar_offset = 0; BOOL PAL_VirtualUnwind(CONTEXT *context, KNONVOLATILE_CONTEXT_POINTERS *contextPointers) { @@ -579,6 +582,18 @@ BOOL PAL_VirtualUnwind(CONTEXT *context, KNONVOLATILE_CONTEXT_POINTERS *contextP return TRUE; } + // Check if the PC is the return address from the InvokeActivationHandler. + // If that's the case, extract its local variable containing a pointer to the windows style context of the activation + // injection location and return that. This skips the signal handler trampoline that the libunwind + // cannot cross on some systems. + if ((void*)curPc == g_InvokeActivationHandlerReturnAddress) + { + CONTEXT* activationContext = (CONTEXT*)(CONTEXTGetFP(context) + g_inject_activation_context_locvar_offset); + memcpy_s(context, sizeof(CONTEXT), activationContext, sizeof(CONTEXT)); + + return TRUE; + } + if ((context->ContextFlags & CONTEXT_EXCEPTION_ACTIVE) != 0) { // The current frame is a source of hardware exception. Due to the fact that diff --git a/src/coreclr/pal/src/exception/seh.cpp b/src/coreclr/pal/src/exception/seh.cpp index 6b02ba34e26f8e..2045084a5683e8 100644 --- a/src/coreclr/pal/src/exception/seh.cpp +++ b/src/coreclr/pal/src/exception/seh.cpp @@ -61,6 +61,7 @@ PGET_GCMARKER_EXCEPTION_CODE g_getGcMarkerExceptionCode = NULL; // Return address of the SEHProcessException, which is used to enable walking over // the signal handler trampoline on some Unixes where the libunwind cannot do that. void* g_SEHProcessExceptionReturnAddress = NULL; +void* g_InvokeActivationHandlerReturnAddress = NULL; /* Internal function definitions **********************************************/ diff --git a/src/coreclr/pal/src/exception/signal.cpp b/src/coreclr/pal/src/exception/signal.cpp index 604c9ffb1c069c..cfe48452578889 100644 --- a/src/coreclr/pal/src/exception/signal.cpp +++ b/src/coreclr/pal/src/exception/signal.cpp @@ -73,6 +73,7 @@ typedef void (*SIGFUNC)(int, siginfo_t *, void *); static void sigterm_handler(int code, siginfo_t *siginfo, void *context); #ifdef INJECT_ACTIVATION_SIGNAL static void inject_activation_handler(int code, siginfo_t *siginfo, void *context); +extern void* g_InvokeActivationHandlerReturnAddress; #endif static void sigill_handler(int code, siginfo_t *siginfo, void *context); @@ -775,6 +776,28 @@ static void sigterm_handler(int code, siginfo_t *siginfo, void *context) } #ifdef INJECT_ACTIVATION_SIGNAL + +/*++ +Function : + InvokeActivationHandler + + Invoke the registered activation handler. + It also saves the return address (inject_activation_handler) so that PAL_VirtualUnwind can detect that + it has reached that method and use the context stored in the winContext there to unwind to the code + where the activation was injected. This is necessary on Alpine Linux where the libunwind cannot correctly + unwind past the signal frame. + +Parameters : + Windows style context of the location where the activation was injected + +(no return value) +--*/ +static void InvokeActivationHandler(CONTEXT *pWinContext) +{ + g_InvokeActivationHandlerReturnAddress = __builtin_return_address(0); + g_activationFunction(pWinContext); +} + /*++ Function : inject_activation_handler @@ -803,15 +826,26 @@ static void inject_activation_handler(int code, siginfo_t *siginfo, void *contex native_context_t *ucontext = (native_context_t *)context; CONTEXT winContext; + // Pre-populate context with data from current frame, because ucontext doesn't have some data (e.g. SS register) + // which is required for restoring context + RtlCaptureContext(&winContext); + + ULONG contextFlags = CONTEXT_CONTROL | CONTEXT_INTEGER | CONTEXT_FLOATING_POINT; + +#if defined(HOST_AMD64) + contextFlags |= CONTEXT_XSTATE; +#endif + CONTEXTFromNativeContext( ucontext, &winContext, - CONTEXT_CONTROL | CONTEXT_INTEGER); + contextFlags); if (g_safeActivationCheckFunction(CONTEXTGetPC(&winContext), /* checkingCurrentThread */ TRUE)) { + g_inject_activation_context_locvar_offset = (int)((char*)&winContext - (char*)__builtin_frame_address(0)); int savedErrNo = errno; // Make sure that errno is not modified - g_activationFunction(&winContext); + InvokeActivationHandler(&winContext); errno = savedErrNo; // Activation function may have modified the context, so update it. diff --git a/src/coreclr/pal/src/include/pal/seh.hpp b/src/coreclr/pal/src/include/pal/seh.hpp index 327fe0d7fb03e2..5109eca56d79bd 100644 --- a/src/coreclr/pal/src/include/pal/seh.hpp +++ b/src/coreclr/pal/src/include/pal/seh.hpp @@ -148,7 +148,9 @@ CorUnix::PAL_ERROR SEHDisable(CorUnix::CPalThread *pthrCurrent); // Offset of the local variable containing pointer to windows style context in the common_signal_handler / PAL_DispatchException function. // This offset is relative to the frame pointer. extern int g_hardware_exception_context_locvar_offset; - +// Offset of the local variable containing pointer to windows style context in the inject_activation_handler. +// This offset is relative to the frame pointer. +extern int g_inject_activation_context_locvar_offset; #endif /* _PAL_SEH_HPP_ */ From bae3963faa9a428dc1bd86c380623c8b41f8c8b2 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Sat, 6 Aug 2022 02:51:58 +0200 Subject: [PATCH 5/5] Added forgotten inlining disabling for the InvokeActivationHandler --- src/coreclr/pal/src/exception/signal.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/pal/src/exception/signal.cpp b/src/coreclr/pal/src/exception/signal.cpp index cfe48452578889..108b64ae9954d1 100644 --- a/src/coreclr/pal/src/exception/signal.cpp +++ b/src/coreclr/pal/src/exception/signal.cpp @@ -792,6 +792,7 @@ Parameters : (no return value) --*/ +__attribute__((noinline)) static void InvokeActivationHandler(CONTEXT *pWinContext) { g_InvokeActivationHandlerReturnAddress = __builtin_return_address(0);