From 89313ca06932bdb69c53965ec1e37b67f920ee95 Mon Sep 17 00:00:00 2001
From: Jan Kotas <jkotas@microsoft.com>
Date: Thu, 27 May 2021 16:42:12 -0700
Subject: [PATCH] Marshal.PtrToStructure fixes (#53360)

- Fix reflection annotations. PtrToStructure calls private constructors.
- Move PtrToStructure overload that creates objects to shared CoreLib.
- Fix Mono implementation to call private constructor.
- Fix Mono error handling to match CoreCLR.
---
 .../Runtime/InteropServices/Marshal.CoreCLR.cs     |  8 ------
 .../src/System.Diagnostics.EventLog.csproj         |  2 ++
 .../src/System/Diagnostics/Reader/NativeWrapper.cs |  3 ++-
 .../src/System/Runtime/InteropServices/Marshal.cs  | 29 +++++++++++++++++++---
 .../ref/System.Runtime.InteropServices.cs          |  4 +--
 .../InteropServices/Marshal/PtrToStructureTests.cs |  9 ++++---
 .../System/Runtime/InteropServices/Marshal.Mono.cs | 11 ++------
 7 files changed, 39 insertions(+), 27 deletions(-)

diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs
index c1604da..688e4f8 100644
--- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs
+++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs
@@ -209,14 +209,6 @@ namespace System.Runtime.InteropServices
         [MethodImpl(MethodImplOptions.InternalCall)]
         public static extern void StructureToPtr(object structure, IntPtr ptr, bool fDeleteOld);
 
-        private static object PtrToStructureHelper(IntPtr ptr, Type structureType)
-        {
-            var rt = (RuntimeType)structureType;
-            object structure = rt.CreateInstanceDefaultCtor(publicOnly: false, wrapExceptions: true)!;
-            PtrToStructureHelper(ptr, structure, allowValueClasses: true);
-            return structure;
-        }
-
         /// <summary>
         /// Helper function to copy a pointer into a preallocated structure.
         /// </summary>
diff --git a/src/libraries/System.Diagnostics.EventLog/src/System.Diagnostics.EventLog.csproj b/src/libraries/System.Diagnostics.EventLog/src/System.Diagnostics.EventLog.csproj
index 3343628..57cef21 100644
--- a/src/libraries/System.Diagnostics.EventLog/src/System.Diagnostics.EventLog.csproj
+++ b/src/libraries/System.Diagnostics.EventLog/src/System.Diagnostics.EventLog.csproj
@@ -129,6 +129,8 @@
     <ProjectReference Include="$(LibrariesProjectRoot)System.Threading.AccessControl\src\System.Threading.AccessControl.csproj" />
   </ItemGroup>
   <ItemGroup Condition="!$(TargetFramework.StartsWith('$(NetCoreAppCurrent)'))">
+    <Compile Include="$(CoreLibSharedDir)System\Diagnostics\CodeAnalysis\DynamicallyAccessedMembersAttribute.cs" />
+    <Compile Include="$(CoreLibSharedDir)System\Diagnostics\CodeAnalysis\DynamicallyAccessedMemberTypes.cs" />
     <Compile Include="$(CoreLibSharedDir)System\Diagnostics\CodeAnalysis\UnconditionalSuppressMessageAttribute.cs" />
   </ItemGroup>
   <!-- Ensure we run ResolveProjectReferences to add the message DLL to the package.
diff --git a/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/Reader/NativeWrapper.cs b/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/Reader/NativeWrapper.cs
index 44020c5..1bd7e19 100644
--- a/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/Reader/NativeWrapper.cs
+++ b/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/Reader/NativeWrapper.cs
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.Runtime.InteropServices;
 using System.Text;
 using System.Security.Principal;
@@ -1260,7 +1261,7 @@ namespace System.Diagnostics.Eventing.Reader
                 return new EventLogHandle(val.Handle, true);
         }
 
-        public static Array ConvertToArray<T>(UnsafeNativeMethods.EvtVariant val, int size) where T : struct
+        public static Array ConvertToArray<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]T>(UnsafeNativeMethods.EvtVariant val, int size) where T : struct
         {
             IntPtr ptr = val.Reference;
             if (ptr == IntPtr.Zero)
diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
index 6b0838f..0f06864 100644
--- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
@@ -573,7 +573,7 @@ namespace System.Runtime.InteropServices
         /// native memory block to it.
         /// </summary>
         public static object? PtrToStructure(IntPtr ptr,
-            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
+            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
             Type structureType)
         {
             if (ptr == IntPtr.Zero)
@@ -594,7 +594,9 @@ namespace System.Runtime.InteropServices
                 throw new ArgumentException(SR.Argument_MustBeRuntimeType, nameof(structureType));
             }
 
-            return PtrToStructureHelper(ptr, structureType);
+            object structure = Activator.CreateInstance(structureType, nonPublic: true)!;
+            PtrToStructureHelper(ptr, structure, allowValueClasses: true);
+            return structure;
         }
 
         /// <summary>
@@ -607,10 +609,29 @@ namespace System.Runtime.InteropServices
 
         public static void PtrToStructure<T>(IntPtr ptr, [DisallowNull] T structure)
         {
-            PtrToStructure(ptr, (object)structure!);
+            PtrToStructureHelper(ptr, structure, allowValueClasses: false);
         }
 
-        public static T? PtrToStructure<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]T>(IntPtr ptr) => (T)PtrToStructure(ptr, typeof(T))!;
+        public static T? PtrToStructure<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]T>(IntPtr ptr)
+        {
+            if (ptr == IntPtr.Zero)
+            {
+                // Compat: this was originally implemented as a call to the non-generic version+cast.
+                // It would throw for non-nullable valuetypes here and return null for Nullable<T> even
+                // though it's generic.
+                return (T)(object)null!;
+            }
+
+            Type structureType = typeof(T);
+            if (structureType.IsGenericType)
+            {
+                throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(T));
+            }
+
+            object structure = Activator.CreateInstance(structureType, nonPublic: true)!;
+            PtrToStructureHelper(ptr, structure, allowValueClasses: true);
+            return (T)structure;
+        }
 
         public static void DestroyStructure<T>(IntPtr ptr) => DestroyStructure(ptr, typeof(T));
 
diff --git a/src/libraries/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs b/src/libraries/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs
index b9179b6..0cf9aff 100644
--- a/src/libraries/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs
+++ b/src/libraries/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs
@@ -624,8 +624,8 @@ namespace System.Runtime.InteropServices
         [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
         public static void PtrToStructure(System.IntPtr ptr, object structure) { }
         [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
-        public static object? PtrToStructure(System.IntPtr ptr, [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] System.Type structureType) { throw null; }
-        public static T? PtrToStructure<[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]T>(System.IntPtr ptr) { throw null; }
+        public static object? PtrToStructure(System.IntPtr ptr, [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.NonPublicConstructors| System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] System.Type structureType) { throw null; }
+        public static T? PtrToStructure<[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.NonPublicConstructors | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)]T>(System.IntPtr ptr) { throw null; }
         public static void PtrToStructure<T>(System.IntPtr ptr, [System.Diagnostics.CodeAnalysis.DisallowNullAttribute] T structure) { }
         public static int QueryInterface(System.IntPtr pUnk, ref System.Guid iid, out System.IntPtr ppv) { throw null; }
         public static byte ReadByte(System.IntPtr ptr) { throw null; }
diff --git a/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/Marshal/PtrToStructureTests.cs b/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/Marshal/PtrToStructureTests.cs
index 86cd412..f2a6c2b 100644
--- a/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/Marshal/PtrToStructureTests.cs
+++ b/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/Marshal/PtrToStructureTests.cs
@@ -15,7 +15,7 @@ namespace System.Runtime.InteropServices.Tests
         [Fact]
         public void StructureToPtr_NonGenericType_ReturnsExpected()
         {
-            var structure = new SomeTestStruct
+            var structure = new SequentialClass
             {
                 i = 10,
                 s = "hello"
@@ -27,7 +27,7 @@ namespace System.Runtime.InteropServices.Tests
             {
                 Marshal.StructureToPtr(structure, ptr, false);
 
-                SomeTestStruct result = Assert.IsType<SomeTestStruct>(Marshal.PtrToStructure(ptr, typeof(SomeTestStruct)));
+                SequentialClass result = Assert.IsType<SequentialClass>(Marshal.PtrToStructure(ptr, typeof(SequentialClass)));
                 Assert.Equal(10, result.i);
                 Assert.Equal("hello", result.s);
             }
@@ -125,7 +125,6 @@ namespace System.Runtime.InteropServices.Tests
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/mono/mono/issues/15101", TestRuntimes.Mono)]
         public void PtrToStructure_ZeroPointer_ThrowsArgumentNullException()
         {
             AssertExtensions.Throws<ArgumentNullException>("ptr", () => Marshal.PtrToStructure(IntPtr.Zero, (object)new SomeTestStruct()));
@@ -256,6 +255,10 @@ namespace System.Runtime.InteropServices.Tests
         [StructLayout(LayoutKind.Sequential)]
         public class SequentialClass
         {
+            internal SequentialClass()
+            {
+            }
+
             public int i;
             public string s;
         }
diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.Mono.cs
index 6e71235..5b42f03 100644
--- a/src/mono/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.Mono.cs
+++ b/src/mono/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.Mono.cs
@@ -67,18 +67,11 @@ namespace System.Runtime.InteropServices
         {
             if (structure == null)
                 throw new ArgumentNullException(nameof(structure));
+            if (ptr == IntPtr.Zero)
+                throw new ArgumentNullException(nameof(ptr));
             PtrToStructureInternal(ptr, structure, allowValueClasses);
         }
 
-        private static object PtrToStructureHelper(IntPtr ptr,
-            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
-            Type structureType)
-        {
-            object obj = Activator.CreateInstance(structureType)!;
-            PtrToStructureHelper(ptr, obj, true);
-            return obj;
-        }
-
         [MethodImpl(MethodImplOptions.InternalCall)]
         private static extern Delegate GetDelegateForFunctionPointerInternal(IntPtr ptr, Type t);
 
-- 
2.7.4