Expose interop manipulation of SafeHandle

[AaronRobinsonMSFT]

Background and Motivation

The SafeHandle type was designed around an assumption that the runtime could manage all aspects of its lifetime. The source generation proposal pushed this responsible onto a third party tool (i.e. Roslyn source generator). In order to better support that scenario, the following APIs are proposed to help with SafeHandle manipulation.

Proposed API

Update guidance here to indicate derived types would need a public default constructor. This proposal would include ensuring all SafeHandle implementations provided by .NET follow this guidance.

• Microsoft.Win32.SafeHandles.SafeAccessTokenHandle
• Microsoft.Win32.SafeHandles.SafeHandleMinusOneIsInvalid
• System.Net.Sockets.SafeSocketHandle
• Microsoft.Win32.SafeHandles.SafeHandleZeroOrMinusOneIsInvalid
• Microsoft.Win32.SafeHandles.SafeFileHandle
• Microsoft.Win32.SafeHandles.SafeMemoryMappedFileHandle
• Microsoft.Win32.SafeHandles.SafeNCryptHandle
• Microsoft.Win32.SafeHandles.SafePipeHandle
• Microsoft.Win32.SafeHandles.SafeProcessHandle
• Microsoft.Win32.SafeHandles.SafeRegistryHandle
• Microsoft.Win32.SafeHandles.SafeWaitHandle
• Microsoft.Win32.SafeHandles.SafeX509ChainHandle
• System.Runtime.InteropServices.SafeBuffer
• System.Security.Authentication.ExtendedProtection.ChannelBinding
• System.Security.Cryptography.SafeEvpPKeyHandle

Expose SetHandle as a public API so source generators may call it.

See #45633 (comment).

Usage Examples

Usage example would the ability to call SetHandle and instantiate any type derived from SafeHandle so they can be used from a source generator.

Alternative Designs

Expose static functions for SafeHandle manipulation.

namespace System.Runtime.InteropServices
{
    public static class Marshal
    {
+        /// <summary>
+        /// Create an instance of the given <typeparamref name="TSafeHandle"/>.
+        /// </summary>
+        /// <typeparam name="TSafeHandle">Type of the SafeHandle</typeparam>
+        /// <returns>New instance of <typeparamref name="TSafeHandle"/></returns>
+        /// <remarks>
+        /// The <typeparamref name="TSafeHandle"/> must be non-abstract and have a parameterless constructor.
+        /// </remarks>
+        public static TSafeHandle CreateSafeHandle<
+                      [DynamicallyAccessedMembers(
+                              DynamicallyAccessedMemberTypes.PublicParameterlessConstructor
+                              | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
+                       TSafeHandle>() where TSafeHandle : SafeHandle;

+        /// <summary>
+        /// Sets the handle of <paramref name="safeHandle"/> to the specified <paramref name="handle"/>.
+        /// </summary>
+        /// <param name="safeHandle"><see cref="SafeHandle"/> instance to update</param>
+        /// <param name="handle">Pre-existing handle</param>
+        public static void SetHandle(SafeHandle safeHandle, IntPtr handle);
    }
}

Risks

Low. These APIs expose internal lifetime operations that users have often wanted to hide. This is however mitigated by the fact that using reflection these "hidden" APIs can be used regardless.

[AaronRobinsonMSFT]

/cc @jkoritzinsky @elinor-fung

[stephentoub]

What is the purpose of the CreateSafeHandle method? Does it do reflection to access a private parameterless ctor?

[jkoritzinsky]

Yes, it uses reflection to access the private parameterless constructor on the passed in SafeHandle-derived type.

[jkotas]

Would it better to change the guidelines for safe handles instead? We can fix the ones that are part of the platform that should cover a lot.

SetHandle should be an instance method on SafeHandle.

[AaronRobinsonMSFT]

Would it better to change the guidelines for safe handles instead? We can fix the ones that are part of the platform that should cover a lot.
SetHandle should be an instance method on SafeHandle.

That is an option covered in the Alternative Design. I prefer the new APIs since it avoids anyone having to change code in order to consume the new source generator. That is my only argument though so if that isn't a concern then exposing SetHandle as a member and updating all .NET supplied SafeHandle implementations to have a public constructor would work.

[jkotas]

I think we should go with changing the guidelines as our first choice. It avoids all issues with private reflection and saves us from building creative solutions to avoid reflection performance issues.

If the interop source generators end up needing a private reflection fallback, they can just emit (TSafeHandle)Activator.CreateInstance(typeof(TSafeHandle), nonPublic: true) as the fallback for safe handles that were not updated to use the new guidelines. We should not need a special API for it.

[AaronRobinsonMSFT]

I think we should go with changing the guidelines as our first choice.

Okay. I will flip the proposal then - update guidance and expose SetHandle is the primary and new API is the alternative.

[bartonjs]

Video

We think that a hybrid approach is best:

• Update guidance to say that non-abstract SafeHandle types should have a default ctor (where IsInvalid is true).
  • Update all of our SafeHandle types to follow this guidance.
  • Add an analyzer to suggest this guidance.
• Do not make SafeHandle.SetHandle public.
• Add Marshal.InitHandle (using "Init" instead of "Set" to suggest that it should only be used on new instances).
• The reflection code to create instances of types lacking a public constructor is easy enough that the CreateSafeHandle method isn't required ((TSafeHandle)Activator.CreateInstance(typeof(TSafeHandle), nonPublic: true))

namespace System.Runtime.InteropServices
{
    public static class Marshal
    {
        /// <summary>
        /// Initializes the handle of a newly created blah blah blah.
        /// </summary>
        /// <param name="safeHandle"><see cref="SafeHandle"/> instance to update</param>
        /// <param name="handle">Pre-existing handle</param>
        public static void InitHandle(SafeHandle safeHandle, IntPtr handle);
    }
}

[marek-safar]

Update guidance to say that non-abstract SafeHandle types should have a default ctor (where IsInvalid is true).

Could an analyzer be added to help with the guidance?

[stephentoub]

@jkoritzinsky, this can be closed now, right?

[jkoritzinsky]

We still need to add the analyzer. If we want, we can open another issue to track just that