From 288de113d01c183d5e323005728096911dc03c55 Mon Sep 17 00:00:00 2001 From: Johan Lorensson Date: Tue, 3 May 2022 09:02:53 +0200 Subject: [PATCH] [Mono]: Fix static closed delegate fnptr crash. (#68701) * Fix static closed delegate fnptr crash. When accessing a function pointer to a static closed delegate like done in added test: GetFunctionPointerForDelegate_MarshalledClosedStaticDelegate it will trigger a read outside of the allocated mspecs buffer since invoke_sig and method signature arguments wont match. There was already logic to adjust this in emit_managed_wrapper_ilgen, but done after call to emit_managed_wrapper_validate_signature that will touch memory and depending on its content, trigger a crash. Fix make sure we do signature adjustments first and then validate the signature. Fix also adjust a couple of mspecs allocations to use g_new0 as all others to make sure we get NULL pointers in the mspecs array. Since this scenario was not covered on CI, commit also adds a new test in GetFunctionPointerForDelegateTests.cs covering this scenario. --- .../GetFunctionPointerForDelegateTests.cs | 21 ++++++++++++++ src/mono/mono/metadata/cominterop.c | 2 +- src/mono/mono/metadata/marshal-ilgen.c | 29 ++++++++++--------- src/mono/mono/metadata/marshal.c | 2 +- 4 files changed, 39 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Runtime.InteropServices/tests/System.Runtime.InteropServices.UnitTests/System/Runtime/InteropServices/Marshal/GetFunctionPointerForDelegateTests.cs b/src/libraries/System.Runtime.InteropServices/tests/System.Runtime.InteropServices.UnitTests/System/Runtime/InteropServices/Marshal/GetFunctionPointerForDelegateTests.cs index 85328ac802696..9fd725f06b7ff 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/System.Runtime.InteropServices.UnitTests/System/Runtime/InteropServices/Marshal/GetFunctionPointerForDelegateTests.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/System.Runtime.InteropServices.UnitTests/System/Runtime/InteropServices/Marshal/GetFunctionPointerForDelegateTests.cs @@ -92,8 +92,29 @@ public void GetFunctionPointer_GenericDelegate_ThrowsArgumentException() AssertExtensions.Throws("delegate", () => Marshal.GetFunctionPointerForDelegate(d)); } + [Fact] + [SkipOnPlatform(TestPlatforms.Browser, "Not supported on Browser.")] + public void GetFunctionPointerForDelegate_MarshalledOpenStaticDelegate() + { + MethodInfo targetMethod = typeof(GetFunctionPointerForDelegateTests).GetMethod(nameof(Method), BindingFlags.NonPublic | BindingFlags.Static); + Delegate original = targetMethod.CreateDelegate(typeof(NonGenericDelegate), null); + IntPtr ptr = Marshal.GetFunctionPointerForDelegate(original); + Assert.NotEqual(IntPtr.Zero, ptr); + } + + [Fact] + [SkipOnPlatform(TestPlatforms.Browser, "Not supported on Browser.")] + public void GetFunctionPointerForDelegate_MarshalledClosedStaticDelegate() + { + MethodInfo targetMethod = typeof(GetFunctionPointerForDelegateTests).GetMethod(nameof(Method), BindingFlags.NonPublic | BindingFlags.Static); + Delegate original = targetMethod.CreateDelegate(typeof(NoArgsDelegate), "value"); + IntPtr ptr = Marshal.GetFunctionPointerForDelegate(original); + Assert.NotEqual(IntPtr.Zero, ptr); + } + public delegate void GenericDelegate(T t); public delegate void NonGenericDelegate(string t); + public delegate void NoArgsDelegate(); private static void Method(string s) { } } diff --git a/src/mono/mono/metadata/cominterop.c b/src/mono/mono/metadata/cominterop.c index ef16f24ffdd16..d28c6802842df 100644 --- a/src/mono/mono/metadata/cominterop.c +++ b/src/mono/mono/metadata/cominterop.c @@ -2089,7 +2089,7 @@ cominterop_get_ccw_method (MonoClass *iface, MonoMethod *method, MonoError *erro adjust_method = cominterop_get_managed_wrapper_adjusted (method); sig_adjusted = mono_method_signature_internal (adjust_method); - mspecs = g_new (MonoMarshalSpec*, sig_adjusted->param_count + 1); + mspecs = g_new0 (MonoMarshalSpec*, sig_adjusted->param_count + 1); mono_method_get_marshal_info (method, mspecs); /* move managed args up one */ diff --git a/src/mono/mono/metadata/marshal-ilgen.c b/src/mono/mono/metadata/marshal-ilgen.c index 974a5d1663dfc..7c03d64617e37 100644 --- a/src/mono/mono/metadata/marshal-ilgen.c +++ b/src/mono/mono/metadata/marshal-ilgen.c @@ -743,7 +743,7 @@ emit_native_wrapper_validate_signature (MonoMethodBuilder *mb, MonoMethodSignatu if (mspecs) { for (int i = 0; i < sig->param_count; i ++) { if (mspecs [i + 1] && mspecs [i + 1]->native == MONO_NATIVE_CUSTOM) { - if (!mspecs [i + 1]->data.custom_data.custom_name || strlen (mspecs [i + 1]->data.custom_data.custom_name) == 0) { + if (!mspecs [i + 1]->data.custom_data.custom_name || *mspecs [i + 1]->data.custom_data.custom_name == '\0') { mono_mb_emit_exception_full (mb, "System", "TypeLoadException", g_strdup ("Missing ICustomMarshaler type")); return FALSE; } @@ -5023,7 +5023,7 @@ emit_managed_wrapper_validate_signature (MonoMethodSignature* sig, MonoMarshalSp if (mspecs) { for (int i = 0; i < sig->param_count; i ++) { if (mspecs [i + 1] && mspecs [i + 1]->native == MONO_NATIVE_CUSTOM) { - if (!mspecs [i + 1]->data.custom_data.custom_name || strlen (mspecs [i + 1]->data.custom_data.custom_name) == 0) { + if (!mspecs [i + 1]->data.custom_data.custom_name || *mspecs [i + 1]->data.custom_data.custom_name == '\0') { mono_error_set_generic_error (error, "System", "TypeLoadException", "Missing ICustomMarshaler type"); return FALSE; } @@ -5067,8 +5067,21 @@ emit_managed_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *invoke_s sig = m->sig; csig = m->csig; - if (!emit_managed_wrapper_validate_signature (sig, mspecs, error)) + if (!sig->hasthis && sig->param_count != invoke_sig->param_count) { + /* Closed delegate */ + g_assert (sig->param_count == invoke_sig->param_count + 1); + closed = TRUE; + /* Use a new signature without the first argument */ + sig = mono_metadata_signature_dup (sig); + memmove (&sig->params [0], &sig->params [1], (sig->param_count - 1) * sizeof (MonoType*)); + sig->param_count --; + } + + if (!emit_managed_wrapper_validate_signature (sig, mspecs, error)) { + if (closed) + g_free (sig); return; + } MonoType *int_type = mono_get_int_type (); MonoType *boolean_type = m_class_get_byval_arg (mono_defaults.boolean_class); @@ -5079,16 +5092,6 @@ emit_managed_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *invoke_s /* allocate local 2 (boolean) delete_old */ mono_mb_add_local (mb, boolean_type); - if (!sig->hasthis && sig->param_count != invoke_sig->param_count) { - /* Closed delegate */ - g_assert (sig->param_count == invoke_sig->param_count + 1); - closed = TRUE; - /* Use a new signature without the first argument */ - sig = mono_metadata_signature_dup (sig); - memmove (&sig->params [0], &sig->params [1], (sig->param_count - 1) * sizeof (MonoType*)); - sig->param_count --; - } - if (!MONO_TYPE_IS_VOID(sig->ret)) { /* allocate local 3 to store the return value */ mono_mb_add_local (mb, sig->ret); diff --git a/src/mono/mono/metadata/marshal.c b/src/mono/mono/metadata/marshal.c index c0f1d70ba65f4..7659e943a5032 100644 --- a/src/mono/mono/metadata/marshal.c +++ b/src/mono/mono/metadata/marshal.c @@ -3561,7 +3561,7 @@ mono_marshal_get_native_wrapper (MonoMethod *method, gboolean check_exceptions, csig = mono_metadata_signature_dup_full (get_method_image (method), sig); mono_marshal_set_callconv_from_modopt (method, csig, FALSE); - mspecs = g_new (MonoMarshalSpec*, sig->param_count + 1); + mspecs = g_new0 (MonoMarshalSpec*, sig->param_count + 1); mono_method_get_marshal_info (method, mspecs); if (mono_class_try_get_suppress_gc_transition_attribute_class ()) {