Fix COM ref counting in SOSMethodEnum#5749
Conversation
- Fix SOSMethodEnum simulator to use standard COM ref counting: initialize refCount to 0 and use QueryInterface to return the interface pointer with a proper AddRef. - Fix ISOSMethodEnum leak in DumpMT by using ToRelease<>. Matching PR: dotnet/runtime#125231 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes two COM ref-counting bugs in the SOSDacInterface15 simulator, matching a parallel fix in the .NET runtime (dotnet/runtime#125231). The SOSMethodEnum class previously initialized its reference count to 1 (causing a count mismatch) and assigned the raw pointer directly to the output parameter without going through QueryInterface/AddRef. The failure-path cleanup also incorrectly called Release() when refCount was 1 (which would delete the object before returning the pointer), and in the strike.cpp call site the enumerator was never released.
Changes:
util.cpp: FixSOSMethodEnumto initializerefCountto 0, useQueryInterfaceto properlyAddRefon success, and usedeletedirectly on the failure path (sinceQueryInterfacewas never called,refCountis still 0 andRelease()would unsigned-underflow).strike.cpp: WrapISOSMethodEnum*inToRelease<>to ensureRelease()is called on scope exit, fixing a resource leak.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/SOS/Strike/util.cpp |
Fixes COM ref-counting in SOSMethodEnum: initializes refCount to 0, routes output through QueryInterface, and uses delete in error cleanup. |
src/SOS/Strike/strike.cpp |
Wraps ISOSMethodEnum* in ToRelease<> to prevent resource leak. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
max-charlamb
changed the title
| table.WriteRow("Entry", "MethodDesc", "JIT", "Slot", "Name"); | ||
|
|
||
| ISOSMethodEnum *pMethodEnumerator; | ||
| ToRelease<ISOSMethodEnum> pMethodEnumerator; |
There was a problem hiding this comment.
What's going to happen when latest SOS with this change runs against older DAC? Is it going to just leak (that's ok) or crash (that's bad)?
There was a problem hiding this comment.
This is unrelated to the changes in dotnet/runtime#125231 . This was a standard leak where the COM interface was never freed. It should not cause a problem with either DAC version
Summary
Matching PR for dotnet/runtime#125231, which fixes
DefaultCOMImpl::Release()reference counting bugs in the DAC.Changes
1. Fix
SOSMethodEnumsimulator COM ref counting (util.cpp)The
SOSDacInterface15simulator'sGetMethodTableSlotEnumeratorhad the same bug pattern fixed in the runtime PR — raw pointer assignment withoutQueryInterface, and the null check occurred after the dereference. Fixed to:refCountto 0 (matchingDefaultCOMImplconvention)QueryInterfaceto return the interface, properly callingAddRefdeleteon failure (sinceQueryInterfacewasn't called at refCount 0)2. Fix
ISOSMethodEnumleak instrike.cppChanged raw
ISOSMethodEnum*toToRelease<ISOSMethodEnum>soRelease()is called when the pointer goes out of scope, matching the pattern used everywhere else (e.g.,ToRelease<ISOSHandleEnum>).