Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[release/9.0] Fix CET debugger stepping over CALL instructions #108872

Merged
merged 3 commits into from
Oct 15, 2024
Merged

[release/9.0] Fix CET debugger stepping over CALL instructions #108872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5f061af
Support in-place single-stepping over call instructions from out of p…
tommcdon Sep 9, 2024
c45ff96
Fix func-eval abort with minimal code change
tommcdon Oct 11, 2024
73bdeb5
Merge branch 'release/9.0' into dev/tommcdon/fixCetStepping_9.0
jeffschwMSFT Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
312 changes: 290 additions & 22 deletions src/coreclr/debug/di/process.cpp
View file
Open in desktop

Large diffs are not rendered by default.

51 changes: 51 additions & 0 deletions src/coreclr/debug/di/rspriv.h
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading