Fix SafeHandle marshalling in ref/in/out parameters (mono/mono#17330)
authorFilip Navara <navara@emclient.com>
Wed, 16 Oct 2019 05:43:42 +0000 (07:43 +0200)
committerZoltan Varga <vargaz@gmail.com>
Wed, 16 Oct 2019 05:43:42 +0000 (01:43 -0400)
* Fix SafeHandle marshalling in ref/in/out parameters

* Reorder code to make it exception safe, update comments

* Revert the code reordering because it breaks the invariant that new handle returned from native code should always create new SafeHandle on the managed side

* Fix test_0_safehandle_ref test. The test was expecting incorrect behavior, verified on .NET Core 3 and .NET Framework 4.8.

* Address PR feedback

* Fix cut & paste error

* Fix build

* Really fix the test_0_safehandle_ref tests

* Add more tests for SafeHandle marshalling

* Add exclusion for broken CoreFX test

Commit migrated from https://github.com/mono/mono/commit/0c02bb1207af75c176c369e411d9f4407e21030d

src/mono/mono/metadata/marshal-ilgen.c
src/mono/mono/tests/libtest.c
src/mono/mono/tests/safehandle.2.cs
src/mono/netcore/CoreFX.issues.rsp

index fdc385c..a3342a6 100644 (file)
@@ -59,6 +59,9 @@ enum {
 };
 #undef OPDEF
 
+#define IS_IN(t) ((t->attrs & PARAM_ATTRIBUTE_IN) || !(t->attrs & PARAM_ATTRIBUTE_OUT))
+#define IS_OUT(t) ((t->attrs & PARAM_ATTRIBUTE_OUT) || !(t->attrs & PARAM_ATTRIBUTE_IN))
+
 static GENERATE_GET_CLASS_WITH_CACHE (fixed_buffer_attribute, "System.Runtime.CompilerServices", "FixedBufferAttribute");
 static GENERATE_GET_CLASS_WITH_CACHE (date_time, "System", "DateTime");
 static GENERATE_TRY_GET_CLASS_WITH_CACHE (icustom_marshaler, "System.Runtime.InteropServices", "ICustomMarshaler");
@@ -5134,16 +5137,6 @@ emit_marshal_safehandle_ilgen (EmitMarshalContext *m, int argnum, MonoType *t,
                mono_mb_emit_exception (mb, "ArgumentNullException", NULL);
                
                mono_mb_patch_branch (mb, pos);
-               if (t->byref){
-                       /*
-                        * My tests in show that ref SafeHandles are not really
-                        * passed as ref objects.  Instead a NULL is passed as the
-                        * value of the ref
-                        */
-                       mono_mb_emit_icon (mb, 0);
-                       mono_mb_emit_stloc (mb, conv_arg);
-                       break;
-               } 
 
                /* Create local to hold the ref parameter to DangerousAddRef */
                dar_release_slot = mono_mb_add_local (mb, boolean_type);
@@ -5152,16 +5145,40 @@ emit_marshal_safehandle_ilgen (EmitMarshalContext *m, int argnum, MonoType *t,
                mono_mb_emit_icon (mb, 0);
                mono_mb_emit_stloc (mb, dar_release_slot);
 
-               /* safehandle.DangerousAddRef (ref release) */
-               mono_mb_emit_ldarg (mb, argnum);
-               mono_mb_emit_ldloc_addr (mb, dar_release_slot);
-               mono_mb_emit_managed_call (mb, sh_dangerous_add_ref, NULL);
+               if (t->byref) {
+                       int old_handle_value_slot = mono_mb_add_local (mb, int_type);
 
-               /* Pull the handle field from SafeHandle */
-               mono_mb_emit_ldarg (mb, argnum);
-               mono_mb_emit_ldflda (mb, MONO_STRUCT_OFFSET (MonoSafeHandle, handle));
-               mono_mb_emit_byte (mb, CEE_LDIND_I);
-               mono_mb_emit_stloc (mb, conv_arg);
+                       if (!IS_IN (t)) {
+                               mono_mb_emit_icon (mb, 0);
+                               mono_mb_emit_stloc (mb, conv_arg);
+                       } else {
+                               /* safehandle.DangerousAddRef (ref release) */
+                               mono_mb_emit_ldarg (mb, argnum);
+                               mono_mb_emit_byte (mb, CEE_LDIND_REF);
+                               mono_mb_emit_ldloc_addr (mb, dar_release_slot);
+                               mono_mb_emit_managed_call (mb, sh_dangerous_add_ref, NULL);
+
+                               /* Pull the handle field from SafeHandle */
+                               mono_mb_emit_ldarg (mb, argnum);
+                               mono_mb_emit_byte (mb, CEE_LDIND_REF);
+                               mono_mb_emit_ldflda (mb, MONO_STRUCT_OFFSET (MonoSafeHandle, handle));
+                               mono_mb_emit_byte (mb, CEE_LDIND_I);
+                               mono_mb_emit_byte (mb, CEE_DUP);
+                               mono_mb_emit_stloc (mb, conv_arg);
+                               mono_mb_emit_stloc (mb, old_handle_value_slot);
+                       }
+               } else {
+                       /* safehandle.DangerousAddRef (ref release) */
+                       mono_mb_emit_ldarg (mb, argnum);
+                       mono_mb_emit_ldloc_addr (mb, dar_release_slot);
+                       mono_mb_emit_managed_call (mb, sh_dangerous_add_ref, NULL);
+
+                       /* Pull the handle field from SafeHandle */
+                       mono_mb_emit_ldarg (mb, argnum);
+                       mono_mb_emit_ldflda (mb, MONO_STRUCT_OFFSET (MonoSafeHandle, handle));
+                       mono_mb_emit_byte (mb, CEE_LDIND_I);
+                       mono_mb_emit_stloc (mb, conv_arg);
+               }
 
                break;
        }
@@ -5182,35 +5199,61 @@ emit_marshal_safehandle_ilgen (EmitMarshalContext *m, int argnum, MonoType *t,
                        init_safe_handle ();
 
                if (t->byref){
-                       ERROR_DECL (error);
-                       MonoMethod *ctor;
-                       
-                       /*
-                        * My tests indicate that ref SafeHandles parameters are not actually
-                        * passed by ref, but instead a new Handle is created regardless of
-                        * whether a change happens in the unmanaged side.
-                        *
-                        * Also, the Handle is created before calling into unmanaged code,
-                        * but we do not support that mechanism (getting to the original
-                        * handle) and it makes no difference where we create this
-                        */
-                       ctor = mono_class_get_method_from_name_checked (t->data.klass, ".ctor", 0, 0, error);
-                       if (ctor == NULL || !is_ok (error)){
-                               mono_mb_emit_exception (mb, "MissingMethodException", "paramterless constructor required");
-                               mono_error_cleanup (error);
-                               break;
+                       /* If there was SafeHandle on input we have to release the reference to it */
+                       if (IS_IN (t)) {
+                               mono_mb_emit_ldloc (mb, dar_release_slot);
+                               label_next = mono_mb_emit_branch (mb, CEE_BRFALSE);
+                               mono_mb_emit_ldarg (mb, argnum);
+                               mono_mb_emit_byte (mb, CEE_LDIND_I);
+                               mono_mb_emit_managed_call (mb, sh_dangerous_release, NULL);
+                               mono_mb_patch_branch (mb, label_next);
                        }
-                       /* refval = new SafeHandleDerived ()*/
-                       mono_mb_emit_ldarg (mb, argnum);
-                       mono_mb_emit_op (mb, CEE_NEWOBJ, ctor);
-                       mono_mb_emit_byte (mb, CEE_STIND_REF);
 
-                       /* refval.handle = returned_handle */
-                       mono_mb_emit_ldarg (mb, argnum);
-                       mono_mb_emit_byte (mb, CEE_LDIND_REF);
-                       mono_mb_emit_ldflda (mb, MONO_STRUCT_OFFSET (MonoSafeHandle, handle));
-                       mono_mb_emit_ldloc (mb, conv_arg);
-                       mono_mb_emit_byte (mb, CEE_STIND_I);
+                       if (IS_OUT (t)) {
+                               ERROR_DECL (local_error);
+                               MonoMethod *ctor;
+                       
+                               /*
+                                * If the SafeHandle was marshalled on input we can skip the marshalling on
+                                * output if the handle value is identical.
+                                */
+                               if (IS_IN (t)) {
+                                       int old_handle_value_slot = dar_release_slot + 1;
+                                       mono_mb_emit_ldloc (mb, old_handle_value_slot);
+                                       mono_mb_emit_ldloc (mb, conv_arg);
+                                       label_next = mono_mb_emit_branch (mb, CEE_BEQ);
+                               }
+
+                               /*
+                                * Create an empty SafeHandle (of correct derived type).
+                                * 
+                                * FIXME: If an out-of-memory situation or exception happens here we will
+                                * leak the handle. We should move the allocation of the SafeHandle to the
+                                * input marshalling code to prevent that.
+                                */
+                               ctor = mono_class_get_method_from_name_checked (t->data.klass, ".ctor", 0, 0, local_error);
+                               if (ctor == NULL || !is_ok (local_error)){
+                                       mono_mb_emit_exception (mb, "MissingMethodException", "paramterless constructor required");
+                                       mono_error_cleanup (local_error);
+                                       break;
+                               }
+
+                               /* refval = new SafeHandleDerived ()*/
+                               mono_mb_emit_ldarg (mb, argnum);
+                               mono_mb_emit_op (mb, CEE_NEWOBJ, ctor);
+                               mono_mb_emit_byte (mb, CEE_STIND_REF);
+
+                               /* refval.handle = returned_handle */
+                               mono_mb_emit_ldarg (mb, argnum);
+                               mono_mb_emit_byte (mb, CEE_LDIND_REF);
+                               mono_mb_emit_ldflda (mb, MONO_STRUCT_OFFSET (MonoSafeHandle, handle));
+                               mono_mb_emit_ldloc (mb, conv_arg);
+                               mono_mb_emit_byte (mb, CEE_STIND_I);
+
+                               if (IS_IN (t)) {
+                                       mono_mb_patch_branch (mb, label_next);
+                               }
+                       }
                } else {
                        mono_mb_emit_ldloc (mb, dar_release_slot);
                        label_next = mono_mb_emit_branch (mb, CEE_BRFALSE);
index 22b2631..2b80cd9 100644 (file)
@@ -2593,11 +2593,17 @@ LIBTEST_API void STDCALL
 mono_safe_handle_ref (void **handle)
 {
        if (*handle != 0){
-               *handle = (void *) 0xbad;
+               *handle = (void *) 0x800d;
                return;
        }
 
-       *handle = (void *) 0x800d;
+       *handle = (void *) 0xbad;
+}
+
+LIBTEST_API void* STDCALL
+mono_safe_handle_ref_nomod (void **handle)
+{
+       return *handle;
 }
 
 LIBTEST_API double STDCALL
index 1731e04..4bead18 100644 (file)
@@ -55,13 +55,22 @@ public class Tests {
        [DllImport ("libtest", EntryPoint="mono_safe_handle_ref")]
        public static extern void mono_safe_handle_ref2 (ref MyHandleNoCtor handle);
 
+       [DllImport ("libtest", EntryPoint="mono_safe_handle_ref_nomod")]
+       public static extern IntPtr mono_safe_handle_ref_nomod_in (in MyHandle handle);
+
+       [DllImport ("libtest", EntryPoint="mono_safe_handle_ref_nomod")]
+       public static extern IntPtr mono_safe_handle_ref_nomod_out (out MyHandle handle);
+
+       [DllImport ("libtest", EntryPoint="mono_safe_handle_ref_nomod")]
+       public static extern IntPtr mono_safe_handle_ref_nomod_ref (ref MyHandle handle);
+
        public static int test_0_safehandle_ref_noctor ()
        {
                MyHandleNoCtor m = new MyHandleNoCtor ((IntPtr) 0xdead);
 
                try {
                        mono_safe_handle_ref2 (ref m);
-               } catch (MissingMethodException e){
+               } catch (MissingMethodException{
                        Console.WriteLine ("Good: got exception requried");
                        return 0;
                }
@@ -72,6 +81,7 @@ public class Tests {
        public static int test_0_safehandle_ref ()
        {
                MyHandle m = new MyHandle ((IntPtr) 0xdead);
+               MyHandle m_saved = m;
 
                mono_safe_handle_ref (ref m);
                
@@ -79,10 +89,50 @@ public class Tests {
                        Console.WriteLine ("test_0_safehandle_ref: fail; Expected 0x800d, got: {0:x}", m.DangerousGetHandle ());
                        return 1;
                }
+
+               if (m == m_saved) {
+                       Console.WriteLine ("test_0_safehandle_ref: fail; Expected new SafeHandle on return");
+                       return 2;
+               }
+
                Console.WriteLine ("test_0_safehandle_ref: pass");
                return 0;
        }
 
+       public static int test_0_safehandle_ref_nomod_in ()
+       {
+               MyHandle m = new MyHandle ((IntPtr) 0xdead);
+               IntPtr ret = mono_safe_handle_ref_nomod_in (in m);
+               return ret == (IntPtr) 0xdead ? 0 : 1;
+       }
+
+       public static int test_0_safehandle_ref_nomod_out ()
+       {
+               MyHandle m;
+               IntPtr ret = mono_safe_handle_ref_nomod_out (out m);
+               return ret == IntPtr.Zero ? 0 : 1;
+       }
+
+       public static int test_0_safehandle_ref_nomod_ref ()
+       {
+               MyHandle m = new MyHandle ((IntPtr) 0xdead);
+               MyHandle m_saved = m;
+               IntPtr ret = mono_safe_handle_ref_nomod_ref (ref m);
+               if (ret != (IntPtr) 0xdead) {
+                       Console.WriteLine ("test_0_safehandle_ref_nomod_ref: fail; Expected 0xdead, got {0:x}", ret);
+                       return 1;
+               }
+               if (m != m_saved) {
+                       Console.WriteLine ("test_0_safehandle_ref_nomod_ref: fail; Expected same SafeHandle on input and output");
+                       return 2;
+               }
+               if (m.DangerousGetHandle () != (IntPtr) 0xdead) {
+                       Console.WriteLine ("test_0_safehandle_ref_nomod_ref: fail; Expected 0xdead, got {0:x}", m.DangerousGetHandle ());
+                       return 3;
+               }
+               return 0;
+       }
+
        [DllImport ("libtest")]
        public static extern int mono_xr (SafeHandle sh);
        
@@ -105,12 +155,12 @@ public class Tests {
        }
        
 
-        [StructLayout (LayoutKind.Sequential)]
+       [StructLayout (LayoutKind.Sequential)]
        public struct StringOnStruct {
                public string a;
        }
 
-        [StructLayout (LayoutKind.Sequential)]
+       [StructLayout (LayoutKind.Sequential)]
        public struct StructTest {
                public int a;
                public SafeHandle handle1;
@@ -118,7 +168,7 @@ public class Tests {
                public int b;
        }
 
-        [StructLayout (LayoutKind.Sequential)]
+       [StructLayout (LayoutKind.Sequential)]
        public struct StructTest1 {
                public SafeHandle a;
        }
index 223ebd8..1c094ce 100644 (file)
 
 # Requires NativeLibrary implementation
 -nomethod System.Net.Tests.HttpWebRequestTest.ServicePoint_GetValue_ExpectedResult
+
+# Broken SafeWaitHandle test on finalization
+# https://github.com/dotnet/corefx/issues/41635#issuecomment-542409299
+-nomethod SafeWaitHandleExtensionsTests.SafeWaitHandleExtensions_set
\ No newline at end of file