search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.96k stars 4.65k forks source link

Misinterpretation of VT_INT/UINT in new COM interop #97363

Closed weltkante closed 7 months ago

weltkante commented 7 months ago

Description

Comments in the new Variant Marshalling code and unit test indicate a misunderstanding of VT_INT/VT_UINT as meaning IntPtr/UIntPtr, however as far as I can tell the actual implementation for unmanaged-to-managed conversions (same file) is correctly implementing it like classic interop as Int32/UInt32. The comment in the Windows headers indicates that this is the int/long difference in C/C++ which both are 32 bit on x64 Windows, unlike in C#, so always treating it as 32 bit should be correct unless you expect compatibility with different ABIs than x86/x64 (for example I don't know what the situation is on ARM64)

For what its worth these types do appear in the typelib of ActiveX controls depending on int vs. long usage, I came across it in the Microsoft RDP Client ActiveX control (mstscax.dll, part of the Windows installation) while doing a proof-of-concept implementation using it in .NET Core. For example in the OnFocusReleased event of the non-dual IDispatch event interface the argument is specified as VT_INT instead of VT_I4 while other events like OnLogonError use long instead of int and have the usual VT_I4. However I haven't yet seen VT_INT passed at runtime during IDispatch event handling, OnFocusReleased seems to get passed VT_I4 instead in my tests.

/cc @jkoritzinsky @AaronRobinsonMSFT

Reproduction Steps

Review of Variant Marshaller implementation and commit https://github.com/dotnet/runtime/pull/93635

VT_INT appearing in the type library of mstscax.dll from a normal Windows installation

Expected behavior

Unit tests and comments do not misinterpret VT_INT/VT_UINT semantics.

Actual behavior

Comments and unit test in source code indicate an incorrect mapping of VT_INT/VT_UINT.

The practical implementation of unmanaged-to-managed mappings seems to differ from comments and unit tests, so is unlikely to have introduced actual bugs.

Regression?

VT_INT/VT_UINT works in classic COM interop and map to Int32/UInt32

Known Workarounds

not necessary as far as I can tell, worst case a custom marshaller could be used

Configuration

64-bit .NET 9.0.0-alpha.1.23617.4

Other information

No response

jkoritzinsky commented 7 months ago

The documentation for VT_INT and VT_UINT are not specific on the width of the value and various issues. A simple reading of the docs for VT_INT and VT_UINT leads to matching them to nint and nuint, especially with looking at other languages with similarly named integer types. As a result, I decided to add comments about the incongruity so anyone who looks at it in the future can understand why we mapped them to int/uint.

The unit test you linked to works a little differently than you think. The CreateRaw method doesn't require a strict type match, it only validates that the size of the type is equal to the expected size. The Create method is the only method that does an exact type match, and as you saw in the comments, we explicitly don't match nint and nuint to VT_INT and VT_UINT as it's incorrect.

We made this decision to treat them as int/uint to avoid the problems you raised in #36354 about the DLR's implementation.

weltkante commented 7 months ago

We made this decision to treat them as int/uint to avoid the problems you raised in https://github.com/dotnet/runtime/issues/36354 about the DLR's implementation.

Forgot about that issue, making them int/uint is definitely the right thing. In that old issue I think I was working against the docs and comparing implementations, without any practical use cases, but now I've got actually TLB and IDL code where it happens in practice.

Having read the corresponding IDL that generates VT_INT/VT_UINT and VT_I4/VT_UI4 (looking at IDL via oleview32, headers/docs of the APIs and interacting with the TLB via ITypeLib) it seems pretty clear that the intention behind VT_INT/VT_UINT is the LONG vs. INT difference in C/C++, which mostly matters to 16 bit vs. 32 bit and is an ABI decision that on the amd64 architecture they are not expanded to 32/64 bit, not something languages can decide.

For the pointer sized variant types we have VT_INT_PTR and VT_UINT_PTR, I actually got them in the RDP Client ActiveX as well, and interestingly enough the old tlbimp gets those very wrong and generates bad APIs in the assembly that are not portable between x86/amd64 even though it generates an AnyCpu assembly.

Anyways, I would have commented on the PR and not made a new issue but it was already closed when I came across this, so figured I'd document it in a new issue instead to document my findings. Since this is just about comments/unit tests feel free to close the issue if you are aware of the problems and don't think they need to be further clarified in source and comments.