From 76ab0ab31e64407f686a25b92f219cd293493ecc Mon Sep 17 00:00:00 2001 From: Lee Culver Date: Tue, 19 Sep 2023 16:14:04 -0700 Subject: [PATCH 1/4] ICorDebug: Properly handle interior pointers There is an ancient bug in the dac root walking code. If we hit an interior pointer on a callstack that lives outside of the GC heap, we will report that pointer as-is to ICorDebug (specifically `CordbRefEnum::Next`). This is despite the fact that ICorDebug *specifically* requests that the dac only enumerate pointers to real objects. This non-gc pointer will be treated as a real object pointer and will cause a failed HRESULT in CordbRefEnum. Since this call is wrapped with `IfFailThrow` it will halt all further processing of roots. This code makes a small, targeted change to not enumerate non-GC pointers when the caller requests we specifically only enumerate pointers to real objects. This is not a recent regression, but rather a bug that has likely existed since the original code was written. --- src/coreclr/debug/daccess/daccess.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/coreclr/debug/daccess/daccess.cpp b/src/coreclr/debug/daccess/daccess.cpp index 2ff45fd348e30a..5793c63b9b8908 100644 --- a/src/coreclr/debug/daccess/daccess.cpp +++ b/src/coreclr/debug/daccess/daccess.cpp @@ -7978,10 +7978,15 @@ void DacStackReferenceWalker::GCEnumCallback(LPVOID hCallback, OBJECTREF *pObjec { CORDB_ADDRESS fixed_obj = 0; HRESULT hr = dsc->pWalker->mHeap.ListNearObjects((CORDB_ADDRESS)obj, NULL, &fixed_obj, NULL); + + // Interior pointers need not lie on the manage heap at all. When this happens, ListNearObjects + // will return E_FAIL. In this case, we need to be sure to not include this stack slot in our + // enumeration because ICorDebug expects every location enumerated by this API to point to a + // valid object. + if (FAILED(hr)) + return; - // If we failed...oh well, SOS won't mind. We'll just report the interior pointer as is. - if (SUCCEEDED(hr)) - obj = TO_TADDR(fixed_obj); + obj = TO_TADDR(fixed_obj); } // Report the object and where it was found. From a399c5d0271cf38b81d158111c12b2541ac09582 Mon Sep 17 00:00:00 2001 From: Lee Culver Date: Wed, 20 Sep 2023 10:25:01 -0700 Subject: [PATCH 2/4] Fix issue with enregistered values --- src/coreclr/debug/daccess/dacdbiimpl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/debug/daccess/dacdbiimpl.cpp b/src/coreclr/debug/daccess/dacdbiimpl.cpp index 5ab0e1fb74be4f..61dc5589dcba78 100644 --- a/src/coreclr/debug/daccess/dacdbiimpl.cpp +++ b/src/coreclr/debug/daccess/dacdbiimpl.cpp @@ -7781,7 +7781,7 @@ HRESULT DacStackReferenceWalker::Next(ULONG count, DacGcReference stackRefs[], U stackRefs[i].i64ExtraData = 0; const SOSStackRefData &sosStackRef = mList.Get(i); - if (sosStackRef.Flags & GC_CALL_INTERIOR) + if (sosStackRef.Address == 0 || sosStackRef.Flags & GC_CALL_INTERIOR) { stackRefs[i].pObject = CLRDATA_ADDRESS_TO_TADDR(sosStackRef.Object) | 1; } From da1d424d9ca60594aa64bdfc17eeccff97c63f78 Mon Sep 17 00:00:00 2001 From: Juan Sebastian Hoyos Ayala Date: Wed, 20 Sep 2023 10:36:25 -0700 Subject: [PATCH 3/4] Fix assert and enregistered ref reporting --- src/coreclr/debug/daccess/dacdbiimpl.cpp | 3 ++- src/coreclr/debug/di/rsclass.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/debug/daccess/dacdbiimpl.cpp b/src/coreclr/debug/daccess/dacdbiimpl.cpp index 61dc5589dcba78..ba46fa0548e172 100644 --- a/src/coreclr/debug/daccess/dacdbiimpl.cpp +++ b/src/coreclr/debug/daccess/dacdbiimpl.cpp @@ -7781,8 +7781,9 @@ HRESULT DacStackReferenceWalker::Next(ULONG count, DacGcReference stackRefs[], U stackRefs[i].i64ExtraData = 0; const SOSStackRefData &sosStackRef = mList.Get(i); - if (sosStackRef.Address == 0 || sosStackRef.Flags & GC_CALL_INTERIOR) + if (sosStackRef.Flags & GC_CALL_INTERIOR | sosStackRef.Address == 0) { + // Direct pointer case - interior pointer, Frame ref, or enregistered var. stackRefs[i].pObject = CLRDATA_ADDRESS_TO_TADDR(sosStackRef.Object) | 1; } else diff --git a/src/coreclr/debug/di/rsclass.cpp b/src/coreclr/debug/di/rsclass.cpp index ec52823c07af5f..55f83b48a6d211 100644 --- a/src/coreclr/debug/di/rsclass.cpp +++ b/src/coreclr/debug/di/rsclass.cpp @@ -132,6 +132,7 @@ HRESULT CordbClass::GetStaticFieldValue(mdFieldDef fieldDef, IMetaDataImport * pImport = NULL; EX_TRY { + RSLockHolder lockHolder(GetProcess()->GetProcessLock()); pImport = GetModule()->GetMetaDataImporter(); // throws // Validate the token. @@ -1191,4 +1192,3 @@ HRESULT CordbClass::SearchFieldInfo( // Well, the field doesn't even belong to this class... ThrowHR(E_INVALIDARG); } - From 79ff97020200846e637a35bde07ccda6c5df97a8 Mon Sep 17 00:00:00 2001 From: Juan Hoyos <19413848+hoyosjs@users.noreply.github.com> Date: Wed, 20 Sep 2023 10:49:02 -0700 Subject: [PATCH 4/4] Fix issue from merge --- src/coreclr/debug/daccess/dacdbiimpl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/debug/daccess/dacdbiimpl.cpp b/src/coreclr/debug/daccess/dacdbiimpl.cpp index ba46fa0548e172..759c6998bb436f 100644 --- a/src/coreclr/debug/daccess/dacdbiimpl.cpp +++ b/src/coreclr/debug/daccess/dacdbiimpl.cpp @@ -7781,7 +7781,7 @@ HRESULT DacStackReferenceWalker::Next(ULONG count, DacGcReference stackRefs[], U stackRefs[i].i64ExtraData = 0; const SOSStackRefData &sosStackRef = mList.Get(i); - if (sosStackRef.Flags & GC_CALL_INTERIOR | sosStackRef.Address == 0) + if (sosStackRef.Flags & GC_CALL_INTERIOR || sosStackRef.Address == 0) { // Direct pointer case - interior pointer, Frame ref, or enregistered var. stackRefs[i].pObject = CLRDATA_ADDRESS_TO_TADDR(sosStackRef.Object) | 1;