P/Invoke function returning SafeSocketHandle throws "System.MissingMethodException: .ctor"

[asmichi]

Description

On .NET 5, P/Invoke function returning SafeSocketHandle throws "System.MissingMethodException: .ctor". See MissingSafeSocketHandleCtor.zip for a full repro project.

    public static class Program
    {
        private const int AF_INET = 2;
        private const int SOCK_STREAM = 1;

        public static void Main()
        {
            // System.MissingMethodException: '.ctor'
            // (This returns -1 because we have not initialized WinSock2; that does not matter, though.)
            var sock = socket(AF_INET, SOCK_STREAM, 0);
            sock.Dispose();
        }

        [DllImport("Ws2_32.dll")]
        public static extern SafeSocketHandle socket(int af, int type, int protocol);
    }

Configuration

• .NET SDK 5.0.100
• Windows 10 Pro x64 (20H2, 19042.630)

Regression?

Yes. Worked on 3.1.404.

Other information

The SafeHandle doc says "You should also provide a parameterless constructor".

A parameterless ctor is defined in the SafeSocketHandle source file but missing from the released assembly at "C:\Program Files\dotnet\shared\Microsoft.NETCore.App\5.0.0\System.Net.Sockets.dll".

I do not think this is an intended behavior. I do now know the root cause, though.

Stack traces:

Unhandled exception. System.MissingMethodException: .ctor
   at ConsoleAppSafeSocketHandle.WinSock2.socket(Int32 af, Int32 type, Int32 protocol)
   at ConsoleAppSafeSocketHandle.Program.Main()

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

On .NET 5, P/Invoke function returning SafeSocketHandle throws "System.MissingMethodException: .ctor". See MissingSafeSocketHandleCtor.zip for a full repro project.

    public static class Program
    {
        private const int AF_INET = 2;
        private const int SOCK_STREAM = 1;

        public static void Main()
        {
            // System.MissingMethodException: '.ctor'
            // (This returns -1 because we have not initialized WinSock2; that does not matter, though.)
            var sock = socket(AF_INET, SOCK_STREAM, 0);
            sock.Dispose();
        }

        [DllImport("Ws2_32.dll")]
        public static extern SafeSocketHandle socket(int af, int type, int protocol);
    }

Configuration

• .NET SDK 5.0.100
• Windows 10 Pro x64 (20H2, 19042.630)

Regression?

Yes. Worked on 3.1.404.

Other information

The SafeHandle doc says "You should also provide a parameterless constructor".

A parameterless ctor is defined in the SafeSocketHandle source file but missing from the released assembly at "C:\Program Files\dotnet\shared\Microsoft.NETCore.App\5.0.0\System.Net.Sockets.dll".

I do not think this is an intended behavior. I do now know the root cause, though.

Stack traces:

Unhandled exception. System.MissingMethodException: .ctor
   at ConsoleAppSafeSocketHandle.WinSock2.socket(Int32 af, Int32 type, Int32 protocol)
   at ConsoleAppSafeSocketHandle.Program.Main()

Author:	asmichi
Assignees:	-
Labels:	area-System.Net.Sockets, untriaged
Milestone:	-

[asmichi]

I just wanted to bring this to your attention. Something seems wrong with the released assemblies.

This issue itself is not a blocking issue to me because this issue is easy to work around. It is even acceptable to me if this is marked as an unsupported use case (by design).

[GrabYourPitchforks]

Possible linker issue? I'd think the linker would special-case SafeHandle-derived types and not remove the ctors, but possible something went wrong in this instance.

/cc @marek-safar @MichalStrehovsky

[stephentoub]

@marek-safar, so even though the ctor exists:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs

Line 41 in dc0e11d

private SafeSocketHandle() : base(ownsHandle: true) => OwnsHandle = true;

the linker is trimming it out in the assembly build because no P/Invokes are using it in the assembly build?

This should be fixed in servicing; I'm not sure how that works. Can we service the linker and then service release/5.0 to use the updated linker?

[GrabYourPitchforks]

As a follow-up, we should diff all (incl. non-public) members of the runtime + BCL's pre-trimmed DLLs and post-trimmed DLLs. Could help identify other places where we've inadvertently made a break.

[MichalStrehovsky]

the linker is trimming it out in the assembly build because no P/Invokes are using it in the assembly build?

Yeah, I assume this code would cover it if the SafeHandle was actually used in a p/invoke:

https://github.com/mono/linker/blob/b24d0e663d5bf4772b0896e2bc3dab3391fae744/src/linker/Linker.Steps/MarkStep.cs#L2647-L2648

It happens to cover safe handles and things like blittable classes. If the SafeHandle is actually used in a p/invoke, linker will keep it. But it's not used in a p/invoke during the library build.

Linker should probably treat constructors of SafeHandles the same as if they were public.

[marek-safar]

Yep, this is an actually different issue. We are trimming the constructor as part of libraries build because it's not referenced anywhere and it's private.

I don't think there is much to be fixed in the linker. The linker on final apps would work fine if the constructor was actually present in the runtime/SDK assemblies. We should fix libraries build not to trim such constructors. There are multiple ways how to do it, one of them is to change the constructor to be public.

[stephentoub]

one of them is to change the constructor to be public

We should not need to contort the public API surface; whether we want to expose that ctor to be used by code directly is a separate conversation.

Linker should probably treat constructors of SafeHandles the same as if they were public

I agree with @MichalStrehovsky on this. This isn't specific to this particular SafeHandle.

[marek-safar]

We should not need to contort the public API surface

I meant to make it public in the implementation library, there are many other options but private does not feel like the right fit for a method which is called externally.

Linker should probably treat constructors of SafeHandles the same as if they were public

We could add another quirk option for the libraries build only into linker but that sounds like a riskier option for servicing to me.

This isn't specific to this particular SafeHandle.

Correct but wouldn't be better to handle the analysis of correct pinvoke support of SafeHandles via Roslyn Analyzers or even source code generator?

[stephentoub]

Correct but wouldn't be better to handle the analysis of correct pinvoke support of SafeHandles via Roslyn Analyzers or even source code generator?

I'm not following. It's perfectly correct to have a public SafeHandle with a private parameterless ctor and that isn't used by any P/Invokes in that library. And such a SafeHandle, like the one in question here, would validate correctly with an analyzer.

[jkotas]

The current convention for safe handle constructors works poorly for source generated interop as well. The constructor cannot be invoked directly from source, and thus it requires workarounds using private reflection or similar solution. cc @AaronRobinsonMSFT

I agree with @marek-safar that the best forward looking option is to mark the safe handle constructors as public.

A separate question is whether this is the best option for servicing. It may be - marking a few methods public in the implementation is fairly low-risk.

[MichalStrehovsky]

Note that this problem is not limited to SafeHandle - interop will also call private parameterless constructors of blittable classes if they're in an out position, so linker would also need to assume those are used too, for parity. It kind of feels like a reflection-like problem, thinking about it more.

We've been using ILLinkTrim/making members public as a solution to private reflection problems.

[stephentoub]

I agree with @marek-safar that the best forward looking option is to mark the safe handle constructors as public.

If we want to define new guidelines moving forward for new stuff, and retroactively make sure all our SafeHandles conform, that's fine. But it doesn't change the fact that a) the linker is a public tool for use in the ecosystem and should work with standard .NET patterns, and b) private ctors are supported as a standard pattern with SafeHandle.

[jkotas]

Many .NET patterns are fundamentally incompatible with linking. We can always add more special cases to the linker to make some of these patterns work. It makes the complexity to go through the roof. We went this route with .NET Native implementation of the linker. It was not a successful sustainable path.

I am just saying that there is a balance to strike. Just because of something is standard .NET pattern does not imply that linker has to support it.

[jkotas]

The linker on final apps would work fine if the constructor was actually present in the runtime/SDK assemblies.

Is it going to work fine when the SafeHandle is used as argument of a marshaled delegate; or when the interop signature is emitted via Reflection.Emit?

[MichalStrehovsky]

Is it going to work fine when the SafeHandle is used as argument of a marshaled delegate

Nope: dotnet/linker#1660

interop signature is emitted via Reflection.Emit

I was going back and forth whether Ref.Emit should be considered linking safe or not. Maybe this is the case where it isn't.

[marek-safar]

private ctors are supported as a standard pattern with SafeHandle.

That's the pattern linker supports today and works fine but we run linker in a special mode nobody else runs for our own build. We run linker on single library in kind of pre-trim mode to remove any unused methods. We have handled previous cases where unused methods were removed via XML descriptors, new linker options but and we could handle it with yet another special option but I think we have a broader problem here.

1, Are we sure we have the right default constructors on all SafeHandle types?
2, If never referenced methods are problematic with tooling (source generators, correctness tools, etc) is adding a special flag to linker the right solution?

[stephentoub]

Many .NET patterns are fundamentally incompatible with linking.

This seems equally unsustainable, if "many" common patterns simply fail to work properly. Do most of them produce warnings? This one obviously doesn't.

[jkotas]

They do not produce warnings today and it is something we need to work on. We have a long way to go on linker compatibility.

The problematic relationship of interop and linker friendliness is called out as one of the motivations for https://github.com/dotnet/runtime/blob/master/docs/design/features/source-generator-pinvokes.md. I expect that we will eventually recommend interop source generators for linker friendliness of all interop, except the most trivial one.

[AaronRobinsonMSFT]

I expect that we will eventually recommend interop source generators for linker friendliness of all interop, except the most trivial one.

For what its worth the Interop team is going to be writing a proposal to handle this for source generators - see #43060 under "New .NET APIs".

[MichalStrehovsky]

Do most of them produce warnings? This one obviously doesn't.

I just want to reiterate Marek's comment: "That's the pattern linker supports today and works fine but we run linker in a special mode nobody else runs for our own build" - if we were trimming an entire app and the SafeHandle is actually used in a p/invoke, linker would keep the ctor - not because linker knows about SafeHandle, but because it just keeps ctors of all parameter types used in out/return position of p/invokes. The issue here is hit because we run linker in a weird mode as part of library build that is not a user scenario with nice warnings. It's the "I know what I'm doing" mode.

[stephentoub]

What to do going forward: I think we should change the guideline for SafeHandle constructors to make them public

This alone makes it easy to create a handle instance you can't actually use, as the private ctor was used by the runtime with access to the protected handle field as well. We'd probably also want to add a public SetHandle-like API, while thinking through implications on source compatibility with such a new method on the base type.

[danmoseley]

cc @eerhardt

Folks that hit this in 5.0 have the workaround of adding a linker.xml file, right? It's just tough for them to realize what the fix/workaround is.

[AaronRobinsonMSFT]

What to do going forward: I think we should change the guideline for SafeHandle constructors to make them public, and fix all SafeHandles in the framework to conform.

Yes. This is part of what the interop team will be proposing to help support the DllImport source generator.

/cc @jkoritzinsky @elinor-fung

[marek-safar]

You're saying it no longer does for us, or our libraries are different, or we care about reductions too small for others to care about, or something else?

It was in the quote ".. step for library authors to reduce the size of their NuGet packages."

who should this bug be assigned to?

I think this should be handled on @danmosemsft side for servicing. Maybe @eerhardt can quickly step in but the cut for 5.0.2 is soon.

I think we should do something low-risk like add linker .XML

I agree that XML descriptor for libraries build is the easiest fix with no risks.

[stephentoub]

Folks that hit this in 5.0 have the workaround of adding a linker.xml file, right? It's just tough for them to realize what the fix/workaround is.

For SafeSocketHandle, no; it's removed as part of our build of the .dll.

[AaronRobinsonMSFT]

For SafeSocketHandle, no; it's removed as part of our build of the .dll.

This was one of the COM related issues that also occurred: dotnet/coreclr#28061

[eerhardt]

For the 5.0 fix, I looked through all SafeHandle types in the shared framework with the following code:

using System;
using System.IO;
using System.Reflection;
using System.Runtime.InteropServices;

foreach (var file in Directory.EnumerateFiles(@"C:\Program Files\dotnet\shared\Microsoft.NETCore.App\5.0.1\", "*.dll"))
{
    Assembly assembly;
    try
    {
        assembly = Assembly.Load(Path.GetFileNameWithoutExtension(file));
    }
    catch
    {
        continue;
    }

    foreach (Type t in assembly.GetTypes())
    {
        if (t.IsAssignableTo(typeof(SafeHandle)) && t.IsPublic && !t.IsAbstract)
        {
            if (t.GetConstructor(BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance, null, Type.EmptyTypes, null) == null)
            {
                Console.WriteLine(t.FullName);
            }
        }
    }
}

That spit out 2 types that would have this issue:

System.Net.Sockets.SafeSocketHandle
System.Security.Cryptography.SafeEvpPKeyHandle

So to fix this in 5.0 servicing, my plan is to introduce 2 "Library_Build" XML descriptors for the 2 SafeHandles above. That will ensure their private constructors are preserved.

Going forward, I agree that we need a better long term strategy here - manually inspecting/verifying that every SafeHandle we create has the right constructors after building isn't a sustainable solution.

---

Looking through the rest of the SafeHandle types that aren't public, one jumps out that may be in trouble: System.Reflection.Emit.PunkSafeHandle.

runtime/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ISymWrapperCore.cs

Lines 368 to 374 in 53c1619

	private delegate int DDefineDocument(ISymUnmanagedWriter* pthis,
	[MarshalAs(UnmanagedType.LPWStr)] string url,
	[In] ref Guid language,
	[In] ref Guid languageVender,
	[In] ref Guid documentType,
	[Out] out PunkSafeHandle ppsymUnmanagedDocumentWriter
	);

This SafeHandle is used as an Out of a delegate to a native function. Looking through the code, it appears this is only ever used if someone calls AssemblyBuilder::DefineDynamicModule(name, emitSymbolInfo: true). However, that API is not public in .NET Standard or .NET Core - only .NET Framework and Mono. So there is no way for a test, or user code to call into PunkSafeHandle, from what I can tell.

[eerhardt]

Some more information on the above research:

1. System.Security.Cryptography.SafeEvpPKeyHandle is only used with OpenSsl which doesn't work on Windows and throws PNSE.The default ctor exists for this Type on Linux. There isn't a reason someone would use this type on Windows, so I don't think we should make a change for it.

2. I ran the above program on Linux and macOS, and it identified 4 other potentially problematic SafeHandles on non-Windows builds:

   • Microsoft.Win32.SafeHandles.SafeRegistryHandle
   • Microsoft.Win32.SafeHandles.SafeAccessTokenHandle
   • Microsoft.Win32.SafeHandles.SafeMemoryMappedFileHandle
   • Microsoft.Win32.SafeHandles.SafeMemoryMappedViewHandle

The first 2 are Windows-specific, so there is no need to change them, for the same reason as SafeEvpPKeyHandle on Windows.

The 2 MemoryMapped SafeHandles can be used cross-platform. We have never shipped a non-Windows version of SafeMemoryMappedFileHandle that has had a default ctor. This SafeHandle is sort of "fake" on non-Windows anyway - it uses a fake IntPtr and doesn't release anything in ReleaseHandle(). So I don't think we should change SafeMemoryMappedFileHandle.

The only remaining SafeHandle is SafeMemoryMappedViewHandle. We DID ship a default ctor on this type for non-Windows for v1.0 and v1.1. However, it was removed in v2.0 (which is the release we started trimming the shared fx assemblies during the build). Someone could theoretically use this type as the return value of a p/invoke into mmap. So I'm leaning towards changing this one to be on the safe side.

[eerhardt]

Reopening for backport to 5.0.

[eerhardt]

This is now fixed in master (PR #45911) and release/5.0 (PR #45942)

[NN---]

@eerhardt When can we expect releasing .NET 5 update with the fix ?

[eerhardt]

When can we expect releasing .NET 5 update with the fix ?

This fix is in the release branch, so the next servicing release of .NET 5 will have the fix. According to https://docs.microsoft.com/en-us/dotnet/core/releases-and-support, Servicing updates ship almost every month, and 5.0.1 shipped in December. So I'd guess that 5.0.2 would ship in January or February.

[marek-safar]

5.0.2 should go out next week, you can test the fix by installing https://github.com/dotnet/installer#installers-and-binaries 5.0.2 preview version