Skip to content

Fix some Linux (and windows) Dac crashes when given reasonable inputs #124686

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

Closed
leculver wants to merge 5 commits into from
+139 −65
Closed

Fix some Linux (and windows) Dac crashes when given reasonable inputs #124686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
795b8a0
Fix null eeclass issues
leculver Feb 20, 2026
dff13a7
Fix 12 DAC crashes and 4 shadowed HRESULT bugs in request.cpp
leculver Feb 20, 2026
52544a8
Forward-proof RequestGlobal against future MAX_GLOBAL_GC_MECHANISMS_C…
leculver Feb 20, 2026
aacec8e
Code review feedback
leculver Feb 21, 2026
d915be5
Update src/coreclr/debug/daccess/request.cpp
leculver Feb 21, 2026
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
192 changes: 128 additions & 64 deletions src/coreclr/debug/daccess/request.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ BOOL DacValidateEEClass(PTR_EEClass pEEClass)
// PREfix.
retval = FALSE;
}
else if (pEEClass != pMethodTable->GetClass())
else if (pEEClass != pMethodTable->GetClassWithPossibleAV())
Copy link
Member

@jkotas jkotas Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no difference between GetClassWithPossibleAV and GetClass in DAC builds. This change is no-op

Copy link
Contributor Author

@leculver leculver Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I misunderstood that.

Copy link
Contributor Author

@leculver leculver Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted this part back but will wait for your reply on the bigger question below before continuing on this PR.

{
retval = FALSE;
}
Expand Down Expand Up @@ -698,7 +698,7 @@ ClrDataAccess::GetRegisterName(int regNum, unsigned int count, _Inout_updates_z_
return E_UNEXPECTED;

const WCHAR callerPrefix[] = W("caller.");
// Include null terminator in prefixLen/regLen because wcscpy_s will fail otherwise
// Include null terminator in prefixLen/regLen for the 'needed' count
unsigned int prefixLen = (unsigned int)ARRAY_SIZE(callerPrefix);
unsigned int regLen = (unsigned int)u16_strlen(regs[regNum]) + 1;
unsigned int needed = (callerFrame ? prefixLen - 1 : 0) + regLen;
Expand All @@ -707,29 +707,31 @@ ClrDataAccess::GetRegisterName(int regNum, unsigned int count, _Inout_updates_z_

if (buffer)
{
if (count == 0)
return S_FALSE;

WCHAR* curr = buffer;
WCHAR* end = buffer + count;
unsigned int destSize = count;
if (curr < end && callerFrame)
{
unsigned int toCopy = prefixLen < destSize ? prefixLen : destSize;
wcscpy_s(curr, toCopy, callerPrefix);
// Point to null terminator
toCopy--;
unsigned int remaining = count;
if (callerFrame && remaining > 1)
{
unsigned int srcLen = prefixLen - 1; // exclude null
unsigned int toCopy = srcLen < (remaining - 1) ? srcLen : (remaining - 1);
memcpy(curr, callerPrefix, toCopy * sizeof(WCHAR));
curr += toCopy;
destSize -= toCopy;
remaining -= toCopy;
}

if (curr < end)
if (remaining > 1)
{
unsigned int toCopy = regLen < destSize ? regLen : destSize;
wcscpy_s(curr, toCopy, regs[regNum]);
// Point to null terminator
toCopy--;
unsigned int srcLen = regLen - 1; // exclude null
unsigned int toCopy = srcLen < (remaining - 1) ? srcLen : (remaining - 1);
memcpy(curr, regs[regNum], toCopy * sizeof(WCHAR));
curr += toCopy;
destSize -= toCopy;
}

// Always null-terminate
*curr = W('\0');

if (count < needed)
return S_FALSE;
}
Expand Down Expand Up @@ -1707,7 +1709,7 @@ ClrDataAccess::GetMethodDescFromToken(CLRDATA_ADDRESS moduleAddr, mdToken token,
HRESULT
ClrDataAccess::TraverseModuleMap(ModuleMapType mmt, CLRDATA_ADDRESS moduleAddr, MODULEMAPTRAVERSE pCallback, LPVOID token)
{
if (moduleAddr == 0)
if (moduleAddr == 0 || pCallback == NULL)
return E_INVALIDARG;

SOSDacEnter();
Expand Down Expand Up @@ -2057,13 +2059,18 @@ ClrDataAccess::GetMethodTableFieldData(CLRDATA_ADDRESS mt, struct DacpMethodTabl
{
hr = E_INVALIDARG;
}
else if (pMT->GetClass() == NULL)
{
hr = E_INVALIDARG;
}
Comment on lines +2062 to +2065
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null check pMT->GetClass() == NULL is redundant. GetClass() never returns NULL in DAC builds - it calls GetClassWithPossibleAV() which throws DacError(E_UNEXPECTED) if the class pointer is null. The exception is caught by the EX_CATCH block from SOSDacEnter, so the explicit check and hr = E_INVALIDARG assignment will never be reached. Consider removing this check or replacing it with a direct call to GetClassWithPossibleAV() wrapped in a nested EX_TRY if you want to distinguish this error case.

Suggested change
else if (pMT->GetClass() == NULL)
{
hr = E_INVALIDARG;
}

Copilot uses AI. Check for mistakes.
else
{
data->wNumInstanceFields = pMT->GetNumInstanceFields();
data->wNumStaticFields = pMT->GetNumStaticFields();
data->wNumThreadStaticFields = pMT->GetNumThreadStaticFields();
PTR_EEClass pClass = pMT->GetClassWithPossibleAV();
Copy link
Member

@jkotas jkotas Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary change

data->wNumInstanceFields = pClass->GetNumInstanceFields();
data->wNumStaticFields = pClass->GetNumStaticFields();
data->wNumThreadStaticFields = pClass->GetNumThreadStaticFields();

data->FirstField = PTR_TO_TADDR(pMT->GetClass()->GetFieldDescList());
data->FirstField = PTR_TO_TADDR(pClass->GetFieldDescList());

data->wContextStaticsSize = 0;
data->wContextStaticOffset = 0;
Expand Down Expand Up @@ -2620,6 +2627,9 @@ ClrDataAccess::GetFailedAssemblyList(CLRDATA_ADDRESS appDomain, int count,
HRESULT
ClrDataAccess::GetAppDomainName(CLRDATA_ADDRESS addr, unsigned int count, _Inout_updates_z_(count) WCHAR *name, unsigned int *pNeeded)
{
if (addr == 0)
return E_INVALIDARG;

SOSDacEnter();

if (addr == HOST_CDADDR(SystemDomain::System()))
Expand Down Expand Up @@ -3615,7 +3625,7 @@ ClrDataAccess::TraverseLoaderHeap(CLRDATA_ADDRESS loaderHeapAddr, LoaderHeapKind
HRESULT
ClrDataAccess::TraverseVirtCallStubHeap(CLRDATA_ADDRESS pAppDomain, VCSHeapType heaptype, VISITHEAP pFunc)
{
if (pAppDomain == 0)
if (pAppDomain == 0 || pFunc == NULL)
return E_INVALIDARG;

SOSDacEnter();
Expand Down Expand Up @@ -4679,17 +4689,24 @@ HRESULT ClrDataAccess::GetPendingReJITID(CLRDATA_ADDRESS methodDesc, int *pRejit
#ifdef FEATURE_CODE_VERSIONING
PTR_MethodDesc pMD = PTR_MethodDesc(TO_TADDR(methodDesc));

CodeVersionManager* pCodeVersionManager = pMD->GetCodeVersionManager();
CodeVersionManager::LockHolder codeVersioningLockHolder;
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(pMD);
if (ilVersion.IsNull())
if (!DacValidateMD(pMD))
Copy link
Member

@jkotas jkotas Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea that every legacy DAC API that takes a MethodDesc pointer should validate it using DacValidateMD? If it is the case, there is a bunch more to fix.

{
hr = E_INVALIDARG;
}
else if (ilVersion.GetRejitState() == RejitFlags::kStateRequested)
else
{
*pRejitId = (int)ilVersion.GetVersionId();
hr = S_OK;
CodeVersionManager* pCodeVersionManager = pMD->GetCodeVersionManager();
CodeVersionManager::LockHolder codeVersioningLockHolder;
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(pMD);
if (ilVersion.IsNull())
{
hr = E_INVALIDARG;
}
else if (ilVersion.GetRejitState() == RejitFlags::kStateRequested)
{
*pRejitId = (int)ilVersion.GetVersionId();
hr = S_OK;
}
}
#endif // FEATURE_CODE_VERSIONING
SOSDacLeave();
Expand All @@ -4707,6 +4724,12 @@ HRESULT ClrDataAccess::GetReJITInformation(CLRDATA_ADDRESS methodDesc, int rejit
SOSDacEnter();

PTR_MethodDesc pMD = PTR_MethodDesc(TO_TADDR(methodDesc));
if (!DacValidateMD(pMD))
{
hr = E_INVALIDARG;
}
else
{
#ifdef FEATURE_CODE_VERSIONING
CodeVersionManager* pCodeVersionManager = pMD->GetCodeVersionManager();
CodeVersionManager::LockHolder codeVersioningLockHolder;
Expand Down Expand Up @@ -4745,6 +4768,7 @@ HRESULT ClrDataAccess::GetReJITInformation(CLRDATA_ADDRESS methodDesc, int rejit
pReJitData->il = HOST_CDADDR(pMD->GetILHeader());
pReJitData->ilCodeVersionNodePtr = 0;
#endif // FEATURE_CODE_VERSIONING
}

SOSDacLeave();

Expand All @@ -4766,22 +4790,29 @@ HRESULT ClrDataAccess::GetProfilerModifiedILInformation(CLRDATA_ADDRESS methodDe
pILData->il = (CLRDATA_ADDRESS)NULL;
PTR_MethodDesc pMD = PTR_MethodDesc(TO_TADDR(methodDesc));

#ifdef FEATURE_CODE_VERSIONING
CodeVersionManager* pCodeVersionManager = pMD->GetCodeVersionManager();
CodeVersionManager::LockHolder codeVersioningLockHolder;
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(pMD);
if (ilVersion.GetRejitState() != RejitFlags::kStateActive || !ilVersion.HasDefaultIL())
if (!DacValidateMD(pMD))
{
pILData->type = DacpProfilerILData::ReJITModified;
pILData->rejitID = static_cast<ULONG>(pCodeVersionManager->GetActiveILCodeVersion(pMD).GetVersionId());
hr = E_INVALIDARG;
}
else
{
#ifdef FEATURE_CODE_VERSIONING
CodeVersionManager* pCodeVersionManager = pMD->GetCodeVersionManager();
CodeVersionManager::LockHolder codeVersioningLockHolder;
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(pMD);
if (ilVersion.GetRejitState() != RejitFlags::kStateActive || !ilVersion.HasDefaultIL())
{
pILData->type = DacpProfilerILData::ReJITModified;
pILData->rejitID = static_cast<ULONG>(pCodeVersionManager->GetActiveILCodeVersion(pMD).GetVersionId());
}
#endif // FEATURE_CODE_VERSIONING

TADDR pDynamicIL = pMD->GetModule()->GetDynamicIL(pMD->GetMemberDef());
if (pDynamicIL != (TADDR)NULL)
{
pILData->type = DacpProfilerILData::ILModified;
pILData->il = (CLRDATA_ADDRESS)pDynamicIL;
TADDR pDynamicIL = pMD->GetModule()->GetDynamicIL(pMD->GetMemberDef());
if (pDynamicIL != (TADDR)NULL)
{
pILData->type = DacpProfilerILData::ILModified;
pILData->il = (CLRDATA_ADDRESS)pDynamicIL;
}
}

SOSDacLeave();
Expand All @@ -4802,6 +4833,12 @@ HRESULT ClrDataAccess::GetMethodsWithProfilerModifiedIL(CLRDATA_ADDRESS mod, CLR

#ifdef FEATURE_CODE_VERSIONING
PTR_Module pModule = PTR_Module(TO_TADDR(mod));
if (dac_cast<TADDR>(pModule) == (TADDR)-1)
Copy link
Member

@jkotas jkotas Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of special-casing -1 for this API only?

{
hr = E_INVALIDARG;
}
else
{
CodeVersionManager* pCodeVersionManager = pModule->GetCodeVersionManager();
CodeVersionManager::LockHolder codeVersioningLockHolder;

Comment on lines +4841 to 4844
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The else block added in GetMethodsWithProfilerModifiedIL isn’t indented, which makes the control flow hard to follow and increases the risk of future edits accidentally changing which code is guarded by the pointer validation. Please reformat/indent the contents of the else block (and its closing brace) to match the file’s existing style.

Copilot uses AI. Check for mistakes.
Expand Down Expand Up @@ -4835,6 +4872,7 @@ HRESULT ClrDataAccess::GetMethodsWithProfilerModifiedIL(CLRDATA_ADDRESS mod, CLR
}
}
}
}
#endif // FEATURE_CODE_VERSIONING
SOSDacLeave();

Expand Down Expand Up @@ -4865,7 +4903,6 @@ HRESULT ClrDataAccess::GetGenerationTable(unsigned int cGenerations, struct Dacp

SOSDacEnter();

HRESULT hr = S_OK;
unsigned int numGenerationTableEntries = (unsigned int)(g_gcDacGlobals->total_generation_count);
if (pNeeded != NULL)
{
Expand Down Expand Up @@ -4911,7 +4948,6 @@ HRESULT ClrDataAccess::GetFinalizationFillPointers(unsigned int cFillPointers, C

SOSDacEnter();

HRESULT hr = S_OK;
unsigned int numFillPointers = (unsigned int)(g_gcDacGlobals->total_generation_count + dac_finalize_queue::ExtraSegCount);
if (pNeeded != NULL)
{
Expand Down Expand Up @@ -4952,7 +4988,6 @@ HRESULT ClrDataAccess::GetGenerationTableSvr(CLRDATA_ADDRESS heapAddr, unsigned

SOSDacEnter();

HRESULT hr = S_OK;
#ifdef FEATURE_SVR_GC
unsigned int numGenerationTableEntries = (unsigned int)(g_gcDacGlobals->total_generation_count);
if (pNeeded != NULL)
Expand Down Expand Up @@ -5002,7 +5037,6 @@ HRESULT ClrDataAccess::GetFinalizationFillPointersSvr(CLRDATA_ADDRESS heapAddr,

SOSDacEnter();

HRESULT hr = S_OK;
#ifdef FEATURE_SVR_GC
unsigned int numFillPointers = (unsigned int)(g_gcDacGlobals->total_generation_count + dac_finalize_queue::ExtraSegCount);
if (pNeeded != NULL)
Expand All @@ -5019,13 +5053,32 @@ HRESULT ClrDataAccess::GetFinalizationFillPointersSvr(CLRDATA_ADDRESS heapAddr,
TADDR heapAddress = TO_TADDR(heapAddr);
if (heapAddress != 0)
{
dac_gc_heap heap = LoadGcHeapData(heapAddress);
dac_gc_heap* pHeap = &heap;
DPTR(dac_finalize_queue) fq = pHeap->finalize_queue;
DPTR(uint8_t*) pFillPointerArray= dac_cast<TADDR>(fq) + offsetof(dac_finalize_queue, m_FillPointers);
for (unsigned int i = 0; i < numFillPointers; ++i)
// Validate that heapAddr is a known server GC heap address
bool validHeap = false;
int heapCount = GCHeapCount();
for (int i = 0; i < heapCount; i++)
{
if (heapAddress == HeapTableIndex(g_gcDacGlobals->g_heaps, i))
{
validHeap = true;
break;
}
}

if (!validHeap)
{
pFinalizationFillPointers[i] = (CLRDATA_ADDRESS) pFillPointerArray[i];
hr = E_INVALIDARG;
}
else
{
dac_gc_heap heap = LoadGcHeapData(heapAddress);
dac_gc_heap* pHeap = &heap;
DPTR(dac_finalize_queue) fq = pHeap->finalize_queue;
DPTR(uint8_t*) pFillPointerArray = dac_cast<TADDR>(fq) + offsetof(dac_finalize_queue, m_FillPointers);
for (unsigned int i = 0; i < numFillPointers; ++i)
{
pFinalizationFillPointers[i] = (CLRDATA_ADDRESS) pFillPointerArray[i];
}
}
}
else
Expand All @@ -5046,22 +5099,32 @@ HRESULT ClrDataAccess::GetAssemblyLoadContext(CLRDATA_ADDRESS methodTable, CLRDA
if (methodTable == 0 || assemblyLoadContext == NULL)
return E_INVALIDARG;

*assemblyLoadContext = 0;

SOSDacEnter();
PTR_MethodTable pMT = PTR_MethodTable(CLRDATA_ADDRESS_TO_TADDR(methodTable));
PTR_Module pModule = pMT->GetModule();
BOOL bIsFree;
if (!DacValidateMethodTable(pMT, bIsFree))
{
hr = E_INVALIDARG;
}
else
{
PTR_Module pModule = pMT->GetModule();

PTR_PEAssembly pPEAssembly = pModule->GetPEAssembly();
PTR_AssemblyBinder pBinder = pPEAssembly->GetAssemblyBinder();
PTR_PEAssembly pPEAssembly = pModule->GetPEAssembly();
PTR_AssemblyBinder pBinder = pPEAssembly->GetAssemblyBinder();

INT_PTR AssemblyLoadContextHandle = pBinder->GetAssemblyLoadContext();
INT_PTR AssemblyLoadContextHandle = pBinder->GetAssemblyLoadContext();

TADDR AssemblyLoadContextAddr = 0;
if (AssemblyLoadContextHandle != 0)
{
DacReadAll(AssemblyLoadContextHandle,&AssemblyLoadContextAddr,sizeof(TADDR),true);
}
TADDR AssemblyLoadContextAddr = 0;
if (AssemblyLoadContextHandle != 0)
{
DacReadAll(AssemblyLoadContextHandle,&AssemblyLoadContextAddr,sizeof(TADDR),true);
}

*assemblyLoadContext = TO_CDADDR(AssemblyLoadContextAddr);
*assemblyLoadContext = TO_CDADDR(AssemblyLoadContextAddr);
}

SOSDacLeave();
return hr;
Expand Down Expand Up @@ -5152,7 +5215,7 @@ HRESULT ClrDataAccess::GetObjectComWrappersData(CLRDATA_ADDRESS objAddr, CLRDATA
SOSDacEnter();

// Default to having found no information.
HRESULT hr = S_FALSE;
hr = S_FALSE;
Copy link
Member

@noahfalk noahfalk Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, I just discovered the same issue in my own testing. If you are fixing these shadowing bugs mind getting all of them at once? I believe there are three more in GetHandleEnumForTypes, GetHandleEnumForGC, and GetObjectStringData.

Copy link
Contributor Author

@leculver leculver Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I missed those. Did you see them actually happen in practice, or just code review? I will add those to the PR, assuming this is one we will merge. I'll hold off on work until I know the ground rules.

Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In GetObjectComWrappersData, now that the inner HRESULT hr shadowing was removed, the unconditional hr = S_FALSE; near the end of the method will force this API to always return S_FALSE even when rcw/mowList were successfully populated. Please remove that final overwrite (or gate it behind the “no data found” path) so callers can rely on S_OK when data is returned.

Suggested change
hr = S_FALSE;

Copilot uses AI. Check for mistakes.

if (pNeeded != NULL)
{
Expand Down Expand Up @@ -5655,7 +5718,8 @@ HRESULT STDMETHODCALLTYPE ClrDataAccess::GetThreadStaticBaseAddress(CLRDATA_ADDR
}
else
{
if (mTable->GetClass()->GetNumThreadStaticFields() == 0)
PTR_EEClass pClass = mTable->GetClassWithPossibleAV();
if (pClass->GetNumThreadStaticFields() == 0)
{
Comment on lines +5722 to 5723
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetThreadStaticBaseAddress dereferences pClass without checking for null. MethodTable::GetClassWithPossibleAV() can return NULL (e.g., if m_pEEClass is null), which would still crash here. Please add a null check and return an error HRESULT (or S_FALSE) instead of dereferencing.

Suggested change
if (pClass->GetNumThreadStaticFields() == 0)
{
if (pClass == NULL)
{
hr = E_FAIL;
}
else if (pClass->GetNumThreadStaticFields() == 0)
{

Copilot uses AI. Check for mistakes.
if (GCStaticsAddress != NULL)
{
Expand Down
12 changes: 11 additions & 1 deletion src/coreclr/inc/dacprivate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,7 +857,17 @@ struct MSLAYOUT DacpGCInterestingInfoData
ISOSDacInterface3 *psos3 = NULL;
if (SUCCEEDED(hr = sos->QueryInterface(__uuidof(ISOSDacInterface3), (void**) &psos3)))
{
hr = psos3->GetGCGlobalMechanisms(globalMechanisms);
// GetGCGlobalMechanisms writes MAX_GLOBAL_GC_MECHANISMS_COUNT entries (currently 6)
Copy link
Member

@noahfalk noahfalk Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better fix would be to enforce in the DAC that it never writes more than 6 entries. If MAX_GLOBAL_GC_MECHANISMS_COUNT is greater than 6 the API should return a failing HRESULT. If we ever need to support more than 6 we can make an API that properly handles dynamic sizing.

Also heads up this is dead code in the runtime repo as far as I know. The header is used by some other libraries but I don't know of a code path that invokes this method. There is another copy of this header in the diagnostics repo where SOS is built and that one calls this code.

// into the provided buffer with no count parameter. Over-allocate so that a newer
// runtime writing more entries won't overflow our buffer. We only read back
// DAC_MAX_GLOBAL_GC_MECHANISMS_COUNT elements into globalMechanisms.
size_t buffer[256] = {};
hr = psos3->GetGCGlobalMechanisms(buffer);
if (SUCCEEDED(hr))
{
for (int i = 0; i < DAC_MAX_GLOBAL_GC_MECHANISMS_COUNT; i++)
globalMechanisms[i] = buffer[i];
}
psos3->Release();
}
return hr;
Expand Down
Loading