Skip to content

Commit

Permalink
Use alternatives to redirecting threads via IP changes in the context…
Browse files Browse the repository at this point in the history
…, for a few cases on Windows (#55649)

* Use alternatives to redirecting threads via IP changes in the context, for a few cases on Windows

- For GC/debugger suspension, a new variant of an APC is used to interrupt the thread
- For hardware exceptions, an exception is raised from the vectored exception handler instead of redirecting and then raising an exception
- Made changes to enable /guard:ehcont and /cetcompat for binaries, but commented out the enablement for now since it looks like we will first need .pgd files generated with /guard:ehcont and published for release builds to succeed
  • Loading branch information
kouvel committed Jul 15, 2021
1 parent 7aaffcb commit 663013d
Show file tree
Hide file tree
Showing 16 changed files with 527 additions and 133 deletions.
17 changes: 16 additions & 1 deletion eng/native/configurecompiler.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ add_compile_definitions("$<$<CONFIG:CHECKED>:DEBUG;_DEBUG;_DBG;URTBLDENV_FRIENDL
add_compile_definitions("$<$<OR:$<CONFIG:RELEASE>,$<CONFIG:RELWITHDEBINFO>>:NDEBUG;URTBLDENV_FRIENDLY=Retail>")

if (MSVC)
add_linker_flag(/GUARD:CF)
add_linker_flag(/guard:cf)
#if (CLR_CMAKE_HOST_ARCH_AMD64)
# add_linker_flag(/guard:ehcont)
#endif (CLR_CMAKE_HOST_ARCH_AMD64)

# Linker flags
#
Expand All @@ -71,6 +74,10 @@ if (MSVC)
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /IGNORE:4197,4013,4254,4070,4221")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /SUBSYSTEM:WINDOWS,${WINDOWS_SUBSYSTEM_VERSION}")

#if (CLR_CMAKE_HOST_ARCH_AMD64)
# set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /CETCOMPAT")
#endif (CLR_CMAKE_HOST_ARCH_AMD64)

set(CMAKE_STATIC_LINKER_FLAGS "${CMAKE_STATIC_LINKER_FLAGS} /IGNORE:4221")

set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /DEBUG")
Expand Down Expand Up @@ -565,6 +572,14 @@ if (MSVC)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /guard:cf")
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /guard:cf")

# Enable EH-continuation table for native components for amd64 builds
# Added using variables instead of add_compile_options to let individual projects override it
#if (CLR_CMAKE_HOST_ARCH_AMD64)
# set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /guard:ehcont")
# set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /guard:ehcont")
# set(CMAKE_ASM_MASM_FLAGS "${CMAKE_C_FLAGS} /guard:ehcont")
#endif (CLR_CMAKE_HOST_ARCH_AMD64)

# Statically linked CRT (libcmt[d].lib, libvcruntime[d].lib and libucrt[d].lib) by default. This is done to avoid
# linking in VCRUNTIME140.DLL for a simplified xcopy experience by reducing the dependency on VC REDIST.
#
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/clrdefinitions.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,10 @@ if (NOT CLR_CMAKE_TARGET_ARCH_I386 OR NOT CLR_CMAKE_TARGET_WIN32)
add_compile_definitions($<$<NOT:$<BOOL:$<TARGET_PROPERTY:IGNORE_DEFAULT_TARGET_ARCH>>>:FEATURE_EH_FUNCLETS>)
endif (NOT CLR_CMAKE_TARGET_ARCH_I386 OR NOT CLR_CMAKE_TARGET_WIN32)

if (CLR_CMAKE_TARGET_WIN32 AND CLR_CMAKE_TARGET_ARCH_AMD64)
add_definitions(-DFEATURE_SPECIAL_USER_MODE_APC)
endif()


# Use this function to enable building with a specific target OS and architecture set of defines
# This is known to work for the set of defines used by the JIT and gcinfo, it is not likely correct for
Expand Down
28 changes: 20 additions & 8 deletions src/coreclr/debug/ee/debugger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -992,10 +992,9 @@ Debugger::~Debugger()
}

#if defined(FEATURE_HIJACK) && !defined(TARGET_UNIX)
typedef void (*PFN_HIJACK_FUNCTION) (void);

// Given the start address and the end address of a function, return a MemoryRange for the function.
inline MemoryRange GetMemoryRangeForFunction(PFN_HIJACK_FUNCTION pfnStart, PFN_HIJACK_FUNCTION pfnEnd)
inline MemoryRange GetMemoryRangeForFunction(void *pfnStart, void *pfnEnd)
{
PCODE pfnStartAddress = (PCODE)GetEEFuncEntryPoint(pfnStart);
PCODE pfnEndAddress = (PCODE)GetEEFuncEntryPoint(pfnEnd);
Expand All @@ -1016,6 +1015,11 @@ MemoryRange Debugger::s_hijackFunction[kMaxHijackFunctions] =
GetMemoryRangeForFunction(RedirectedHandledJITCaseForGCStress_Stub,
RedirectedHandledJITCaseForGCStress_StubEnd)
#endif // HAVE_GCCOVER && TARGET_AMD64
#ifdef FEATURE_SPECIAL_USER_MODE_APC
,
GetMemoryRangeForFunction(ApcActivationCallbackStub,
ApcActivationCallbackStubEnd)
#endif // FEATURE_SPECIAL_USER_MODE_APC
};
#endif // FEATURE_HIJACK && !TARGET_UNIX

Expand Down Expand Up @@ -15890,7 +15894,7 @@ HRESULT Debugger::UpdateSpecialThreadList(DWORD cThreadArrayLength,
//
// 3) If the IP is in the prolog or epilog of a managed function.
//
BOOL Debugger::IsThreadContextInvalid(Thread *pThread)
BOOL Debugger::IsThreadContextInvalid(Thread *pThread, CONTEXT *pCtx)
{
CONTRACTL
{
Expand All @@ -15902,14 +15906,22 @@ BOOL Debugger::IsThreadContextInvalid(Thread *pThread)
BOOL invalid = FALSE;

// Get the thread context.
BOOL success = pCtx != NULL;
CONTEXT ctx;
ctx.ContextFlags = CONTEXT_CONTROL;
BOOL success = pThread->GetThreadContext(&ctx);
if (!success)
{
ctx.ContextFlags = CONTEXT_CONTROL;
BOOL success = pThread->GetThreadContext(&ctx);
if (success)
{
pCtx = &ctx;
}
}

if (success)
{
// Check single-step flag
if (IsSSFlagEnabled(reinterpret_cast<DT_CONTEXT *>(&ctx) ARM_ARG(pThread) ARM64_ARG(pThread)))
if (IsSSFlagEnabled(reinterpret_cast<DT_CONTEXT *>(pCtx) ARM_ARG(pThread) ARM64_ARG(pThread)))
{
// Can't hijack a thread whose SS-flag is set. This could lead to races
// with the thread taking the SS-exception.
Expand All @@ -15923,7 +15935,7 @@ BOOL Debugger::IsThreadContextInvalid(Thread *pThread)
{
#ifdef TARGET_X86
// Grab Eip - 1
LPVOID address = (((BYTE*)GetIP(&ctx)) - 1);
LPVOID address = (((BYTE*)GetIP(pCtx)) - 1);

EX_TRY
{
Expand All @@ -15934,7 +15946,7 @@ BOOL Debugger::IsThreadContextInvalid(Thread *pThread)
if (AddressIsBreakpoint((CORDB_ADDRESS_TYPE*)address))
{
size_t prologSize; // Unused...
if (g_pEEInterface->IsInPrologOrEpilog((BYTE*)GetIP(&ctx), &prologSize))
if (g_pEEInterface->IsInPrologOrEpilog((BYTE*)GetIP(pCtx), &prologSize))
{
LOG((LF_CORDB, LL_INFO1000, "D::ITCI: thread is after a BP and in prolog or epilog.\n"));
invalid = TRUE;
Expand Down
10 changes: 9 additions & 1 deletion src/coreclr/debug/ee/debugger.h
Original file line number Diff line number Diff line change
Expand Up @@ -2505,7 +2505,7 @@ class Debugger : public DebugInterface
SIZE_T *rgVal2,
BYTE **rgpVCs);

BOOL IsThreadContextInvalid(Thread *pThread);
BOOL IsThreadContextInvalid(Thread *pThread, T_CONTEXT *pCtx);

// notification for SQL fiber debugging support
void CreateConnection(CONNID dwConnectionId, __in_z WCHAR *wzName);
Expand Down Expand Up @@ -2875,6 +2875,9 @@ class Debugger : public DebugInterface
#if defined(HAVE_GCCOVER) && defined(TARGET_AMD64)
kRedirectedForGCStress,
#endif // HAVE_GCCOVER && TARGET_AMD64
#ifdef FEATURE_SPECIAL_USER_MODE_APC
kRedirectedForApcActivation,
#endif // FEATURE_SPECIAL_USER_MODE_APC
kMaxHijackFunctions,
};

Expand Down Expand Up @@ -2976,6 +2979,11 @@ void RedirectedHandledJITCaseForUserSuspend_StubEnd();
void RedirectedHandledJITCaseForGCStress_Stub();
void RedirectedHandledJITCaseForGCStress_StubEnd();
#endif // HAVE_GCCOVER && TARGET_AMD64

#ifdef FEATURE_SPECIAL_USER_MODE_APC
void NTAPI ApcActivationCallbackStub(ULONG_PTR Parameter);
void ApcActivationCallbackStubEnd();
#endif // FEATURE_SPECIAL_USER_MODE_APC
};


Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/dlls/clretwrc/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@ if(CLR_CMAKE_HOST_WIN32)
string(REPLACE "/LTCG" "" CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO ${CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO})
string(REPLACE "/LTCG" "" CMAKE_STATIC_LINKER_FLAGS_RELWITHDEBINFO ${CMAKE_STATIC_LINKER_FLAGS_RELWITHDEBINFO})

# remove /guard:cf from resource-only libraries
# remove /guard:cf, /guard:ehcont, and /CETCOMPAT from resource-only libraries
string(REPLACE "/guard:cf" "" CMAKE_SHARED_LINKER_FLAGS ${CMAKE_SHARED_LINKER_FLAGS})
if (CLR_CMAKE_HOST_ARCH_AMD64)
string(REPLACE "/guard:ehcont" "" CMAKE_SHARED_LINKER_FLAGS ${CMAKE_SHARED_LINKER_FLAGS})
string(REPLACE "/CETCOMPAT" "" CMAKE_SHARED_LINKER_FLAGS ${CMAKE_SHARED_LINKER_FLAGS})
endif (CLR_CMAKE_HOST_ARCH_AMD64)

set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /NOENTRY")
endif(CLR_CMAKE_HOST_WIN32)
Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/dlls/mscorrc/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,12 @@ if(CLR_CMAKE_HOST_WIN32)
string(REPLACE "/LTCG" "" CMAKE_SHARED_LINKER_FLAGS_RELEASE ${CMAKE_SHARED_LINKER_FLAGS_RELEASE})
string(REPLACE "/LTCG" "" CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO ${CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO})

# remove /guard:cf from resource-only libraries
# remove /guard:cf, /guard:ehcont, and /CETCOMPAT from resource-only libraries
string(REPLACE "/guard:cf" "" CMAKE_SHARED_LINKER_FLAGS ${CMAKE_SHARED_LINKER_FLAGS})
if (CLR_CMAKE_HOST_ARCH_AMD64)
string(REPLACE "/guard:ehcont" "" CMAKE_SHARED_LINKER_FLAGS ${CMAKE_SHARED_LINKER_FLAGS})
string(REPLACE "/CETCOMPAT" "" CMAKE_SHARED_LINKER_FLAGS ${CMAKE_SHARED_LINKER_FLAGS})
endif (CLR_CMAKE_HOST_ARCH_AMD64)

add_library_clr(mscorrc SHARED
include.rc
Expand Down
33 changes: 32 additions & 1 deletion src/coreclr/vm/amd64/RedirectedHandledJITCase.asm
Original file line number Diff line number Diff line change
Expand Up @@ -232,5 +232,36 @@ NESTED_END NakedThrowHelper2, _TEXT
GenerateRedirectedStubWithFrame NakedThrowHelper, FixContextHandler, NakedThrowHelper2


end
ifdef FEATURE_SPECIAL_USER_MODE_APC

extern ?ApcActivationCallback@Thread@@CAX_K@Z:proc

; extern "C" void NTAPI ApcActivationCallbackStub(ULONG_PTR Parameter);
NESTED_ENTRY ApcActivationCallbackStub, _TEXT, FixRedirectContextHandler

push_nonvol_reg rbp
alloc_stack 30h ; padding for alignment, CONTEXT *, callee scratch area
set_frame rbp, 0
.errnz REDIRECTSTUB_ESTABLISHER_OFFSET_RBP, REDIRECTSTUB_ESTABLISHER_OFFSET_RBP has changed - update asm stubs
END_PROLOGUE

; Save the pointer to the interrupted context on the stack for the stack walker
mov rax, [rcx + OFFSETOF__APC_CALLBACK_DATA__ContextRecord]
mov [rbp + 20h], rax
.errnz REDIRECTSTUB_RBP_OFFSET_CONTEXT - 20h, REDIRECTSTUB_RBP_OFFSET_CONTEXT has changed - update asm stubs

call ?ApcActivationCallback@Thread@@CAX_K@Z

add rsp, 30h
pop rbp
ret

; Put a label here to tell the debugger where the end of this function is.
PATCH_LABEL ApcActivationCallbackStubEnd

NESTED_END ApcActivationCallbackStub, _TEXT

endif ; FEATURE_SPECIAL_USER_MODE_APC


end
4 changes: 4 additions & 0 deletions src/coreclr/vm/amd64/asmconstants.h
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,10 @@ ASMCONSTANTS_C_ASSERT(OFFSET__TEB__ThreadLocalStoragePointer == offsetof(TEB, Th

#define THROWSTUB_ESTABLISHER_OFFSET_FaultingExceptionFrame 0x30

#ifdef FEATURE_SPECIAL_USER_MODE_APC
#define OFFSETOF__APC_CALLBACK_DATA__ContextRecord 0x8
#endif

#define Thread__ObjectRefFlush ?ObjectRefFlush@Thread@@SAXPEAV1@@Z


Expand Down
1 change: 1 addition & 0 deletions src/coreclr/vm/ceemain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,7 @@ void EEStartupHelper()

IfFailGo(ExecutableAllocator::StaticInitialize(FatalErrorHandler));

Thread::StaticInitialize();
ThreadpoolMgr::StaticInitialize();

MethodDescBackpatchInfoTracker::StaticInitialize();
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/dbginterface.h
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ class DebugInterface
SIZE_T *rgVal2,
BYTE **rgpVCs) = 0;

virtual BOOL IsThreadContextInvalid(Thread *pThread) = 0;
virtual BOOL IsThreadContextInvalid(Thread *pThread, CONTEXT *pCtx) = 0;

// For Just-My-Code (aka Just-User-Code).
// The jit inserts probes that look like.
Expand Down
28 changes: 23 additions & 5 deletions src/coreclr/vm/excep.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6971,13 +6971,31 @@ void HandleManagedFault(EXCEPTION_RECORD* pExceptionRecord,
WRAPPER_NO_CONTRACT;

// Ok. Now we have a brand new fault in jitted code.
g_SavedExceptionInfo.Enter();
g_SavedExceptionInfo.SaveExceptionRecord(pExceptionRecord);
g_SavedExceptionInfo.SaveContext(pContext);
if (!Thread::UseContextBasedThreadRedirection())
{
// Once this code path gets enough bake time, perhaps this path could always be used instead of the alternative path to
// redirect the thread
FrameWithCookie<FaultingExceptionFrame> frameWithCookie;
FaultingExceptionFrame *frame = &frameWithCookie;
#if defined(FEATURE_EH_FUNCLETS)
*frame->GetGSCookiePtr() = GetProcessGSCookie();
#endif // FEATURE_EH_FUNCLETS
frame->InitAndLink(pContext);

SEHException exception(pExceptionRecord);
OBJECTREF managedException = CLRException::GetThrowableFromException(&exception);
RaiseTheExceptionInternalOnly(managedException, FALSE);
}
else
{
g_SavedExceptionInfo.Enter();
g_SavedExceptionInfo.SaveExceptionRecord(pExceptionRecord);
g_SavedExceptionInfo.SaveContext(pContext);

SetNakedThrowHelperArgRegistersInContext(pContext);
SetNakedThrowHelperArgRegistersInContext(pContext);

SetIP(pContext, GetEEFuncEntryPoint(NakedThrowHelper));
SetIP(pContext, GetEEFuncEntryPoint(NakedThrowHelper));
}
}

#else // USE_FEF && !TARGET_UNIX
Expand Down
55 changes: 55 additions & 0 deletions src/coreclr/vm/threads.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@
#include "roapi.h"
#endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT

#ifdef FEATURE_SPECIAL_USER_MODE_APC
#include "asmconstants.h"
#endif

static const PortableTailCallFrame g_sentinelTailCallFrame = { NULL, NULL };

TailCallTls::TailCallTls()
Expand Down Expand Up @@ -1641,6 +1645,7 @@ Thread::Thread()

m_currentPrepareCodeConfig = nullptr;
m_isInForbidSuspendForDebuggerRegion = false;
m_hasPendingActivation = false;

#ifdef _DEBUG
memset(dangerousObjRefs, 0, sizeof(dangerousObjRefs));
Expand Down Expand Up @@ -8304,7 +8309,57 @@ BOOL dbgOnly_IsSpecialEEThread()

#endif // _DEBUG

void Thread::StaticInitialize()
{
WRAPPER_NO_CONTRACT;

#ifdef FEATURE_SPECIAL_USER_MODE_APC
InitializeSpecialUserModeApc();

// When CET shadow stacks are enabled, support for special user-mode APCs with the necessary functionality is required
_ASSERTE_ALL_BUILDS(__FILE__, !AreCetShadowStacksEnabled() || UseSpecialUserModeApc());
#endif
}

#ifdef FEATURE_SPECIAL_USER_MODE_APC

QueueUserAPC2Proc Thread::s_pfnQueueUserAPC2Proc;

static void NTAPI EmptyApcCallback(ULONG_PTR Parameter)
{
LIMITED_METHOD_CONTRACT;
}

void Thread::InitializeSpecialUserModeApc()
{
WRAPPER_NO_CONTRACT;
static_assert_no_msg(OFFSETOF__APC_CALLBACK_DATA__ContextRecord == offsetof(CLONE_APC_CALLBACK_DATA, ContextRecord));

HMODULE hKernel32 = WszLoadLibraryEx(WINDOWS_KERNEL32_DLLNAME_W, NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);

// See if QueueUserAPC2 exists
QueueUserAPC2Proc pfnQueueUserAPC2Proc = (QueueUserAPC2Proc)GetProcAddress(hKernel32, "QueueUserAPC2");
if (pfnQueueUserAPC2Proc == nullptr)
{
return;
}

// See if QueueUserAPC2 supports the special user-mode APC with a callback that includes the interrupted CONTEXT. A special
// user-mode APC can interrupt a thread that is in user mode and not in a non-alertable wait.
if (!(*pfnQueueUserAPC2Proc)(EmptyApcCallback, GetCurrentThread(), 0, SpecialUserModeApcWithContextFlags))
{
return;
}

// In the future, once code paths using the special user-mode APC get some bake time, it should be used regardless of
// whether CET shadow stacks are enabled
if (AreCetShadowStacksEnabled())
{
s_pfnQueueUserAPC2Proc = pfnQueueUserAPC2Proc;
}
}

#endif // FEATURE_SPECIAL_USER_MODE_APC
#endif // #ifndef DACCESS_COMPILE

#ifdef DACCESS_COMPILE
Expand Down
Loading

0 comments on commit 663013d

Please sign in to comment.