From 40b08bd37a2961c5d385292051ec20e1b84e6796 Mon Sep 17 00:00:00 2001 From: Max Charlamb Date: Thu, 5 Mar 2026 12:52:00 -0500 Subject: [PATCH 1/3] Fix DefaultCOMImpl reference counting bugs in DAC Fix two bugs in the legacy DAC's DefaultCOMImpl template class: 1. DefaultCOMImpl::Release() used post-decrement (mRef--) instead of pre-decrement (--mRef), causing it to return the old ref count and never delete the object when the count reaches zero. This violates the IUnknown::Release contract and leaks all DefaultCOMImpl-derived objects (DacHandleWalker, DacStackReferenceWalker, DacMemoryEnumerator, DacMethodTableSlotEnumerator, etc.). 2. DacMethodTableSlotEnumerator was returned to callers via raw pointer assignment without calling QueryInterface/AddRef, leaving mRef at 0. All other DefaultCOMImpl subclasses correctly use QueryInterface before returning, which calls AddRef to give the caller an owning reference. Bump SOS_BREAKING_CHANGE_VERSION from 5 to 6. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/debug/daccess/dacimpl.h | 2 +- src/coreclr/debug/daccess/request.cpp | 9 +++++++-- src/coreclr/inc/sospriv.idl | 2 +- src/coreclr/pal/prebuilt/inc/sospriv.h | 2 +- 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/coreclr/debug/daccess/dacimpl.h b/src/coreclr/debug/daccess/dacimpl.h index 4a86846a89d913..cfd973cddcf725 100644 --- a/src/coreclr/debug/daccess/dacimpl.h +++ b/src/coreclr/debug/daccess/dacimpl.h @@ -1574,7 +1574,7 @@ class DefaultCOMImpl : public T ULONG STDMETHODCALLTYPE Release() { - ULONG res = mRef--; + ULONG res = --mRef; if (res == 0) delete this; return res; diff --git a/src/coreclr/debug/daccess/request.cpp b/src/coreclr/debug/daccess/request.cpp index c4af77adc6bfa0..64476850c43755 100644 --- a/src/coreclr/debug/daccess/request.cpp +++ b/src/coreclr/debug/daccess/request.cpp @@ -443,14 +443,19 @@ ClrDataAccess::GetMethodTableSlotEnumerator(CLRDATA_ADDRESS mt, ISOSMethodEnum * else { DacMethodTableSlotEnumerator *methodTableSlotEnumerator = new (nothrow) DacMethodTableSlotEnumerator(); - *enumerator = methodTableSlotEnumerator; - if (*enumerator == NULL) + if (methodTableSlotEnumerator == NULL) { hr = E_OUTOFMEMORY; } else { hr = methodTableSlotEnumerator->Init(mTable); + + if (SUCCEEDED(hr)) + hr = methodTableSlotEnumerator->QueryInterface(__uuidof(ISOSMethodEnum), (void**)enumerator); + + if (FAILED(hr)) + delete methodTableSlotEnumerator; } } diff --git a/src/coreclr/inc/sospriv.idl b/src/coreclr/inc/sospriv.idl index 0820d20386b9e0..13575cd854f502 100644 --- a/src/coreclr/inc/sospriv.idl +++ b/src/coreclr/inc/sospriv.idl @@ -444,7 +444,7 @@ interface ISOSDacInterface8 : IUnknown // Increment anytime there is a change in the data structures that SOS depends on like // stress log structs (StressMsg, StressLogChunck, ThreadStressLog, etc), exception // stack traces (StackTraceElement), the PredefinedTlsSlots enums, etc. -cpp_quote("#define SOS_BREAKING_CHANGE_VERSION 5") +cpp_quote("#define SOS_BREAKING_CHANGE_VERSION 6") [ object, diff --git a/src/coreclr/pal/prebuilt/inc/sospriv.h b/src/coreclr/pal/prebuilt/inc/sospriv.h index 4ff12d95b74894..360d714938b377 100644 --- a/src/coreclr/pal/prebuilt/inc/sospriv.h +++ b/src/coreclr/pal/prebuilt/inc/sospriv.h @@ -2802,7 +2802,7 @@ EXTERN_C const IID IID_ISOSDacInterface8; /* interface __MIDL_itf_sospriv_0000_0012 */ /* [local] */ -#define SOS_BREAKING_CHANGE_VERSION 5 +#define SOS_BREAKING_CHANGE_VERSION 6 extern RPC_IF_HANDLE __MIDL_itf_sospriv_0000_0012_v0_0_c_ifspec; From 40e8babb603cc8af734ff0add9be3d2b4c14f3a5 Mon Sep 17 00:00:00 2001 From: Max Charlamb Date: Thu, 5 Mar 2026 12:58:40 -0500 Subject: [PATCH 2/3] Remove intentional ref count leak from cDAC GetHandleEnum APIs The cDAC's GetHandleEnum and GetHandleEnumForTypes implementations previously called ComInterfaceMarshaller.ConvertToUnmanaged to intentionally leak a ref count, matching a bug in the legacy DAC's DefaultCOMImpl::Release (post-decrement instead of pre-decrement). Now that the legacy DAC bug is fixed, remove the compat workaround so the cDAC returns handle enumerators with the correct ref count. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../SOSDacImpl.cs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs index 936999964d8024..7eaa45a6a9cdc6 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs @@ -1528,9 +1528,6 @@ int ISOSDacInterface.GetHandleEnum(DacComNullableByRef ppHandleE } #endif ppHandleEnum.Interface = new SOSHandleEnum(_target, supportedHandleTypes, legacyHandleEnum); - // COMPAT: In the legacy DAC, this API leaks a ref-count of the returned enumerator. - // Manually leak a refcount here to match previous behavior and avoid breaking customer code. - ComInterfaceMarshaller.ConvertToUnmanaged(ppHandleEnum.Interface); } catch (System.Exception ex) { @@ -1558,9 +1555,6 @@ int ISOSDacInterface.GetHandleEnumForTypes([In, MarshalUsing(CountElementName = IGC gc = _target.Contracts.GC; HandleType[] handleTypes = gc.GetHandleTypes(types); ppHandleEnum.Interface = new SOSHandleEnum(_target, handleTypes, legacyHandleEnum); - // COMPAT: In the legacy DAC, this API leaks a ref-count of the returned enumerator. - // Manually leak a refcount here to match previous behavior and avoid breaking customer code. - ComInterfaceMarshaller.ConvertToUnmanaged(ppHandleEnum.Interface); } catch (System.Exception ex) { From 2e0b30be35771bc9c48030ab71d52c701724c3fd Mon Sep 17 00:00:00 2001 From: Max Charlamb Date: Fri, 6 Mar 2026 11:09:34 -0500 Subject: [PATCH 3/3] Use CLRDATA_REQUEST_REVISION instead of SOS_BREAKING_CHANGE_VERSION MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revert SOS_BREAKING_CHANGE_VERSION back to 5 — per review feedback, it is meant for structural changes (struct layouts, enums), not for ref count fixes. Instead, bump ClrDataAccess::Request CLRDATA_REQUEST_REVISION from 9 to 10. ClrMD queries this via IXCLRDataProcess::Request and can use it to detect the fixed ref counting behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/debug/daccess/request.cpp | 5 ++++- src/coreclr/inc/sospriv.idl | 2 +- src/coreclr/pal/prebuilt/inc/sospriv.h | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/coreclr/debug/daccess/request.cpp b/src/coreclr/debug/daccess/request.cpp index 64476850c43755..3ab622c50ebd23 100644 --- a/src/coreclr/debug/daccess/request.cpp +++ b/src/coreclr/debug/daccess/request.cpp @@ -3974,7 +3974,10 @@ ClrDataAccess::Request(IN ULONG32 reqCode, } else { - *(ULONG32*)outBuffer = 9; + // Revision 10: Fixed DefaultCOMImpl::Release() to use pre-decrement (--mRef). + // Consumers that previously compensated for the broken ref counting (e.g., ClrMD) + // should check this revision to avoid double-freeing. + *(ULONG32*)outBuffer = 10; status = S_OK; } break; diff --git a/src/coreclr/inc/sospriv.idl b/src/coreclr/inc/sospriv.idl index 13575cd854f502..0820d20386b9e0 100644 --- a/src/coreclr/inc/sospriv.idl +++ b/src/coreclr/inc/sospriv.idl @@ -444,7 +444,7 @@ interface ISOSDacInterface8 : IUnknown // Increment anytime there is a change in the data structures that SOS depends on like // stress log structs (StressMsg, StressLogChunck, ThreadStressLog, etc), exception // stack traces (StackTraceElement), the PredefinedTlsSlots enums, etc. -cpp_quote("#define SOS_BREAKING_CHANGE_VERSION 6") +cpp_quote("#define SOS_BREAKING_CHANGE_VERSION 5") [ object, diff --git a/src/coreclr/pal/prebuilt/inc/sospriv.h b/src/coreclr/pal/prebuilt/inc/sospriv.h index 360d714938b377..4ff12d95b74894 100644 --- a/src/coreclr/pal/prebuilt/inc/sospriv.h +++ b/src/coreclr/pal/prebuilt/inc/sospriv.h @@ -2802,7 +2802,7 @@ EXTERN_C const IID IID_ISOSDacInterface8; /* interface __MIDL_itf_sospriv_0000_0012 */ /* [local] */ -#define SOS_BREAKING_CHANGE_VERSION 6 +#define SOS_BREAKING_CHANGE_VERSION 5 extern RPC_IF_HANDLE __MIDL_itf_sospriv_0000_0012_v0_0_c_ifspec;