From 6ed0b492c18b90bcb0ea5d2f30ddd80c61828139 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Wed, 16 Oct 2019 07:43:42 +0200 Subject: [PATCH] Fix SafeHandle marshalling in ref/in/out parameters (mono/mono#17330) * 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 | 135 ++++++++++++++++++++++----------- src/mono/mono/tests/libtest.c | 10 ++- src/mono/mono/tests/safehandle.2.cs | 58 +++++++++++++- src/mono/netcore/CoreFX.issues.rsp | 4 + 4 files changed, 155 insertions(+), 52 deletions(-) diff --git a/src/mono/mono/metadata/marshal-ilgen.c b/src/mono/mono/metadata/marshal-ilgen.c index fdc385c..a3342a6 100644 --- a/src/mono/mono/metadata/marshal-ilgen.c +++ b/src/mono/mono/metadata/marshal-ilgen.c @@ -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); diff --git a/src/mono/mono/tests/libtest.c b/src/mono/mono/tests/libtest.c index 22b2631..2b80cd9 100644 --- a/src/mono/mono/tests/libtest.c +++ b/src/mono/mono/tests/libtest.c @@ -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 diff --git a/src/mono/mono/tests/safehandle.2.cs b/src/mono/mono/tests/safehandle.2.cs index 1731e04..4bead18 100644 --- a/src/mono/mono/tests/safehandle.2.cs +++ b/src/mono/mono/tests/safehandle.2.cs @@ -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; } diff --git a/src/mono/netcore/CoreFX.issues.rsp b/src/mono/netcore/CoreFX.issues.rsp index 223ebd8..1c094ce 100644 --- a/src/mono/netcore/CoreFX.issues.rsp +++ b/src/mono/netcore/CoreFX.issues.rsp @@ -954,3 +954,7 @@ # 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 -- 2.7.4