Skip to content

Commit

Permalink
[release/9.0] Fix CET debugger stepping over CALL instructions (#108872)
Browse files Browse the repository at this point in the history 
* Support in-place single-stepping over call instructions from out of process

* Fix func-eval abort with minimal code change

---------

Co-authored-by: Jeff Schwartz <[email protected]>
  • Loading branch information
@tommcdon @jeffschwMSFT
tommcdon and jeffschwMSFT authored Oct 15, 2024
1 parent dc9d87f commit 066cfa7
Show file tree
Hide file tree
Showing 9 changed files with 505 additions and 57 deletions.
312 changes: 290 additions & 22 deletions src/coreclr/debug/di/process.cpp
View file

Large diffs are not rendered by default.

51 changes: 51 additions & 0 deletions src/coreclr/debug/di/rspriv.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ struct MachineInfo;

#include "eventchannel.h"

#include <shash.h>

#undef ASSERT
#define CRASH(x) _ASSERTE(!(x))
#define ASSERT(x) _ASSERTE(x)
Expand Down Expand Up @@ -2883,6 +2885,10 @@ class IProcessShimHooks
#ifdef FEATURE_INTEROP_DEBUGGING
virtual bool IsUnmanagedThreadHijacked(ICorDebugThread * pICorDebugThread) = 0;
#endif

#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
virtual void HandleDebugEventForInPlaceStepping(const DEBUG_EVENT * pEvent) = 0;
#endif
};


Expand Down Expand Up @@ -2921,6 +2927,42 @@ struct DbgAssertAppDomainDeletedData
VMPTR_AppDomain m_vmAppDomainDeleted;
};

#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
class UnmanagedThreadTracker
{
DWORD m_dwThreadId = (DWORD)-1;
HANDLE m_hThread = INVALID_HANDLE_VALUE;
CORDB_ADDRESS_TYPE *m_pPatchSkipAddress = NULL;
DWORD m_dwSuspendCount = 0;

public:
UnmanagedThreadTracker(DWORD wThreadId, HANDLE hThread) : m_dwThreadId(wThreadId), m_hThread(hThread) {}

DWORD GetThreadId() const { return m_dwThreadId; }
HANDLE GetThreadHandle() const { return m_hThread; }
bool IsInPlaceStepping() const { return m_pPatchSkipAddress != NULL; }
void SetPatchSkipAddress(CORDB_ADDRESS_TYPE *pPatchSkipAddress) { m_pPatchSkipAddress = pPatchSkipAddress; }
CORDB_ADDRESS_TYPE *GetPatchSkipAddress() const { return m_pPatchSkipAddress; }
void ClearPatchSkipAddress() { m_pPatchSkipAddress = NULL; }
void Suspend();
void Resume();
void Close();
};

class EMPTY_BASES_DECL CUnmanagedThreadSHashTraits : public DefaultSHashTraits<UnmanagedThreadTracker*>
{
public:
typedef DWORD key_t;

static key_t GetKey(const element_t &e) { return e->GetThreadId(); }
static BOOL Equals(const key_t &e, const key_t &f) { return e == f; }
static count_t Hash(const key_t &e) { return (count_t)(e ^ (e >> 16) * 0x45D9F43); }
};

typedef SHash<CUnmanagedThreadSHashTraits> CUnmanagedThreadHashTableImpl;
typedef SHash<CUnmanagedThreadSHashTraits>::Iterator CUnmanagedThreadHashTableIterator;
#endif // OUT_OF_PROCESS_SETTHREADCONTEXT

class CordbProcess :
public CordbBase,
public ICorDebugProcess,
Expand Down Expand Up @@ -3286,6 +3328,7 @@ class CordbProcess :

#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
void HandleSetThreadContextNeeded(DWORD dwThreadId);
bool HandleInPlaceSingleStep(DWORD dwThreadId, PVOID pExceptionAddress);
#endif

//
Expand Down Expand Up @@ -4125,6 +4168,14 @@ class CordbProcess :
WriteableMetadataUpdateMode m_writableMetadataUpdateMode;

COM_METHOD GetObjectInternal(CORDB_ADDRESS addr, CordbAppDomain* pAppDomainOverride, ICorDebugObjectValue **pObject);

#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
CUnmanagedThreadHashTableImpl m_unmanagedThreadHashTable;
DWORD m_dwOutOfProcessStepping;
public:
void HandleDebugEventForInPlaceStepping(const DEBUG_EVENT * pEvent);
#endif // OUT_OF_PROCESS_SETTHREADCONTEXT

};

// Some IMDArocess APIs are supported as interop-only.
Expand Down
8 changes: 8 additions & 0 deletions src/coreclr/debug/di/shimprocess.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -789,6 +789,14 @@ HRESULT ShimProcess::HandleWin32DebugEvent(const DEBUG_EVENT * pEvent)
}
}
}
#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
else if (pEvent->dwDebugEventCode == CREATE_THREAD_DEBUG_EVENT ||
pEvent->dwDebugEventCode == EXIT_THREAD_DEBUG_EVENT ||
pEvent->dwDebugEventCode == CREATE_PROCESS_DEBUG_EVENT)
{
m_pProcess->HandleDebugEventForInPlaceStepping(pEvent);
}
#endif

// Do standard event handling, including Handling loader-breakpoint,
// and callback into CordbProcess for Attach if needed.
Expand Down
97 changes: 75 additions & 22 deletions src/coreclr/debug/ee/controller.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2473,7 +2473,12 @@ bool DebuggerController::MatchPatch(Thread *thread,

DebuggerPatchSkip *DebuggerController::ActivatePatchSkip(Thread *thread,
const BYTE *PC,
BOOL fForEnC)
BOOL fForEnC
#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
,
DebuggerSteppingInfo *pDebuggerSteppingInfo
#endif
)
{
#ifdef _DEBUG
BOOL shouldBreak = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_ActivatePatchSkip);
Expand Down Expand Up @@ -2519,6 +2524,14 @@ DebuggerPatchSkip *DebuggerController::ActivatePatchSkip(Thread *thread,
LOG((LF_CORDB,LL_INFO10000, "DC::APS: About to skip from PC=0x%p\n", PC));
skip = new (interopsafe) DebuggerPatchSkip(thread, patch);
TRACE_ALLOC(skip);

#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
if (pDebuggerSteppingInfo != NULL && skip != NULL && skip->IsInPlaceSingleStep())
{
// send the opcode to the right side so it can be restored during single-step
pDebuggerSteppingInfo->EnableInPlaceSingleStepOverCall(patch->opcode);
}
#endif
}

return skip;
Expand Down Expand Up @@ -2830,7 +2843,11 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address,
// DebuggerController's TriggerPatch). Put any DCs that are interested into a queue and then calls
// SendEvent on each.
// Note that control will not return from this function in the case of EnC remap
DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTEXT *context, CORDB_ADDRESS_TYPE *address, SCAN_TRIGGER which)
DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTEXT *context, CORDB_ADDRESS_TYPE *address, SCAN_TRIGGER which
#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
,DebuggerSteppingInfo *pDebuggerSteppingInfo
#endif
)
{
CONTRACT(DPOSS_ACTION)
{
Expand Down Expand Up @@ -3024,7 +3041,11 @@ DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTE
}
#endif

ActivatePatchSkip(thread, dac_cast<PTR_CBYTE>(GetIP(pCtx)), FALSE);
ActivatePatchSkip(thread, dac_cast<PTR_CBYTE>(GetIP(pCtx)), FALSE
#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
, pDebuggerSteppingInfo
#endif
);

lockController.Release();

Expand Down Expand Up @@ -4053,7 +4074,12 @@ void ThisFunctionMayHaveTriggerAGC()
bool DebuggerController::DispatchNativeException(EXCEPTION_RECORD *pException,
CONTEXT *pContext,
DWORD dwCode,
Thread *pCurThread)
Thread *pCurThread
#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
,
DebuggerSteppingInfo *pDebuggerSteppingInfo
#endif
)
{
CONTRACTL
{
Expand Down Expand Up @@ -4224,7 +4250,12 @@ bool DebuggerController::DispatchNativeException(EXCEPTION_RECORD *pException,
result = DebuggerController::DispatchPatchOrSingleStep(pCurThread,
pContext,
ip,
ST_PATCH);
ST_PATCH
#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
,
pDebuggerSteppingInfo
#endif
);
LOG((LF_CORDB, LL_EVERYTHING, "DC::DNE DispatchPatch call returned\n"));

// If we detached, we should remove all our breakpoints. So if we try
Expand All @@ -4241,7 +4272,12 @@ bool DebuggerController::DispatchNativeException(EXCEPTION_RECORD *pException,
result = DebuggerController::DispatchPatchOrSingleStep(pCurThread,
pContext,
ip,
(SCAN_TRIGGER)(ST_PATCH|ST_SINGLE_STEP));
(SCAN_TRIGGER)(ST_PATCH|ST_SINGLE_STEP)
#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
,
pDebuggerSteppingInfo
#endif
);
// We pass patch | single step since single steps actually
// do both (eg, you SS onto a breakpoint).
break;
Expand Down Expand Up @@ -4355,12 +4391,19 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread,

NativeWalker::DecodeInstructionForPatchSkip(patchBypassRX, &(m_instrAttrib));

#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
if (g_pDebugInterface->IsOutOfProcessSetContextEnabled() && m_instrAttrib.m_fIsCall)
{
m_fInPlaceSS = true;
}
#endif

#if defined(TARGET_AMD64)

// The code below handles RIP-relative addressing on AMD64. The original implementation made the assumption that
// we are only using RIP-relative addressing to access read-only data (see VSW 246145 for more information). This
// has since been expanded to handle RIP-relative writes as well.
if (m_instrAttrib.m_dwOffsetToDisp != 0)
if (m_instrAttrib.m_dwOffsetToDisp != 0 && !IsInPlaceSingleStep())
{
_ASSERTE(m_instrAttrib.m_cbInstr != 0);

Expand Down Expand Up @@ -4466,13 +4509,16 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread,
patchBypassRX = NativeWalker::SetupOrSimulateInstructionForPatchSkip(context, m_pSharedPatchBypassBuffer, (const BYTE *)patch->address, patch->opcode);
#endif //TARGET_ARM64

//set eip to point to buffer...
SetIP(context, (PCODE)patchBypassRX);
if (!IsInPlaceSingleStep())
{
//set eip to point to buffer...
SetIP(context, (PCODE)patchBypassRX);

if (context ==(T_CONTEXT*) &c)
thread->SetThreadContext(&c);
if (context ==(T_CONTEXT*) &c)
thread->SetThreadContext(&c);

LOG((LF_CORDB, LL_INFO10000, "DPS::DPS Bypass at %p for opcode 0x%zx \n", patchBypassRX, patch->opcode));
LOG((LF_CORDB, LL_INFO10000, "DPS::DPS Bypass at %p for opcode 0x%zx \n", patchBypassRX, patch->opcode));
}

//
// Turn on single step (if the platform supports it) so we can
Expand Down Expand Up @@ -4691,14 +4737,18 @@ TP_RESULT DebuggerPatchSkip::TriggerExceptionHook(Thread *thread, CONTEXT * cont
{
// Fixup return address on stack
#if defined(TARGET_X86) || defined(TARGET_AMD64)
SIZE_T *sp = (SIZE_T *) GetSP(context);
if (!IsInPlaceSingleStep())
{
// Fixup return address on stack
SIZE_T *sp = (SIZE_T *) GetSP(context);

LOG((LF_CORDB, LL_INFO10000,
"Bypass call return address redirected from %p\n", *sp));
LOG((LF_CORDB, LL_INFO10000,
"Bypass call return address redirected from %p\n", *sp));

*sp -= patchBypass - (BYTE*)m_address;
*sp -= patchBypass - (BYTE*)m_address;

LOG((LF_CORDB, LL_INFO10000, "to %p\n", *sp));
LOG((LF_CORDB, LL_INFO10000, "to %p\n", *sp));
}
#else
PORTABILITY_ASSERT("DebuggerPatchSkip::TriggerExceptionHook -- return address fixup NYI");
#endif
Expand All @@ -4718,10 +4768,13 @@ TP_RESULT DebuggerPatchSkip::TriggerExceptionHook(Thread *thread, CONTEXT * cont
((size_t)GetIP(context) > (size_t)patchBypass &&
(size_t)GetIP(context) <= (size_t)(patchBypass + MAX_INSTRUCTION_LENGTH + 1)))
{
LOG((LF_CORDB, LL_INFO10000, "Bypass instruction redirected because still in skip area.\n"
"\tm_fIsCall = %s, patchBypass = %p, m_address = %p\n",
(m_instrAttrib.m_fIsCall ? "true" : "false"), patchBypass, m_address));
SetIP(context, (PCODE)((BYTE *)GetIP(context) - (patchBypass - (BYTE *)m_address)));
if (!IsInPlaceSingleStep())
{
LOG((LF_CORDB, LL_INFO10000, "Bypass instruction redirected because still in skip area.\n"
"\tm_fIsCall = %s, patchBypass = %p, m_address = %p\n",
(m_instrAttrib.m_fIsCall ? "true" : "false"), patchBypass, m_address));
SetIP(context, (PCODE)((BYTE *)GetIP(context) - (patchBypass - (BYTE *)m_address)));
}
}
else
{
Expand Down Expand Up @@ -4788,7 +4841,7 @@ bool DebuggerPatchSkip::TriggerSingleStep(Thread *thread, const BYTE *ip)
#if defined(TARGET_AMD64)
// Dev11 91932: for RIP-relative writes we need to copy the value that was written in our buffer to the actual address
_ASSERTE(m_pSharedPatchBypassBuffer);
if (m_pSharedPatchBypassBuffer->RipTargetFixup)
if (m_pSharedPatchBypassBuffer->RipTargetFixup && !IsInPlaceSingleStep())
{
_ASSERTE(m_pSharedPatchBypassBuffer->RipTargetFixupSize);

Expand Down
57 changes: 54 additions & 3 deletions src/coreclr/debug/ee/controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1072,7 +1072,12 @@ class DebuggerController
static bool DispatchNativeException(EXCEPTION_RECORD *exception,
CONTEXT *context,
DWORD code,
Thread *thread);
Thread *thread
#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
,
DebuggerSteppingInfo *pDebuggerSteppingInfo = NULL
#endif
);

static bool DispatchUnwind(Thread *thread,
MethodDesc *fd, DebuggerJitInfo * pDJI, SIZE_T offset,
Expand Down Expand Up @@ -1113,13 +1118,23 @@ class DebuggerController

static DebuggerPatchSkip *ActivatePatchSkip(Thread *thread,
const BYTE *eip,
BOOL fForEnC);
BOOL fForEnC
#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
,
DebuggerSteppingInfo *pDebuggerSteppingInfo = NULL
#endif
);


static DPOSS_ACTION DispatchPatchOrSingleStep(Thread *thread,
CONTEXT *context,
CORDB_ADDRESS_TYPE *ip,
SCAN_TRIGGER which);
SCAN_TRIGGER which
#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
,
DebuggerSteppingInfo *pDebuggerSteppingInfo = NULL
#endif
);


static int GetNumberOfPatches()
Expand Down Expand Up @@ -1444,6 +1459,23 @@ class DebuggerController

#if !defined(DACCESS_COMPILE)

// this structure stores useful information about single-stepping over a call instruction
// it is used to communicate the patch skip opcode and current state between the controller on left side and HandleSetThreadContextNeeded on the right side
class DebuggerSteppingInfo
{
bool fIsInPlaceSingleStep = false;
PRD_TYPE opcode = 0;

public:
bool IsInPlaceSingleStep() { return fIsInPlaceSingleStep; }
PRD_TYPE GetOpcode() { return opcode; }
void EnableInPlaceSingleStepOverCall(PRD_TYPE opcode)
{
this->fIsInPlaceSingleStep = true;
this->opcode = opcode;
}
};

/* ------------------------------------------------------------------------- *
* DebuggerPatchSkip routines
* ------------------------------------------------------------------------- */
Expand Down Expand Up @@ -1479,6 +1511,9 @@ class DebuggerPatchSkip : public DebuggerController
CORDB_ADDRESS_TYPE *m_address;
int m_iOrigDisp; // the original displacement of a relative call or jump
InstructionAttribute m_instrAttrib; // info about the instruction being skipped over
#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
bool m_fInPlaceSS; // is this an in-place single-step instruction?
#endif // OUT_OF_PROCESS_SETTHREADCONTEXT
#ifndef FEATURE_EMULATE_SINGLESTEP
// this is shared among all the skippers and the controller. see the comments
// right before the definition of SharedPatchBypassBuffer for lifetime info.
Expand All @@ -1491,7 +1526,23 @@ class DebuggerPatchSkip : public DebuggerController
BYTE* patchBypass = m_pSharedPatchBypassBuffer->PatchBypass;
return (CORDB_ADDRESS_TYPE *)patchBypass;
}

#endif // !FEATURE_EMULATE_SINGLESTEP

BOOL IsInPlaceSingleStep()
{
#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
#ifndef FEATURE_EMULATE_SINGLESTEP
// only in-place single steps over call intructions are supported at this time
_ASSERTE(m_instrAttrib.m_fIsCall);
return m_fInPlaceSS;
#else
#error only non-emulated single-steps with OUT_OF_PROCESS_SETTHREADCONTEXT enabled are supported
#endif
#else
return false;
#endif
}
};

/* ------------------------------------------------------------------------- *
Expand Down
Loading

0 comments on commit 066cfa7

Please sign in to comment.