Skip to content

Commit

Permalink
Fix x64 macOS and Linux managed debugging (#84655)
Browse files Browse the repository at this point in the history
* Limit context size in DAC stackwalker

* Constrain debugger x64 context size check to Linux and macOS

* Exclude DAC and DBI cross-os compilation with context adjustment

* Fix non-x64 builds

---------

Co-authored-by: Mike McLaughlin <mikem@microsoft.com>
  • Loading branch information
tommcdon and mikem8361 authored Apr 15, 2023
1 parent a2c095e commit 49af07c
Show file tree
Hide file tree
Showing 9 changed files with 64 additions and 21 deletions.
3 changes: 2 additions & 1 deletion src/coreclr/debug/createdump/datatarget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

#include "createdump.h"
#include <dbgtargetcontext.h>

#if defined(HOST_ARM64)
// Flag to check if atomics feature is available on
Expand Down Expand Up @@ -181,7 +182,7 @@ DumpDataTarget::GetThreadContext(
/* [in] */ ULONG32 contextSize,
/* [out, size_is(contextSize)] */ PBYTE context)
{
if (contextSize < sizeof(CONTEXT))
if (contextSize < sizeof(DT_CONTEXT))
{
assert(false);
return E_INVALIDARG;
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/debug/daccess/daccess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7896,7 +7896,7 @@ void DacStackReferenceWalker::WalkStack()
// Get the current thread's context and set that as the filter context
if (mThread->GetFilterContext() == NULL && mThread->GetProfilerFilterContext() == NULL)
{
mDac->m_pTarget->GetThreadContext(mThread->GetOSThreadId(), CONTEXT_FULL, sizeof(ctx), (BYTE*)&ctx);
mDac->m_pTarget->GetThreadContext(mThread->GetOSThreadId(), CONTEXT_FULL, sizeof(DT_CONTEXT), (BYTE*)&ctx);
mThread->SetProfilerFilterContext(&ctx);
contextHolder.Activate(mThread);
}
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/debug/daccess/dacdbiimpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5199,7 +5199,7 @@ void DacDbiInterfaceImpl::Hijack(
HRESULT hr = m_pTarget->GetThreadContext(
dwThreadId,
CONTEXT_FULL,
sizeof(ctx),
sizeof(DT_CONTEXT),
(BYTE*) &ctx);
IfFailThrow(hr);

Expand Down Expand Up @@ -5346,7 +5346,7 @@ void DacDbiInterfaceImpl::Hijack(
//
// Commit the context.
//
hr = m_pMutableTarget->SetThreadContext(dwThreadId, sizeof(ctx), reinterpret_cast<BYTE*> (&ctx));
hr = m_pMutableTarget->SetThreadContext(dwThreadId, sizeof(DT_CONTEXT), reinterpret_cast<BYTE*> (&ctx));
IfFailThrow(hr);
}

Expand Down Expand Up @@ -5717,7 +5717,7 @@ void DacDbiInterfaceImpl::GetContext(VMPTR_Thread vmThread, DT_CONTEXT * pContex
pContextBuffer->ContextFlags = DT_CONTEXT_ALL;
HRESULT hr = m_pTarget->GetThreadContext(pThread->GetOSThreadId(),
pContextBuffer->ContextFlags,
sizeof(*pContextBuffer),
sizeof(DT_CONTEXT),
reinterpret_cast<BYTE *>(pContextBuffer));
if (hr == E_NOTIMPL)
{
Expand Down
17 changes: 13 additions & 4 deletions src/coreclr/debug/daccess/dacdbiimplstackwalk.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,14 @@ void DacDbiInterfaceImpl::GetStackWalkCurrentContext(StackWalkHandle pSFIHandle,
void DacDbiInterfaceImpl::GetStackWalkCurrentContext(StackFrameIterator * pIter,
DT_CONTEXT * pContext)
{
// convert the current REGDISPLAY to a CONTEXT
// convert the current REGDISPLAY to a DT_CONTEXT
CrawlFrame * pCF = &(pIter->m_crawl);
UpdateContextFromRegDisp(pCF->GetRegisterSet(), reinterpret_cast<T_CONTEXT *>(pContext));
T_CONTEXT tmpContext = { };
UpdateContextFromRegDisp(pCF->GetRegisterSet(), &tmpContext);
CopyMemory(pContext, &tmpContext, sizeof(*pContext));
#if defined(TARGET_X86) || defined(TARGET_AMD64)
pContext->ContextFlags &= ~(CONTEXT_XSTATE & CONTEXT_AREA_MASK);
#endif
}


Expand All @@ -180,7 +185,7 @@ void DacDbiInterfaceImpl::SetStackWalkCurrentContext(VMPTR_Thread vmTh
// Allocate a context in DDImpl's memory space. DDImpl can't contain raw pointers back into
// the client space since that may not marshal.
T_CONTEXT * pContext2 = GetContextBufferFromHandle(pSFIHandle);
*pContext2 = *reinterpret_cast<T_CONTEXT *>(pContext); // memcpy
CopyMemory(pContext2, pContext, sizeof(*pContext));

// update the REGDISPLAY with the given CONTEXT.
// Be sure that the context is in DDImpl's memory space and not the Right-sides.
Expand Down Expand Up @@ -657,8 +662,12 @@ void DacDbiInterfaceImpl::ConvertContextToDebuggerRegDisplay(const DT_CONTEXT *

// This is a bit cumbersome. First we need to convert the CONTEXT into a REGDISPLAY. Then we need
// to convert the REGDISPLAY to a DebuggerREGDISPLAY.
T_CONTEXT tmpContext = { };
CopyMemory(&tmpContext, pInContext, sizeof(*pInContext));

REGDISPLAY rd;
FillRegDisplay(&rd, reinterpret_cast<T_CONTEXT *>(const_cast<DT_CONTEXT *>(pInContext)));
FillRegDisplay(&rd, &tmpContext);

SetDebuggerREGDISPLAYFromREGDISPLAY(pOutDRD, &rd);
}

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/debug/daccess/reimpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ DacGetThreadContext(Thread* thread, T_CONTEXT* context)
HRESULT status =
g_dacImpl->m_pTarget->
GetThreadContext(thread->GetOSThreadId(), contextFlags,
sizeof(*context), (PBYTE)context);
sizeof(DT_CONTEXT), (PBYTE)context);
if (status != S_OK)
{
DacError(status);
Expand Down
9 changes: 5 additions & 4 deletions src/coreclr/debug/daccess/stack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,9 @@ ClrDataStackWalk::GetContext(
}
else
{
*(PT_CONTEXT)contextBuf = m_context;
UpdateContextFromRegDisp(&m_regDisp, (PT_CONTEXT)contextBuf);
T_CONTEXT tmpContext = m_context;
UpdateContextFromRegDisp(&m_regDisp, &tmpContext);
CopyMemory(contextBuf, &tmpContext, contextBufSize);
status = S_OK;
}
}
Expand Down Expand Up @@ -154,7 +155,7 @@ ClrDataStackWalk::SetContext2(
{
// Copy the context to local state so
// that its lifetime extends beyond this call.
m_context = *(PT_CONTEXT)context;
CopyMemory(&m_context, context, contextSize);
m_thread->FillRegDisplay(&m_regDisp, &m_context);
m_frameIter.ResetRegDisp(&m_regDisp, (flags & CLRDATA_STACK_SET_CURRENT_CONTEXT) != 0);
m_stackPrev = (TADDR)GetRegdisplaySP(&m_regDisp);
Expand Down Expand Up @@ -660,7 +661,7 @@ ClrDataFrame::GetContext(

EX_TRY
{
*(PT_CONTEXT)contextBuf = m_context;
CopyMemory(contextBuf, &m_context, contextBufSize);
status = S_OK;
}
EX_CATCH
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/debug/di/process.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6438,7 +6438,7 @@ HRESULT CordbProcess::GetThreadContext(DWORD threadID, ULONG32 contextSize, BYTE

DT_CONTEXT * pContext;

if (contextSize != sizeof(DT_CONTEXT))
if (contextSize < sizeof(DT_CONTEXT))
{
LOG((LF_CORDB, LL_INFO10000, "CP::GTC: thread=0x%x, context size is invalid.\n", threadID));
return E_INVALIDARG;
Expand Down Expand Up @@ -11179,10 +11179,10 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId)
ThrowHR(HRESULT_FROM_GetLastError());
}

CONTEXT context = { 0 };
DT_CONTEXT context = { 0 };
context.ContextFlags = CONTEXT_FULL;

HRESULT hr = GetDataTarget()->GetThreadContext(dwThreadId, CONTEXT_FULL, sizeof(CONTEXT), reinterpret_cast<BYTE*> (&context));
HRESULT hr = GetDataTarget()->GetThreadContext(dwThreadId, CONTEXT_FULL, sizeof(DT_CONTEXT), reinterpret_cast<BYTE*> (&context));
IfFailThrow(hr);

TADDR lsContextAddr = (TADDR)context.Rcx;
Expand Down
8 changes: 8 additions & 0 deletions src/coreclr/debug/inc/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,15 @@ ULONG32 ContextSizeForFlags(ULONG32 flags)
else
#endif // TARGET_X86
{
#if !defined(CROSS_COMPILE) && !defined(TARGET_WINDOWS) && defined(TARGET_AMD64)
if ((flags & CONTEXT_XSTATE) == CONTEXT_XSTATE)
{
return sizeof(T_CONTEXT);
}
return offsetof(T_CONTEXT, XStateFeaturesMask);
#else
return sizeof(T_CONTEXT);
#endif
}
}

Expand Down
32 changes: 28 additions & 4 deletions src/coreclr/debug/inc/dbgtargetcontext.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,15 @@
// ByteSwapContext.
//

//
// **** NOTE: Keep these in sync with pal/inc/pal.h ****
//
// ****
// **** NOTE: T_CONTEXT (in pal/inc/pal.h) can now be larger than DT_CONTEXT (currently T_CONTEXT on Linux/MacOS
// **** x64 includes the XSTATE registers). This means the following:
// ****
// **** 1) The DBI/DAC APIs cannot assume that incoming context buffers are T_CONTEXT sized.
// **** 2) When the DAC calls the supplied data target's context APIs, the size of the context buffer must
// **** be the size of the DT_CONTEXT for compatiblity.
// **** 3) DBI/DAC code can not cast and copy from a T_CONTEXT into a DT_CONTEXT buffer.
// ****

// This odd define pattern is needed because in DBI we set _TARGET_ to match the host and
// DBG_TARGET to control our targeting. In x-plat DBI DBG_TARGET won't match _TARGET_ and
Expand All @@ -54,6 +60,8 @@
#define DTCONTEXT_IS_RISCV64
#endif

#define CONTEXT_AREA_MASK 0xffff

#if defined(DTCONTEXT_IS_X86)

#define DT_SIZE_OF_80387_REGISTERS 80
Expand Down Expand Up @@ -118,6 +126,8 @@ typedef struct {

} DT_CONTEXT;

static_assert(sizeof(DT_CONTEXT) == sizeof(T_CONTEXT), "DT_CONTEXT size must equal the T_CONTEXT size on X86");

// Since the target is little endian in this case we only have to provide a real implementation of
// ByteSwapContext if the platform we're building on is big-endian.
#ifdef BIGENDIAN
Expand Down Expand Up @@ -280,6 +290,12 @@ typedef struct DECLSPEC_ALIGN(16) {
DWORD64 LastExceptionFromRip;
} DT_CONTEXT;

#if !defined(CROSS_COMPILE) && !defined(TARGET_WINDOWS)
static_assert(sizeof(DT_CONTEXT) == offsetof(T_CONTEXT, XStateFeaturesMask), "DT_CONTEXT must not include the XSTATE registers on AMD64");
#else
static_assert(sizeof(DT_CONTEXT) == sizeof(T_CONTEXT), "DT_CONTEXT size must equal the T_CONTEXT size on AMD64");
#endif

#elif defined(DTCONTEXT_IS_ARM)

#define DT_CONTEXT_ARM 0x00200000L
Expand Down Expand Up @@ -361,6 +377,8 @@ typedef DECLSPEC_ALIGN(8) struct {

} DT_CONTEXT;

static_assert(sizeof(DT_CONTEXT) == sizeof(T_CONTEXT), "DT_CONTEXT size must equal the T_CONTEXT size on ARM32");

#elif defined(DTCONTEXT_IS_ARM64)

#define DT_CONTEXT_ARM64 0x00400000L
Expand Down Expand Up @@ -452,7 +470,10 @@ typedef DECLSPEC_ALIGN(16) struct {

} DT_CONTEXT;

static_assert(sizeof(DT_CONTEXT) == sizeof(T_CONTEXT), "DT_CONTEXT size must equal the T_CONTEXT size on ARM64");

#elif defined(DTCONTEXT_IS_LOONGARCH64)

#define DT_CONTEXT_LOONGARCH64 0x00800000L

#define DT_CONTEXT_CONTROL (DT_CONTEXT_LOONGARCH64 | 0x1L)
Expand Down Expand Up @@ -516,7 +537,10 @@ typedef DECLSPEC_ALIGN(16) struct {
ULONGLONG F[32];
} DT_CONTEXT;

static_assert(sizeof(DT_CONTEXT) == sizeof(T_CONTEXT), "DT_CONTEXT size must equal the T_CONTEXT size");

#elif defined(DTCONTEXT_IS_RISCV64)

#define DT_CONTEXT_RISCV64 0x01000000L

#define DT_CONTEXT_CONTROL (DT_CONTEXT_RISCV64 | 0x1L)
Expand Down Expand Up @@ -580,10 +604,10 @@ typedef DECLSPEC_ALIGN(16) struct {
ULONGLONG F[32];
} DT_CONTEXT;

static_assert(sizeof(DT_CONTEXT) == sizeof(T_CONTEXT), "DT_CONTEXT size must equal the T_CONTEXT size");

#else
#error Unsupported platform
#endif


#endif // __DBG_TARGET_CONTEXT_INCLUDED

0 comments on commit 49af07c

Please sign in to comment.