From 11c929661154e34c53dc8c79e7beff9a0ecadb38 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Thu, 22 Aug 2024 16:10:20 -0400 Subject: [PATCH 1/3] [mono] inflate function pointer types; hang indirect call wrappers on the 1. We were missing a case for `MONO_TYPE_FNPTR` in `inflate_generic_type` 2. When an indirect call was inside of a method that was part of a generic class, we were creating wrapper methods for the indirect calls parented by the GTD type of the caller. This ended up making common_call_trampoline consider the wrapper as needing an mrgctx. But if the caller was actually a normal closed instance type no mrgctx is set up. So we end up dereferencing a null pointer. Instead, when we create an indirect call wrapper method (which we asserted has no type params and which is always just sets up the GC transition and forwards the arguments to the indirect callee - so it never depends on the generic context), make its parent be the class of the calling assembly. This is consistent with how we cache these indirect methods (in the calling image, by the signature of the indirect callee) - it has nothing to do with the caller class. Fixes https://github.com/dotnet/runtime/issues/106811 --- src/mono/mono/metadata/class.c | 15 +++++++++++++++ src/mono/mono/metadata/marshal.c | 2 +- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index 834cf8688cd3e..efde26c7d2e28 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -870,6 +870,21 @@ inflate_generic_type (MonoImage *image, MonoType *type, MonoGenericContext *cont nt->data.type = inflated; return nt; } + case MONO_TYPE_FNPTR: { + MonoMethodSignature *in_sig = type->data.method; + // quick bail out - if there are no type variables anywhere in the signature, + // there's nothing that could get inflated. + if (!in_sig->has_type_parameters) + return NULL; + MonoMethodSignature *new_sig = mono_inflate_generic_signature (in_sig, context, error); + if (!new_sig || !is_ok (error)) + return NULL; + if (new_sig == in_sig) + return type; + MonoType *nt = mono_metadata_type_dup (image, type); + nt->data.method = new_sig; + return nt; + } default: if (!changed) return NULL; diff --git a/src/mono/mono/metadata/marshal.c b/src/mono/mono/metadata/marshal.c index 5e8fcc4dfd408..d0e0b2327907c 100644 --- a/src/mono/mono/metadata/marshal.c +++ b/src/mono/mono/metadata/marshal.c @@ -3963,7 +3963,7 @@ mono_marshal_get_native_func_wrapper_indirect (MonoClass *caller_class, MonoMeth return res; char *name = mono_signature_to_name (sig, "wrapper_native_indirect"); - MonoMethodBuilder *mb = mono_mb_new (caller_class, name, MONO_WRAPPER_MANAGED_TO_NATIVE); + MonoMethodBuilder *mb = mono_mb_new (get_wrapper_target_class (image), name, MONO_WRAPPER_MANAGED_TO_NATIVE); mb->method->save_lmf = 1; WrapperInfo *info = mono_wrapper_info_create (mb, WRAPPER_SUBTYPE_NATIVE_FUNC_INDIRECT); From 43aac6cb5b54d2cce988f93f9969cddf876d551a Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 23 Aug 2024 09:34:12 -0400 Subject: [PATCH 2/3] pay attention to `changed` when inflating if the cmods changed, but the sig didn't we want to return `type`. return NULL only if there's an error or if nothing else changed --- src/mono/mono/metadata/class.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index efde26c7d2e28..abd0948e3a2f5 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -874,13 +874,23 @@ inflate_generic_type (MonoImage *image, MonoType *type, MonoGenericContext *cont MonoMethodSignature *in_sig = type->data.method; // quick bail out - if there are no type variables anywhere in the signature, // there's nothing that could get inflated. - if (!in_sig->has_type_parameters) - return NULL; + if (!in_sig->has_type_parameters) { + if (!changed) + return NULL; + else + return type; + } MonoMethodSignature *new_sig = mono_inflate_generic_signature (in_sig, context, error); - if (!new_sig || !is_ok (error)) + if ((!new_sig && !changed) || !is_ok (error)) { return NULL; - if (new_sig == in_sig) + } else if (!new_sig && changed) return type; + if (new_sig == in_sig) { + if (!changed) + return NULL; + else + return type; + } MonoType *nt = mono_metadata_type_dup (image, type); nt->data.method = new_sig; return nt; From 16c3b259159c38aaae9fab8410e37bf5ec2ca5af Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 23 Aug 2024 11:26:01 -0400 Subject: [PATCH 3/3] add regression test --- .../FunctionPointer/GenericFunctionPointer.cs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/tests/Interop/MarshalAPI/FunctionPointer/GenericFunctionPointer.cs b/src/tests/Interop/MarshalAPI/FunctionPointer/GenericFunctionPointer.cs index 14021dae9550e..9bc6d4b05fd6e 100644 --- a/src/tests/Interop/MarshalAPI/FunctionPointer/GenericFunctionPointer.cs +++ b/src/tests/Interop/MarshalAPI/FunctionPointer/GenericFunctionPointer.cs @@ -50,6 +50,11 @@ internal static unsafe void NonGenericCalli(void* fnptr, ref int val, float a { ((delegate* unmanaged)fnptr)(ref val, arg); } + + internal static unsafe int NonGenericCalliInNonGenericMethod(void* fnptr, float arg) + { + return ((delegate* unmanaged)fnptr)(arg); + } } struct BlittableGeneric @@ -99,6 +104,12 @@ public static void RunGenericFunctionPointerTest(float inVal) GenericCaller.NonGenericCalli((delegate* unmanaged)&UnmanagedExportedFunctionRefInt, ref outVar, inVal); } Assert.Equal(expectedValue, outVar); + + unsafe + { + outVar = GenericCaller.NonGenericCalliInNonGenericMethod((delegate* unmanaged)&UnmanagedExportedFunction, inVal); + } + Assert.Equal(expectedValue, outVar); } [ConditionalFact(nameof(CanRunInvalidGenericFunctionPointerTest))]