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

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.

https://github.com/dotnet/runtime/blob/53c16198a8417e87e62c9df7c5e2e2602f9aa01d/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ISymWrapperCore.cs#L368-L374

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.

this mode of operation is supported as a post-compile step for library authors to reduce the size of their NuGet packages.

I don’t think that’s the case, the setup for this is complicated and requires several advanced options to be set correctly. It’d also question if it has any noticeable impact as you are not really trimming but more like tweaking compiler output.

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

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

I just want to reiterate Marek’s comment

I understand this isn’t the “trim my app” mode. But at the same time, we trim the libraries we ship for a reason, to reduce the size of the shared framework distribution, and that reason could be meaningful for any library builder. If memory serves, this reduced the size of our libraries on average by 5-10%. Who wouldn’t want their nuget package to be 5-10% smaller? (Rhetorical question.) I get this isn’t the primary use case for the linker, and that’s fine. But I don’t think we should be discounting the use case nor the bugs that end up silently introduced when it’s used in this supported fashion.

Regardless, this makes me wonder what other issues are lurking, with things being trimmed away we weren’t expecting. I definitely agree with @GrabYourPitchforks earlier suggestion:

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.

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.

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.