From 6166de40e07aaca44fe54e12546a667744630ecb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksey=20Kliger=20=28=CE=BBgeek=29?= Date: Sun, 16 Jul 2023 20:25:35 -0400 Subject: [PATCH] [mono] Some UnsafeAccessorAttribute support for methods and constructors (#88925) * first cut at UnsafeAccessorKind.Constructor * Implement UnsafeAccessorKind.Method and StaticMethod * remove debug output * change some asserts to BadImageFormatException * enable some tests * move CallAmbiguousMethod to a separate test case * Fixup error checking; forbid generics * match CoreCLR: callvirt for instance methods call for static * fixups to pass new tests from https://github.com/dotnet/runtime/pull/88268 * Make CallCtorAsMethod tests work Co-authored-by: Aaron Robinson --- src/mono/mono/metadata/loader.c | 16 ++ src/mono/mono/metadata/marshal-lightweight.c | 165 +++++++++++++++++- src/mono/mono/metadata/marshal.c | 5 +- src/mono/mono/metadata/unsafe-accessor.h | 10 ++ .../UnsafeAccessors/UnsafeAccessorsTests.cs | 29 ++- 5 files changed, 196 insertions(+), 29 deletions(-) diff --git a/src/mono/mono/metadata/loader.c b/src/mono/mono/metadata/loader.c index 31b5d751bed7f..b3b7bbd108608 100644 --- a/src/mono/mono/metadata/loader.c +++ b/src/mono/mono/metadata/loader.c @@ -972,6 +972,22 @@ method_from_memberref (MonoImage *image, guint32 idx, MonoGenericContext *typesp return NULL; } +MonoMethod* +mono_unsafe_accessor_find_ctor (MonoClass *in_class, MonoMethodSignature *sig, MonoClass *from_class, MonoError *error) +{ + // This doesn't work for constructors because find_method explicitly disallows ".ctor" and ".cctor" + //return find_method (in_class, /*ic*/NULL, name, sig, from_class, error); + return find_method_in_class (in_class, ".ctor", /*qname*/NULL, /*fqname*/NULL, sig, from_class, error); +} + +MonoMethod* +mono_unsafe_accessor_find_method (MonoClass *in_class, const char *name, MonoMethodSignature *sig, MonoClass *from_class, MonoError *error) +{ + // This doesn't work for constructors because find_method explicitly disallows ".ctor" and ".cctor" + return find_method (in_class, /*ic*/NULL, name, sig, from_class, error); +} + + static MonoMethod * method_from_methodspec (MonoImage *image, MonoGenericContext *context, guint32 idx, MonoError *error) { diff --git a/src/mono/mono/metadata/marshal-lightweight.c b/src/mono/mono/metadata/marshal-lightweight.c index b4d18d8e0580d..b117f545e14f9 100644 --- a/src/mono/mono/metadata/marshal-lightweight.c +++ b/src/mono/mono/metadata/marshal-lightweight.c @@ -2354,6 +2354,10 @@ emit_unsafe_accessor_field_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_ mono_mb_emit_exception_full (mb, "System", "MissingFieldException", g_strdup_printf("UnsafeAccessorKind does not match expected static modifier on field '%s' in '%s'", member_name, m_class_get_name (target_class))); return; } + if (is_field_static && m_field_get_parent (target_field) != target_class) { + // don't look up static fields using the inheritance hierarchy + mono_mb_emit_exception_full (mb, "System", "MissingFieldException", g_strdup_printf("Field '%s' not found in '%s'", member_name, m_class_get_name (target_class))); + } if (kind == MONO_UNSAFE_ACCESSOR_FIELD) mono_mb_emit_ldarg (mb, 0); @@ -2361,11 +2365,159 @@ emit_unsafe_accessor_field_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_ mono_mb_emit_byte (mb, CEE_RET); } +/* + * Given an accessor method signature (where the first arg is a target class) creates the signature + * of the expected member method (ie, with the first arg removed) + */ +static MonoMethodSignature * +method_sig_from_accessor_sig (MonoMethodBuilder *mb, gboolean hasthis, MonoMethodSignature *accessor_sig, MonoGenericContext *ctx) +{ + MonoMethodSignature *ret = mono_metadata_signature_dup_full (get_method_image (mb->method), accessor_sig); + g_assert (ret->param_count > 0); + ret->hasthis = hasthis; + for (int i = 1; i < ret->param_count; i++) + ret->params [i - 1] = ret->params [i]; + memset (&ret->params[ret->param_count - 1], 0, sizeof (MonoType)); // just in case + ret->param_count--; + return ret; +} + +/* + * Given an accessor method signature (where the return type is a target class) creates the signature + * of the expected constructor method (same args, but return type is void). + */ +static MonoMethodSignature * +ctor_sig_from_accessor_sig (MonoMethodBuilder *mb, MonoMethodSignature *accessor_sig, MonoGenericContext *ctx) +{ + MonoMethodSignature *ret = mono_metadata_signature_dup_full (get_method_image (mb->method), accessor_sig); + ret->hasthis = TRUE; /* ctors are considered instance methods */ + ret->ret = mono_get_void_type (); + return ret; +} + +static void +emit_unsafe_accessor_ldargs (MonoMethodBuilder *mb, MonoMethodSignature *accessor_sig, int skip_count) +{ + for (int i = skip_count; i < accessor_sig->param_count; i++) + mono_mb_emit_ldarg (mb, i); +} + +static gboolean +unsafe_accessor_target_type_forbidden (MonoType *target_type) +{ + switch (target_type->type) + { + case MONO_TYPE_VOID: + case MONO_TYPE_PTR: + case MONO_TYPE_FNPTR: + return TRUE; + default: + return FALSE; + } +} + +static void +emit_missing_method_error (MonoMethodBuilder *mb, MonoError *failure, const char *display_member_name) +{ + if (!is_ok (failure)) { + mono_mb_emit_exception_full (mb, "System", "MissingMethodException", g_strdup_printf ("Could not find %s due to: %s", display_member_name, mono_error_get_message (failure))); + } else { + mono_mb_emit_exception_full (mb, "System", "MissingMethodException", g_strdup_printf ("Could not find %s", display_member_name)); + } +} + +static void +emit_unsafe_accessor_ctor_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_method, MonoMethodSignature *sig, MonoGenericContext *ctx, MonoUnsafeAccessorKind kind, const char *member_name) +{ + g_assert (kind == MONO_UNSAFE_ACCESSOR_CTOR); + // null or empty string member name is ok for a constructor + if (!member_name || member_name[0] == '\0') + member_name = ".ctor"; + if (strcmp (member_name, ".ctor") != 0) { + mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "Invalid UnsafeAccessorAttribute for constructor."); + return; + } + + MonoType *target_type = sig->ret; // for constructors the return type is the target type + if (target_type == NULL || m_type_is_byref (target_type) || unsafe_accessor_target_type_forbidden (target_type)) { + mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "Invalid usage of UnsafeAccessorAttribute."); + return; + } + + MonoMethodSignature *member_sig = ctor_sig_from_accessor_sig (mb, sig, ctx); + + MonoClass *target_class = mono_class_from_mono_type_internal (target_type); + + ERROR_DECL(find_method_error); + MonoClass *in_class = mono_class_is_ginst (target_class) ? mono_class_get_generic_class (target_class)->container_class : target_class; + MonoMethod *target_method = mono_unsafe_accessor_find_ctor (in_class, member_sig, target_class, find_method_error); + if (!is_ok (find_method_error) || target_method == NULL) { + emit_missing_method_error (mb, find_method_error, "constructor"); + mono_error_cleanup (find_method_error); + return; + } + g_assert (target_method->klass == target_class); + + emit_unsafe_accessor_ldargs (mb, sig, 0); + + mono_mb_emit_op (mb, CEE_NEWOBJ, target_method); + mono_mb_emit_byte (mb, CEE_RET); +} + +static void +emit_unsafe_accessor_method_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_method, MonoMethodSignature *sig, MonoGenericContext *ctx, MonoUnsafeAccessorKind kind, const char *member_name) +{ + g_assert (kind == MONO_UNSAFE_ACCESSOR_METHOD || kind == MONO_UNSAFE_ACCESSOR_STATIC_METHOD); + g_assert (member_name != NULL); + + // We explicitly allow calling a constructor as if it was an instance method, but we need some hacks in a couple of places + gboolean ctor_as_method = !strcmp (member_name, ".ctor"); + + if (sig->param_count < 1 || sig->params[0] == NULL || unsafe_accessor_target_type_forbidden (sig->params[0])) { + mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "Invalid usage of UnsafeAccessorAttribute."); + return; + } + gboolean hasthis = kind == MONO_UNSAFE_ACCESSOR_METHOD; + MonoType *target_type = sig->params[0]; + + MonoMethodSignature *member_sig = method_sig_from_accessor_sig (mb, hasthis, sig, ctx); + + MonoClass *target_class = mono_class_from_mono_type_internal (target_type); + + if (hasthis && m_class_is_valuetype (target_class) && !m_type_is_byref (target_type)) { + // If the non-static method access is for a value type, the instance must be byref. + mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "Invalid usage of UnsafeAccessorAttribute."); + } + + ERROR_DECL(find_method_error); + MonoClass *in_class = mono_class_is_ginst (target_class) ? mono_class_get_generic_class (target_class)->container_class : target_class; + MonoMethod *target_method = NULL; + if (!ctor_as_method) + target_method = mono_unsafe_accessor_find_method (in_class, member_name, member_sig, target_class, find_method_error); + else + target_method = mono_unsafe_accessor_find_ctor (in_class, member_sig, target_class, find_method_error); + if (!is_ok (find_method_error) || target_method == NULL) { + emit_missing_method_error (mb, find_method_error, member_name); + mono_error_cleanup (find_method_error); + return; + } + if (!hasthis && target_method->klass != target_class) { + emit_missing_method_error (mb, find_method_error, member_name); + return; + } + g_assert (target_method->klass == target_class); // are instance methods allowed to be looked up using inheritance? + + emit_unsafe_accessor_ldargs (mb, sig, !hasthis ? 1 : 0); + + mono_mb_emit_op (mb, hasthis ? CEE_CALLVIRT : CEE_CALL, target_method); + mono_mb_emit_byte (mb, CEE_RET); +} + static void emit_unsafe_accessor_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethod *accessor_method, MonoMethodSignature *sig, MonoGenericContext *ctx, MonoUnsafeAccessorKind kind, const char *member_name) { - if (accessor_method->is_generic) { - mono_mb_emit_exception_full (mb, "System", "NotImplementedException", "UnsafeAccessor_Generics"); + if (accessor_method->is_inflated || accessor_method->is_generic || mono_class_is_ginst (accessor_method->klass) || ctx != NULL) { + mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "UnsafeAccessor_Generics"); return; } @@ -2380,16 +2532,15 @@ emit_unsafe_accessor_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethod *accessor_ emit_unsafe_accessor_field_wrapper (mb, accessor_method, sig, ctx, kind, member_name); return; case MONO_UNSAFE_ACCESSOR_CTOR: - // TODO - mono_mb_emit_exception_full (mb, "System", "NotImplementedException", "UnsafeAccessor"); + emit_unsafe_accessor_ctor_wrapper (mb, accessor_method, sig, ctx, kind, member_name); return; case MONO_UNSAFE_ACCESSOR_METHOD: case MONO_UNSAFE_ACCESSOR_STATIC_METHOD: - // TODO - mono_mb_emit_exception_full (mb, "System", "NotImplementedException", "UnsafeAccessor"); + emit_unsafe_accessor_method_wrapper (mb, accessor_method, sig, ctx, kind, member_name); return; default: - g_assert_not_reached(); // some unknown wrapper kind + mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "UnsafeAccessor_InvalidKindValue"); + return; } } diff --git a/src/mono/mono/metadata/marshal.c b/src/mono/mono/metadata/marshal.c index 0e48c3d024664..19ca301b56876 100644 --- a/src/mono/mono/metadata/marshal.c +++ b/src/mono/mono/metadata/marshal.c @@ -5140,7 +5140,7 @@ mono_marshal_get_unsafe_accessor_wrapper (MonoMethod *accessor_method, MonoUnsaf MonoMethod *orig_method = NULL; WrapperInfo *info; - if (member_name == NULL) + if (member_name == NULL && kind != MONO_UNSAFE_ACCESSOR_CTOR) member_name = accessor_method->name; /* @@ -5178,8 +5178,7 @@ mono_marshal_get_unsafe_accessor_wrapper (MonoMethod *accessor_method, MonoUnsaf } mono_mb_free (mb); - // TODO: remove before merging - mono_method_print_code (res); + /* mono_method_print_code (res); */ return res; } diff --git a/src/mono/mono/metadata/unsafe-accessor.h b/src/mono/mono/metadata/unsafe-accessor.h index dd50cc6ca97ab..99a8dc445afda 100644 --- a/src/mono/mono/metadata/unsafe-accessor.h +++ b/src/mono/mono/metadata/unsafe-accessor.h @@ -5,6 +5,10 @@ #ifndef __MONO_METADATA_UNSAFE_ACCESSOR_H__ #define __MONO_METADATA_UNSAFE_ACCESSOR_H__ +#include +#include +#include + /* keep in sync with System.Runtime.CompilerServices.UnsafeAccessorKind * https://github.com/dotnet/runtime/blob/a2c19cd005a1130ba7f921e0264287cfbfa8513c/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/UnsafeAccessorAttribute.cs#L9-L35 */ @@ -16,4 +20,10 @@ typedef enum { MONO_UNSAFE_ACCESSOR_STATIC_FIELD, } MonoUnsafeAccessorKind; +MonoMethod* +mono_unsafe_accessor_find_ctor (MonoClass *in_class, MonoMethodSignature *sig, MonoClass *from_class, MonoError *error); + +MonoMethod* +mono_unsafe_accessor_find_method (MonoClass *in_class, const char *name, MonoMethodSignature *sig, MonoClass *from_class, MonoError *error); + #endif /* __MONO_METADATA_UNSAFE_ACCESSOR_H__ */ diff --git a/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs b/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs index b155c0a445e07..65c50f0041956 100644 --- a/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs +++ b/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs @@ -91,7 +91,6 @@ struct UserDataValue extern static UserDataValue CallPrivateConstructorValue(string a); [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_CallDefaultCtorClass() { Console.WriteLine($"Running {nameof(Verify_CallDefaultCtorClass)}"); @@ -101,7 +100,6 @@ public static void Verify_CallDefaultCtorClass() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_CallCtorClass() { Console.WriteLine($"Running {nameof(Verify_CallCtorClass)}"); @@ -112,7 +110,6 @@ public static void Verify_CallCtorClass() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_CallCtorValue() { Console.WriteLine($"Running {nameof(Verify_CallCtorValue)}"); @@ -123,7 +120,6 @@ public static void Verify_CallCtorValue() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_CallCtorAsMethod() { Console.WriteLine($"Running {nameof(Verify_CallCtorAsMethod)}"); @@ -139,7 +135,6 @@ public static void Verify_CallCtorAsMethod() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_CallCtorAsMethodValue() { Console.WriteLine($"Running {nameof(Verify_CallCtorAsMethodValue)}"); @@ -166,7 +161,6 @@ public static void Verify_AccessStaticFieldClass() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_AccessFieldClass() { Console.WriteLine($"Running {nameof(Verify_AccessFieldClass)}"); @@ -202,7 +196,6 @@ public static void Verify_AccessFieldValue() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_AccessStaticMethodClass() { Console.WriteLine($"Running {nameof(Verify_AccessStaticMethodClass)}"); @@ -216,7 +209,6 @@ public static void Verify_AccessStaticMethodClass() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_AccessMethodClass() { Console.WriteLine($"Running {nameof(Verify_AccessMethodClass)}"); @@ -238,7 +230,6 @@ public static void Verify_AccessMethodClass() extern static void _mvv(UserDataClass d); [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_AccessStaticMethodVoidClass() { Console.WriteLine($"Running {nameof(Verify_AccessStaticMethodVoidClass)}"); @@ -251,7 +242,6 @@ public static void Verify_AccessStaticMethodVoidClass() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_AccessMethodVoidClass() { Console.WriteLine($"Running {nameof(Verify_AccessMethodVoidClass)}"); @@ -265,7 +255,6 @@ public static void Verify_AccessMethodVoidClass() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_AccessStaticMethodValue() { Console.WriteLine($"Running {nameof(Verify_AccessStaticMethodValue)}"); @@ -279,7 +268,6 @@ public static void Verify_AccessStaticMethodValue() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_AccessMethodValue() { Console.WriteLine($"Running {nameof(Verify_AccessMethodValue)}"); @@ -315,7 +303,6 @@ public static void Verify_IgnoreCustomModifier() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_PreciseMatchCustomModifier() { Console.WriteLine($"Running {nameof(Verify_PreciseMatchCustomModifier)}"); @@ -369,7 +356,6 @@ public static void Verify_ManagedUnmanagedFunctionPointersDontMatch() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_InvalidTargetUnsafeAccessor() { Console.WriteLine($"Running {nameof(Verify_InvalidTargetUnsafeAccessor)}"); @@ -404,9 +390,6 @@ public static void Verify_InvalidTargetUnsafeAccessor() isNativeAot ? null : UserDataClass.MethodPointerName, () => CallPointerMethod(null, null)); - Assert.Throws( - () => CallAmbiguousMethod(CallPrivateConstructorClass(), null)); - [UnsafeAccessor(UnsafeAccessorKind.Method, Name=DoesNotExist)] extern static void MethodNotFound(UserDataClass d); @@ -429,6 +412,15 @@ public static void Verify_InvalidTargetUnsafeAccessor() [UnsafeAccessor(UnsafeAccessorKind.StaticMethod, Name=UserDataClass.MethodPointerName)] extern static string CallPointerMethod(UserDataClass d, delegate* unmanaged[Stdcall] fptr); + } + + [Fact] + [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] + public static void Verify_InvalidTargetUnsafeAccessorAmbiguousMatch() + { + Assert.Throws( + () => CallAmbiguousMethod(CallPrivateConstructorClass(), null)); + // This is an ambiguous match since there are two methods each with two custom modifiers. // Therefore the default "ignore custom modifiers" logic fails. The fallback is for a // precise match and that also fails because the custom modifiers don't match precisely. @@ -452,7 +444,6 @@ class Invalid } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)] public static void Verify_InvalidUseUnsafeAccessor() { Console.WriteLine($"Running {nameof(Verify_InvalidUseUnsafeAccessor)}"); @@ -513,4 +504,4 @@ public static void Verify_InvalidUseUnsafeAccessor() [UnsafeAccessor(UnsafeAccessorKind.Method, Name=nameof(ToString))] extern static string LookUpFailsOnFunctionPointers(delegate* fptr); } -} \ No newline at end of file +}