From 8f4ad9ff258315830d97693ff1d88245c2a7d86b Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Mon, 2 Sep 2024 12:09:25 -0400 Subject: [PATCH 1/5] Avoid using OpenThread in out of process thread context scenarios --- src/coreclr/debug/di/process.cpp | 37 +++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index aae787486adbc9..8a15905d08072d 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -11157,17 +11157,30 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded\n")); #if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) - HandleHolder hThread = OpenThread( - THREAD_GET_CONTEXT | THREAD_SET_CONTEXT | THREAD_QUERY_INFORMATION | THREAD_SUSPEND_RESUME, - FALSE, // thread handle is not inheritable. - dwThreadId); + CordbThread * pThread = TryLookupOrCreateThreadByVolatileOSId(dwThreadId); - if (hThread == NULL) + IDacDbiInterface* pDAC = GetDAC(); + if (pDAC == NULL) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from OpenThread\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - DAC not initialized\n")); ThrowHR(E_UNEXPECTED); } + HANDLE hOutOfProcThread = pDAC->GetThreadHandle(pThread->m_vmThreadToken); + if (hOutOfProcThread == NULL) + { + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Failed to get thread handle\n")); + ThrowHR(E_UNEXPECTED); + } + + HandleHolder hThread; + BOOL fSuccess = DuplicateHandle(UnsafeGetProcessHandle(), hOutOfProcThread, ::GetCurrentProcess(), &hThread, 0, FALSE, DUPLICATE_SAME_ACCESS); + if (!fSuccess) + { + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from DuplicateHandle\n")); + ThrowHR(HRESULT_FROM_GetLastError()); + } + DWORD previousSuspendCount = ::SuspendThread(hThread); if (previousSuspendCount == (DWORD)-1) { @@ -11178,8 +11191,12 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) DT_CONTEXT context = { 0 }; context.ContextFlags = CONTEXT_FULL; - HRESULT hr = GetDataTarget()->GetThreadContext(dwThreadId, CONTEXT_FULL, sizeof(DT_CONTEXT), reinterpret_cast (&context)); - IfFailThrow(hr); + BOOL success = ::GetThreadContext(hThread, (CONTEXT*)(&context)); + if (!success) + { + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from GetThreadContext\n")); + ThrowHR(HRESULT_FROM_GetLastError()); + } TADDR lsContextAddr = (TADDR)context.Rcx; DWORD contextSize = (DWORD)context.Rdx; @@ -11198,7 +11215,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) PCONTEXT pContext = (PCONTEXT)_alloca(contextSize); ULONG32 cbRead; - hr = GetDataTarget()->ReadVirtual(lsContextAddr, reinterpret_cast(pContext), contextSize, &cbRead); + HRESULT hr = GetDataTarget()->ReadVirtual(lsContextAddr, reinterpret_cast(pContext), contextSize, &cbRead); if (FAILED(hr)) { _ASSERTE(!"ReadVirtual failed"); @@ -11232,7 +11249,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) // The initialize call should fail but return contextSize contextSize = 0; DWORD contextFlags = pContext->ContextFlags; - BOOL success = InitializeContext(NULL, contextFlags, NULL, &contextSize); + success = InitializeContext(NULL, contextFlags, NULL, &contextSize); if(success || GetLastError() != ERROR_INSUFFICIENT_BUFFER) { From b090639cffb3531c4ea366bbe245e77ad8383152 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Sun, 8 Sep 2024 10:54:30 -0400 Subject: [PATCH 2/5] Add comments --- src/coreclr/debug/di/process.cpp | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 8a15905d08072d..2f79bf97c475d4 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -11157,6 +11157,23 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded\n")); #if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) + // Before we can read the left side context information, we must: + // 1. obtain the thread handle + // 2. suspened the thread + // 3. read the thread context, and from that read the pointer to the left-side context and the size of the context + // 4. then we can perform the actual SetThreadContext operation + // 5. lastly, we must resume the thread + // For the first step of obtaining the thread handle, + // we have previously attempted to use ::OpenThead to get a handle to the thread. + // However, there are situations where OpenThread can fail with an Access Denied error. + // From https://github.com/dotnet/runtime/issues/107263, the control-c handler in + // Windows causes the process to have higher privileges. + // We are now using the following approach to acess the thread handle, which is the same + // approach used by CordbThread::RefreshHandle: + // 1. Get the thread handle from the DAC + // 2. Duplicate the handle to the current process + + // lookup the CordbThread by thread ID, so that we can access the left-side thread handle CordbThread * pThread = TryLookupOrCreateThreadByVolatileOSId(dwThreadId); IDacDbiInterface* pDAC = GetDAC(); @@ -11173,6 +11190,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) ThrowHR(E_UNEXPECTED); } + // Duplicate the thread handle to the current process HandleHolder hThread; BOOL fSuccess = DuplicateHandle(UnsafeGetProcessHandle(), hOutOfProcThread, ::GetCurrentProcess(), &hThread, 0, FALSE, DUPLICATE_SAME_ACCESS); if (!fSuccess) @@ -11181,6 +11199,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) ThrowHR(HRESULT_FROM_GetLastError()); } + // Suspend the thread and so that we can read the thread context. DWORD previousSuspendCount = ::SuspendThread(hThread); if (previousSuspendCount == (DWORD)-1) { @@ -11191,6 +11210,9 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) DT_CONTEXT context = { 0 }; context.ContextFlags = CONTEXT_FULL; + // we originally used GetDataTarget()->GetThreadContext, but + // the ipmlementation uses ShimLocalDataTarget::GetThreadContext which + // depends on OpenThread which might fail with an Access Denied error (see note above) BOOL success = ::GetThreadContext(hThread, (CONTEXT*)(&context)); if (!success) { @@ -11198,9 +11220,12 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) ThrowHR(HRESULT_FROM_GetLastError()); } + // Read the pointer to the left-side context and the size of the context from the thread context. TADDR lsContextAddr = (TADDR)context.Rcx; DWORD contextSize = (DWORD)context.Rdx; + // Read the expected Rip and Rsp from the thread context. This is used to + // validate the context read from the left-side. TADDR expectedRip = (TADDR)context.R8; TADDR expectedRsp = (TADDR)context.R9; @@ -11293,6 +11318,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) DWORD lastError = 0; + // Perform the actual SetThreadContext operation. success = ::SetThreadContext(hThread, pFrameContext); if (!success) { @@ -11302,6 +11328,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Set Thread Context Completed: Success=%d GetLastError=%d hr=0x%X\n", success, lastError, HRESULT_FROM_WIN32(lastError))); _ASSERTE(success); + // Now that we have completed the SetThreadContext, resume the thread DWORD suspendCount = ::ResumeThread(hThread); if (suspendCount == (DWORD)-1) { From cc55382279c37590f942ab641851a50f2534068e Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Mon, 9 Sep 2024 10:57:52 -0400 Subject: [PATCH 3/5] Update src/coreclr/debug/di/process.cpp Co-authored-by: mikelle-rogers <45022607+mikelle-rogers@users.noreply.github.com> --- src/coreclr/debug/di/process.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 2f79bf97c475d4..415cac938d8950 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -11164,7 +11164,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) // 4. then we can perform the actual SetThreadContext operation // 5. lastly, we must resume the thread // For the first step of obtaining the thread handle, - // we have previously attempted to use ::OpenThead to get a handle to the thread. + // we have previously attempted to use ::OpenThread to get a handle to the thread. // However, there are situations where OpenThread can fail with an Access Denied error. // From https://github.com/dotnet/runtime/issues/107263, the control-c handler in // Windows causes the process to have higher privileges. From 0cdbf5a5b9fa7e4c978dc8b44d1d2bf477704797 Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Mon, 9 Sep 2024 11:31:43 -0400 Subject: [PATCH 4/5] Update src/coreclr/debug/di/process.cpp Co-authored-by: mikelle-rogers <45022607+mikelle-rogers@users.noreply.github.com> --- src/coreclr/debug/di/process.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 415cac938d8950..2461fe0cb3962f 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -11168,7 +11168,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) // However, there are situations where OpenThread can fail with an Access Denied error. // From https://github.com/dotnet/runtime/issues/107263, the control-c handler in // Windows causes the process to have higher privileges. - // We are now using the following approach to acess the thread handle, which is the same + // We are now using the following approach to access the thread handle, which is the same // approach used by CordbThread::RefreshHandle: // 1. Get the thread handle from the DAC // 2. Duplicate the handle to the current process From 8d67a610aff23e10b6b526fbb4551ecdcd424b10 Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Mon, 9 Sep 2024 15:18:26 -0400 Subject: [PATCH 5/5] Update src/coreclr/debug/di/process.cpp Co-authored-by: Noah Falk --- src/coreclr/debug/di/process.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 2461fe0cb3962f..fffd6df484825f 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -11211,7 +11211,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) context.ContextFlags = CONTEXT_FULL; // we originally used GetDataTarget()->GetThreadContext, but - // the ipmlementation uses ShimLocalDataTarget::GetThreadContext which + // the implementation uses ShimLocalDataTarget::GetThreadContext which // depends on OpenThread which might fail with an Access Denied error (see note above) BOOL success = ::GetThreadContext(hThread, (CONTEXT*)(&context)); if (!success)