Revert "Fix COM ref counting in SOSMethodEnum simulator and bump SOS_BREAKING_CHANGE_VERSION"#5748
Conversation
…BREAKING…" This reverts commit 14d299f.
There was a problem hiding this comment.
Pull request overview
This PR reverts #5746 by restoring the previous SOS breaking-change version and undoing changes related to COM reference counting around the ISOSMethodEnum simulator/enumerator path.
Changes:
- Revert
SOS_BREAKING_CHANGE_VERSIONfrom 6 back to 5 in the SOS IDL and prebuilt header. - Revert
SOSMethodEnumsimulator initialization/refcounting and howGetMethodTableSlotEnumeratorreturns the enumerator. - Update
DumpMTto use a rawISOSMethodEnum*instead ofToRelease<ISOSMethodEnum>.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/shared/pal/prebuilt/inc/sospriv.h | Reverts the SOS breaking-change version macro to 5. |
| src/shared/inc/sospriv.idl | Reverts the IDL-defined SOS breaking-change version macro to 5. |
| src/SOS/Strike/util.cpp | Reverts simulator enumerator creation/hand-off logic for method table slot enumeration. |
| src/SOS/Strike/strike.cpp | Reverts DumpMT enumerator handling from RAII to a raw COM pointer. |
Comments suppressed due to low confidence (1)
src/SOS/Strike/strike.cpp:1326
pMethodEnumeratorwas changed fromToRelease<ISOSMethodEnum>to a rawISOSMethodEnum*, but the code never callsRelease()after the enumeration completes. This leaks the COM reference (and for the simulator case, leaks the heap allocation). Please restore RAII (e.g.,ToRelease/ReleaseHolder) or add a guaranteedRelease()on all paths after the loop.
ISOSMethodEnum *pMethodEnumerator;
if (SUCCEEDED(g_sos15->GetMethodTableSlotEnumerator(dwStartAddr, &pMethodEnumerator)))
{
SOSMethodData entry;
unsigned int fetched;
while (SUCCEEDED(pMethodEnumerator->Next(1, &entry, &fetched)) && fetched != 0)
{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SOSMethodEnum *simulator = new(std::nothrow) SOSMethodEnum(mt); | ||
| *enumerator = simulator; | ||
| if (simulator == NULL) | ||
| { | ||
| return E_OUTOFMEMORY; | ||
| } | ||
| HRESULT hr = simulator->Reset(); | ||
|
|
||
| if (SUCCEEDED(hr)) | ||
| hr = simulator->QueryInterface(__uuidof(ISOSMethodEnum), (void**)enumerator); | ||
|
|
||
| if (FAILED(hr)) | ||
| delete simulator; | ||
|
|
||
| { | ||
| simulator->Release(); | ||
| } | ||
| return hr; |
There was a problem hiding this comment.
GetMethodTableSlotEnumerator assigns *enumerator before validating the out-parameter and before knowing whether initialization succeeds. If Reset() fails, the code releases (and may delete) simulator but leaves *enumerator pointing at freed memory, and it will also AV if enumerator is null. Please validate enumerator (return E_POINTER), set *enumerator = nullptr on entry, and only publish the pointer on success (or explicitly null it after releasing on failure).
Reverts #5746