Fix WSManInstance COM interface with ResourceURI #26692
Conversation
Fixes the COM definitions of some WSMan classes that stopped Set-WSManInstance and New-WSManInstance from working. As the COM definition places the put/set method for `ResourceUri` before the get, it is important we do the same otherwise .NET will call the wrong method in the VTable throwing an exception around an invalid URI. This has been broken since v6 when the CORECLR directives changed it from an IDispatch implementation to IUnknown with the IDispatch methods in the interface.
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the COM interface definitions for WSMan classes that has prevented Set-WSManInstance and New-WSManInstance from working since PowerShell v6. The issue stems from incorrect property accessor ordering in the C# interface definitions that don't match the native COM vtable order.
Changes:
- Swapped
ResourceUriproperty accessor order (set before get) to match COM vtable ordering - Swapped
MustUnderstandOptionsproperty accessor order (set before get) to match COM vtable ordering - Added StyleCop suppressions to document the intentional ordering
Comments suppressed due to low confidence (2)
src/Microsoft.WSMan.Management/Interop.cs:787
- The
FragmentPathproperty likely has the same COM vtable ordering issue asResourceUriandMustUnderstandOptions. If the native COM interface definesput_FragmentPathbeforeget_FragmentPath(as is typical for COM interfaces), this property should also have its accessors swapped with set before get. This property is actively used inWsManHelper.csline 582 where it's assigned a value, so the incorrect ordering could cause similar runtime failures.
string FragmentPath
{
// IDL: HRESULT FragmentPath ([out, retval] BSTR* ReturnValue);
[DispId(4)]
[return: MarshalAs(UnmanagedType.BStr)]
get;
// IDL: HRESULT FragmentPath (BSTR value);
[DispId(4)]
set;
}
src/Microsoft.WSMan.Management/Interop.cs:805
- The
FragmentDialectproperty likely has the same COM vtable ordering issue asResourceUriandMustUnderstandOptions. If the native COM interface definesput_FragmentDialectbeforeget_FragmentDialect(as is typical for COM interfaces), this property should also have its accessors swapped with set before get. This property is actively used inWsManHelper.csline 587 where it's assigned a value, so the incorrect ordering could cause similar runtime failures.
string FragmentDialect
{
// IDL: HRESULT FragmentDialect ([out, retval] BSTR* ReturnValue);
[DispId(5)]
[return: MarshalAs(UnmanagedType.BStr)]
get;
// IDL: HRESULT FragmentDialect (BSTR value);
[DispId(5)]
set;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@jborean93 Thanks for your investigations! Could you please look line 708 in Interop.cs. I guess now we can remove the old workaround and probably get more reliable result. #if CORECLR
[InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
#else
[InterfaceTypeAttribute(ComInterfaceType.InterfaceIsIDispatch)]
#endifI found CORECLR 33 times in the file but I hope these don't related to the issue. |
|
@iSazonov based on the git history and the structure of the file it looks to have been autogenerated using something like CsWin32. It was done when PowerShell was migrated to .NET (Core) so was built to support .NET Framework and the newer .NET at the same time. At the time When testing this change it seems like .NET now has added support back for |
|
@jborean93 Yes, it was at porting time due to limitations of early .Net version. Later .Net 5 was improved and @daxian-dbw backported COM Interop code to the repo. Description README.md And it is a question for @daxian-dbw is it time to remove old workarounds in the code? |
|
We certainly can, when debugging this problem I found that we could either keep using the Also the COM Interop stuff you linked to is AFAIK for |
@jborean93 To be clear I did not say that it is related. The link was for your information to point that COM support was improved in .Net 5 (and as result in pwsh too) five years ago. |
|
I’m just somewhat confused then. The code as is today is fine, it just had a bug that this fixed. It certainly can be cleaned up to remove the netfx specific branches as we aren’t building for netfx but that’s a bigger change so I didn’t do it. Better to get a smaller fix merged then wait months for someone to review the larger stuff which would still require this fix anyway. |
PR Summary
Fixes the COM definitions of some WSMan classes that stopped Set-WSManInstance and New-WSManInstance from working. As the COM definition places the put/set method for
ResourceUribefore the get, it is important we do the same otherwise .NET will call the wrong method in the VTable throwing an exception around an invalid URI. This has been broken since v6 when the CORECLR directives changed it from an IDispatch implementation to IUnknown with the IDispatch methods in the interface. With the move toIUnknownthe definition order is important as that is how C# sets up the VTable to invoke these methods.You can see the header which defines the methods with
put_ResourceUribeforeget_ResourceUrihttps://github.com/nihon-tc/Rtest/blob/8246e4d21323802fb84c406edc1a005991304f5a/header/Microsoft%20SDKs/Windows/v7.0A/Include/wsmandisp.h#L2426-L2430. Also fixesMustUnderstandOptionseven though we don't use it in the codebase.PR Context
Fixes: #26647
ansible/ansible-documentation#2709
MicrosoftDocs/PowerShell-Docs#5687
https://stackoverflow.com/questions/73780191/set-wsmaninstance-fails-in-powershell-7
Unfortuantely I cannot write a test because
New-WSManInstanceandSet-WSManInstancerequires WSMan to be setup which we cannot guarantee in CI. I have manually tested it withPR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header