From 4ae810ddb7684af1edd6a9a87dd65c072dd93994 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Tue, 18 Jan 2022 19:30:32 -0800 Subject: [PATCH 01/25] Add runtime support for ref fields --- src/coreclr/utilcode/util.cpp | 4 +- src/coreclr/vm/field.cpp | 11 ++- src/coreclr/vm/jitinterface.cpp | 3 - src/coreclr/vm/methodtablebuilder.cpp | 26 +++++- .../classloader/RefFields/InvalidCSharp.il | 92 +++++++++++++++++++ .../RefFields/InvalidCSharp.ilproj | 8 ++ .../Loader/classloader/RefFields/Validate.cs | 49 ++++++++++ .../classloader/RefFields/Validate.csproj | 12 +++ 8 files changed, 195 insertions(+), 10 deletions(-) create mode 100644 src/tests/Loader/classloader/RefFields/InvalidCSharp.il create mode 100644 src/tests/Loader/classloader/RefFields/InvalidCSharp.ilproj create mode 100644 src/tests/Loader/classloader/RefFields/Validate.cs create mode 100644 src/tests/Loader/classloader/RefFields/Validate.csproj diff --git a/src/coreclr/utilcode/util.cpp b/src/coreclr/utilcode/util.cpp index afc0c057d3ee5..25a47ec46a563 100644 --- a/src/coreclr/utilcode/util.cpp +++ b/src/coreclr/utilcode/util.cpp @@ -1805,9 +1805,7 @@ HRESULT validateOneArg( // Validate the referenced type. if(FAILED(hr = validateOneArg(tk, pSig, pulNSentinels, pImport, FALSE))) IfFailGo(hr); break; - case ELEMENT_TYPE_BYREF: //fallthru - if(TypeFromToken(tk)==mdtFieldDef) IfFailGo(VLDTR_E_SIG_BYREFINFIELD); - FALLTHROUGH; + case ELEMENT_TYPE_BYREF: case ELEMENT_TYPE_PINNED: case ELEMENT_TYPE_SZARRAY: // Validate the referenced type. diff --git a/src/coreclr/vm/field.cpp b/src/coreclr/vm/field.cpp index a0e1260976dee..d0f8660b470a4 100644 --- a/src/coreclr/vm/field.cpp +++ b/src/coreclr/vm/field.cpp @@ -61,6 +61,8 @@ VOID FieldDesc::Init(mdFieldDef mb, CorElementType FieldType, DWORD dwMemberAttr FieldType == ELEMENT_TYPE_R8 || FieldType == ELEMENT_TYPE_CLASS || FieldType == ELEMENT_TYPE_VALUETYPE || + FieldType == ELEMENT_TYPE_BYREF || + FieldType == ELEMENT_TYPE_TYPEDBYREF || FieldType == ELEMENT_TYPE_PTR || FieldType == ELEMENT_TYPE_FNPTR ); @@ -70,7 +72,8 @@ VOID FieldDesc::Init(mdFieldDef mb, CorElementType FieldType, DWORD dwMemberAttr m_requiresFullMbValue = 0; SetMemberDef(mb); - m_type = FieldType; + // A TypedByRef should be treated like a regular value type. + m_type = FieldType != ELEMENT_TYPE_TYPEDBYREF ? FieldType : ELEMENT_TYPE_VALUETYPE; m_prot = fdFieldAccessMask & dwMemberAttrs; m_isStatic = fIsStatic != 0; m_isRVA = fIsRVA != 0; @@ -81,7 +84,7 @@ VOID FieldDesc::Init(mdFieldDef mb, CorElementType FieldType, DWORD dwMemberAttr #endif _ASSERTE(GetMemberDef() == mb); // no truncation - _ASSERTE(GetFieldType() == FieldType); + _ASSERTE(GetFieldType() == FieldType || (FieldType == ELEMENT_TYPE_TYPEDBYREF && m_type == ELEMENT_TYPE_VALUETYPE)); _ASSERTE(GetFieldProtection() == (fdFieldAccessMask & dwMemberAttrs)); _ASSERTE((BOOL) IsStatic() == (fIsStatic != 0)); } @@ -140,7 +143,8 @@ TypeHandle FieldDesc::LookupFieldTypeHandle(ClassLoadLevel level, BOOL dropGener // Caller should have handled all the non-class cases, already. _ASSERTE(GetFieldType() == ELEMENT_TYPE_CLASS || - GetFieldType() == ELEMENT_TYPE_VALUETYPE); + GetFieldType() == ELEMENT_TYPE_VALUETYPE || + GetFieldType() == ELEMENT_TYPE_TYPEDBYREF); MetaSig sig(this); CorElementType type; @@ -152,6 +156,7 @@ TypeHandle FieldDesc::LookupFieldTypeHandle(ClassLoadLevel level, BOOL dropGener _ASSERTE(type == ELEMENT_TYPE_CLASS || type == ELEMENT_TYPE_VALUETYPE || type == ELEMENT_TYPE_STRING || + type == ELEMENT_TYPE_TYPEDBYREF || type == ELEMENT_TYPE_SZARRAY || type == ELEMENT_TYPE_VAR ); diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index e28434acabe56..f02693ae03421 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -9002,9 +9002,6 @@ CorInfoType CEEInfo::getFieldTypeInternal (CORINFO_FIELD_HANDLE fieldHnd, FieldDesc* field = (FieldDesc*) fieldHnd; CorElementType type = field->GetFieldType(); - // TODO should not burn the time to do this for anything but Value Classes - _ASSERTE(type != ELEMENT_TYPE_BYREF); - if (type == ELEMENT_TYPE_I) { PTR_MethodTable enclosingMethodTable = field->GetApproxEnclosingMethodTable(); diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 694e03cca35f2..ac7f8ecc8c0c3 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -3935,6 +3935,27 @@ VOID MethodTableBuilder::InitializeFieldDescs(FieldDesc *pFieldDescList, break; } + case ELEMENT_TYPE_BYREF: + { + dwLog2FieldSize = LOG2_PTRSIZE; + if (fIsStatic) + { + // Byref-like types cannot be used for static fields + BuildMethodTableThrowException(IDS_CLASSLOAD_BYREFLIKE_STATICFIELD); + } + if (!bmtFP->fIsByRefLikeType) + { + // Non-byref-like types cannot contain byref-like instance fields + BuildMethodTableThrowException(IDS_CLASSLOAD_BYREFLIKE_INSTANCEFIELD); + } + break; + } + + case ELEMENT_TYPE_TYPEDBYREF: + { + goto IS_VALUETYPE; + } + // Class type variable (method type variables aren't allowed in fields) // These only occur in open types used for verification/reflection. case ELEMENT_TYPE_VAR: @@ -4042,7 +4063,10 @@ VOID MethodTableBuilder::InitializeFieldDescs(FieldDesc *pFieldDescList, pByValueClass = (MethodTable *)-1; } } // If 'this' is a value class - + } + // TypedReference shares the rest of the code here +IS_VALUETYPE: + { // It's not self-referential so try to load it if (pByValueClass == NULL) { diff --git a/src/tests/Loader/classloader/RefFields/InvalidCSharp.il b/src/tests/Loader/classloader/RefFields/InvalidCSharp.il new file mode 100644 index 0000000000000..76b3417e0f554 --- /dev/null +++ b/src/tests/Loader/classloader/RefFields/InvalidCSharp.il @@ -0,0 +1,92 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +.assembly extern System.Runtime { .publickeytoken = (B0 3F 5F 7F 11 D5 0A 3A ) } + +.assembly InvalidCSharp { } + +.class public auto ansi sealed beforefieldinit InvalidCSharp.InvalidStructWithRefField + extends [System.Runtime]System.ValueType +{ + // Type requires IsByRefLikeAttribute to be valid. + .field public string& invalid +} + +// This is invalid metadata and is unable to be loaded. +// - [field sig] (0x80131815 (VER_E_FIELD_SIG)) +// +// .class public auto ansi sealed beforefieldinit InvalidCSharp.InvalidStructWithStaticRefField +// extends [System.Runtime]System.ValueType +// { +// .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( +// 01 00 00 00 +// ) +// .field public static string& invalid +// } + +.class public auto ansi sealed beforefieldinit InvalidCSharp.WithRefField + extends [System.Runtime]System.ValueType +{ + .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( + 01 00 00 00 + ) + .field public string& Str + + .method public hidebysig specialname rtspecialname + instance void .ctor ( + string& + ) cil managed + { + ldarg.0 + ldarg.1 + stfld string& InvalidCSharp.WithRefField::Str + ret + } + + .method public hidebysig + instance bool ConfirmFieldInstance ( + string + ) cil managed + { + ldarg.0 + ldfld string& InvalidCSharp.WithRefField::Str + ldind.ref + ldarg.1 + ceq + ret + } +} + +.class public auto ansi sealed beforefieldinit InvalidCSharp.WithRefStructField + extends [System.Runtime]System.ValueType +{ + .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( + 01 00 00 00 + ) + .field public valuetype InvalidCSharp.WithRefField& Field + + .method public hidebysig specialname rtspecialname + instance void .ctor ( + valuetype InvalidCSharp.WithRefField& + ) cil managed + { + ldarg.0 + ldarg.1 + stfld valuetype InvalidCSharp.WithRefField& InvalidCSharp.WithRefStructField::Field + ret + } + + .method public hidebysig + instance bool ConfirmFieldInstance ( + valuetype InvalidCSharp.WithRefField& + ) cil managed + { + ldarg.0 + ldfld valuetype InvalidCSharp.WithRefField& InvalidCSharp.WithRefStructField::Field + ldind.ref + ldarg.1 + ldind.ref + ceq + ret + } +} \ No newline at end of file diff --git a/src/tests/Loader/classloader/RefFields/InvalidCSharp.ilproj b/src/tests/Loader/classloader/RefFields/InvalidCSharp.ilproj new file mode 100644 index 0000000000000..d577c8f9c7a1a --- /dev/null +++ b/src/tests/Loader/classloader/RefFields/InvalidCSharp.ilproj @@ -0,0 +1,8 @@ + + + Library + + + + + diff --git a/src/tests/Loader/classloader/RefFields/Validate.cs b/src/tests/Loader/classloader/RefFields/Validate.cs new file mode 100644 index 0000000000000..33179e12309e2 --- /dev/null +++ b/src/tests/Loader/classloader/RefFields/Validate.cs @@ -0,0 +1,49 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.IO; +using InvalidCSharp; + +using Xunit; + +class Validate +{ + [Fact] + public static void Validate_Invalid_RefField_Fails() + { + Console.WriteLine($"{nameof(Validate_Invalid_RefField_Fails)}..."); + Assert.Throws(() => { var t = typeof(InvalidStructWithRefField); }); + } + + [Fact] + public static void Validate_RefStructWithRefField_Load() + { + Console.WriteLine($"{nameof(Validate_RefStructWithRefField_Load)}..."); + var t = typeof(WithRefField); + } + + [Fact] + public static void Validate_Create_RefField() + { + var str = nameof(Validate_Create_RefField); + Console.WriteLine($"{str}..."); + + WithRefField s = new WithRefField(ref str); + Assert.True(s.ConfirmFieldInstance(str)); + + var newStr = new string(str); + Assert.False(s.ConfirmFieldInstance(newStr)); + } + + [Fact] + public static void Validate_Create_RefStructField() + { + var str = nameof(Validate_Create_RefStructField); + Console.WriteLine($"{str}..."); + + WithRefField s = new WithRefField(ref str); + WithRefStructField t = new WithRefStructField(ref s); + Assert.True(t.ConfirmFieldInstance(ref s)); + } +} \ No newline at end of file diff --git a/src/tests/Loader/classloader/RefFields/Validate.csproj b/src/tests/Loader/classloader/RefFields/Validate.csproj new file mode 100644 index 0000000000000..96fedddd4bdda --- /dev/null +++ b/src/tests/Loader/classloader/RefFields/Validate.csproj @@ -0,0 +1,12 @@ + + + true + Exe + + + + + + + + From 34933ac957f4adaaeb992ef22e000f743ad99800 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 19 Jan 2022 08:25:46 -0800 Subject: [PATCH 02/25] Update Reflection.Emit tests to validate ref fields. --- .../ModuleBuilder/ModuleBuilderDefineEnum.cs | 4 +- .../TypeBuilder/TypeBuilderDefineField.cs | 44 +++++++++++++++---- .../System.Reflection.Emit/tests/Utilities.cs | 11 ++++- 3 files changed, 46 insertions(+), 13 deletions(-) diff --git a/src/libraries/System.Reflection.Emit/tests/ModuleBuilder/ModuleBuilderDefineEnum.cs b/src/libraries/System.Reflection.Emit/tests/ModuleBuilder/ModuleBuilderDefineEnum.cs index 34433ae11f391..e1d927cfa25b0 100644 --- a/src/libraries/System.Reflection.Emit/tests/ModuleBuilder/ModuleBuilderDefineEnum.cs +++ b/src/libraries/System.Reflection.Emit/tests/ModuleBuilder/ModuleBuilderDefineEnum.cs @@ -160,11 +160,11 @@ public void DefineEnum_VoidUnderlyingType_ThrowsArgumentException() } [Fact] - public void DefineEnum_ByRefUnderlyingType_ThrowsCOMExceptionOnCreation() + public void DefineEnum_ByRefUnderlyingType_ThrowsTypeLoadExceptionOnCreation() { ModuleBuilder module = Helpers.DynamicModule(); EnumBuilder enumBuilder = module.DefineEnum("Name", TypeAttributes.Public, typeof(int).MakeByRefType()); - Assert.Throws(() => enumBuilder.CreateTypeInfo()); + Assert.Throws(() => enumBuilder.CreateTypeInfo()); } [Theory] diff --git a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineField.cs b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineField.cs index e765a08dcc403..6126c5744e5f0 100644 --- a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineField.cs +++ b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineField.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Generic; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using Xunit; @@ -112,15 +113,6 @@ public void DefineField_VoidFieldType_ThrowsArgumentException() AssertExtensions.Throws(null, () => type.DefineField("Name", typeof(void), FieldAttributes.Public)); } - [Fact] - public void DefineField_ByRefFieldType_ThrowsCOMExceptionOnCreation() - { - TypeBuilder type = Helpers.DynamicType(TypeAttributes.Public); - type.DefineField("Name", typeof(int).MakeByRefType(), FieldAttributes.Public); - - Assert.Throws(() => type.CreateTypeInfo()); - } - [Theory] [ActiveIssue("https://github.com/dotnet/runtime/issues/2389", TestRuntimes.Mono)] [InlineData((FieldAttributes)(-1), (FieldAttributes)(-38145))] @@ -152,6 +144,40 @@ public void DefineField_DynamicFieldTypeNotCreated_ThrowsTypeLoadException() Assert.Equal(createdFieldType, field.FieldType); } + [Fact] + public void DefineByRefField_Class_ThrowsTypeLoadExceptionOnCreation() + { + TypeBuilder type = Helpers.DynamicType(TypeAttributes.Public); + type.DefineField("Name", typeof(int).MakeByRefType(), FieldAttributes.Public); + + Assert.Throws(() => type.CreateTypeInfo()); + } + + [Fact] + public void DefineByRefField_ValueType_NonByRefLike_ThrowsTypeLoadExceptionOnCreation() + { + TypeBuilder type = Helpers.DynamicType(TypeAttributes.Public, baseType: typeof(ValueType)); + type.DefineField("Name", typeof(int).MakeByRefType(), FieldAttributes.Public); + + Assert.Throws(() => type.CreateTypeInfo()); + } + + [Fact] + public void DefineByRefField_ValueType_ByRefLike() + { + TypeBuilder type = Helpers.DynamicType(TypeAttributes.Public, baseType: typeof(ValueType)); + type.DefineField("Name", typeof(int).MakeByRefType(), FieldAttributes.Public); + + // Define type to be ByRefLike + CustomAttributeBuilder ca = new(typeof(IsByRefLikeAttribute).GetConstructors()[0], new object[] { }); + type.SetCustomAttribute(ca); + + Type createdType = type.CreateTypeInfo().AsType(); + FieldInfo[] fields = createdType.GetFields(); + Assert.Equal(1, fields.Length); + Assert.True(fields[0].FieldType.IsByRef); + } + [Fact] public void GetField_TypeNotCreated_ThrowsNotSupportedException() { diff --git a/src/libraries/System.Reflection.Emit/tests/Utilities.cs b/src/libraries/System.Reflection.Emit/tests/Utilities.cs index 8e1c965e19a27..1e951be849a86 100644 --- a/src/libraries/System.Reflection.Emit/tests/Utilities.cs +++ b/src/libraries/System.Reflection.Emit/tests/Utilities.cs @@ -54,9 +54,16 @@ public static ModuleBuilder DynamicModule(string assemblyName = "TestAssembly", return DynamicAssembly(assemblyName).DefineDynamicModule(moduleName); } - public static TypeBuilder DynamicType(TypeAttributes attributes, string assemblyName = "TestAssembly", string moduleName = "TestModule", string typeName = "TestType") + public static TypeBuilder DynamicType(TypeAttributes attributes, string assemblyName = "TestAssembly", string moduleName = "TestModule", string typeName = "TestType", Type? baseType = null) { - return DynamicModule(assemblyName, moduleName).DefineType(typeName, attributes); + if (baseType is null) + { + return DynamicModule(assemblyName, moduleName).DefineType(typeName, attributes); + } + else + { + return DynamicModule(assemblyName, moduleName).DefineType(typeName, attributes, baseType); + } } public static EnumBuilder DynamicEnum(TypeAttributes visibility, Type underlyingType, string enumName = "TestEnum", string assemblyName = "TestAssembly", string moduleName = "TestModule") From 63391279f911180d144e48a895e1d93d2f07abb0 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 19 Jan 2022 08:35:05 -0800 Subject: [PATCH 03/25] Remove unnecessary addition in ASSERT. --- src/coreclr/vm/field.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/vm/field.cpp b/src/coreclr/vm/field.cpp index d0f8660b470a4..3855f4a22c0df 100644 --- a/src/coreclr/vm/field.cpp +++ b/src/coreclr/vm/field.cpp @@ -143,8 +143,7 @@ TypeHandle FieldDesc::LookupFieldTypeHandle(ClassLoadLevel level, BOOL dropGener // Caller should have handled all the non-class cases, already. _ASSERTE(GetFieldType() == ELEMENT_TYPE_CLASS || - GetFieldType() == ELEMENT_TYPE_VALUETYPE || - GetFieldType() == ELEMENT_TYPE_TYPEDBYREF); + GetFieldType() == ELEMENT_TYPE_VALUETYPE); MetaSig sig(this); CorElementType type; From 5fc6174656df75fa8208a68bf161bb9927a49458 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 19 Jan 2022 12:48:00 -0800 Subject: [PATCH 04/25] Mono should throw TypeLoadException for invalid ref field. Test skips Mono for loading invalid type with a ref field. --- .../src/System/Reflection/Emit/TypeBuilder.Mono.cs | 2 +- src/tests/Loader/classloader/RefFields/Validate.cs | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs index 1e73f98f1505f..992ccf8302b02 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs @@ -884,7 +884,7 @@ private bool has_ctor_method() if (fb == null) continue; if (fb.FieldType.IsByRef) - throw new COMException(); + throw new TypeLoadException(); } } diff --git a/src/tests/Loader/classloader/RefFields/Validate.cs b/src/tests/Loader/classloader/RefFields/Validate.cs index 33179e12309e2..45ef4fe28ef40 100644 --- a/src/tests/Loader/classloader/RefFields/Validate.cs +++ b/src/tests/Loader/classloader/RefFields/Validate.cs @@ -10,6 +10,7 @@ class Validate { [Fact] + [SkipOnMono("Mono doesn't validate ref field state during type load")] public static void Validate_Invalid_RefField_Fails() { Console.WriteLine($"{nameof(Validate_Invalid_RefField_Fails)}..."); @@ -29,10 +30,10 @@ public static void Validate_Create_RefField() var str = nameof(Validate_Create_RefField); Console.WriteLine($"{str}..."); - WithRefField s = new WithRefField(ref str); + WithRefField s = new(ref str); Assert.True(s.ConfirmFieldInstance(str)); - var newStr = new string(str); + string newStr = new(str); Assert.False(s.ConfirmFieldInstance(newStr)); } @@ -42,8 +43,8 @@ public static void Validate_Create_RefStructField() var str = nameof(Validate_Create_RefStructField); Console.WriteLine($"{str}..."); - WithRefField s = new WithRefField(ref str); - WithRefStructField t = new WithRefStructField(ref s); + WithRefField s = new(ref str); + WithRefStructField t = new(ref s); Assert.True(t.ConfirmFieldInstance(ref s)); } } \ No newline at end of file From 2792c743b734f205dae28c06fa744d164b0d0de9 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Thu, 20 Jan 2022 09:15:34 -0800 Subject: [PATCH 05/25] coreclr/ --- .../System/Reflection/Emit/ModuleBuilder.cs | 8 ++------ .../src/System/Reflection/Emit/TypeBuilder.cs | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs index fe5da8367cdd4..416f7e06d2e64 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs @@ -222,6 +222,8 @@ private int GetTokenFromTypeSpec(byte[] signature, int length) private int GetTypeRefNested(Type type, Module? refedModule, string? strRefedModuleFileName) { + type = type.IsByRef ? type.GetElementType()! : type; + // This function will generate correct TypeRef token for top level type and nested type. Type? enclosingType = type.DeclaringType; int tkResolution = 0; @@ -233,7 +235,6 @@ private int GetTypeRefNested(Type type, Module? refedModule, string? strRefedMod typeName = UnmangleTypeName(typeName); } - Debug.Assert(!type.IsByRef, "Must not be ByRef."); Debug.Assert(!type.IsGenericType || type.IsGenericTypeDefinition, "Must not have generic arguments."); ModuleBuilder thisModule = this; @@ -1085,11 +1086,6 @@ private int GetTypeTokenWorkerNoLock(Type type, bool getGenericDefinition) // multiple calles to this method with the same class have no additional side affects. // This function is optimized to use the TypeDef token if Type is within the same module. // We should also be aware of multiple dynamic modules and multiple implementation of Type!!! - if (type.IsByRef) - { - throw new ArgumentException(SR.Argument_CannotGetTypeTokenForByRef); - } - if ((type.IsGenericType && (!type.IsGenericTypeDefinition || !getGenericDefinition)) || type.IsGenericParameter || type.IsArray || diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs index 6d9b8121fa6b4..234b946d67a48 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs @@ -518,6 +518,10 @@ internal TypeBuilder( // cannot contain null in the interface list throw new ArgumentNullException(nameof(interfaces)); } + if (interfaces[i].IsByRef) + { + throw new ArgumentException(SR.Argument_CannotUseByRefType); + } } interfaceTokens = new int[interfaces.Length + 1]; for (i = 0; i < interfaces.Length; i++) @@ -1863,6 +1867,11 @@ private EventBuilder DefineEventNoLock(string name, EventAttributes attributes, tkType = m_module.GetTypeTokenInternal(eventtype); + // For compat, check for ByRef after acquiring the token. The primary + // compat issue here is throwing on a null instance. + if (eventtype.IsByRef) + throw new ArgumentException(SR.Argument_CannotUseByRefType); + // Internal helpers to define property records ModuleBuilder module = m_module; evToken = DefineEvent( @@ -1922,7 +1931,12 @@ private EventBuilder DefineEventNoLock(string name, EventAttributes attributes, int tkParent = 0; if (m_typeParent != null) + { + if (m_typeParent.IsByRef) + throw new ArgumentException(SR.Argument_CannotUseByRefType); + tkParent = m_module.GetTypeTokenInternal(m_typeParent); + } ModuleBuilder module = m_module; @@ -2132,6 +2146,10 @@ public void AddInterfaceImplementation([DynamicallyAccessedMembers(DynamicallyAc { throw new ArgumentNullException(nameof(interfaceType)); } + if (interfaceType.IsByRef) + { + throw new ArgumentException(SR.Argument_CannotUseByRefType); + } AssemblyBuilder.CheckContext(interfaceType); From 7a2d818299b9f602f906210082a20947cc72ca43 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Thu, 20 Jan 2022 09:15:53 -0800 Subject: [PATCH 06/25] libraries/ --- .../src/Resources/Strings.resx | 4 ++-- .../MethodBuilder/MethodBuilderByRefs.cs | 21 +++++++++++++++++++ .../tests/System.Reflection.Emit.Tests.csproj | 1 + .../TypeBuilderAddInterfaceImplementaion.cs | 2 +- 4 files changed, 25 insertions(+), 3 deletions(-) create mode 100644 src/libraries/System.Reflection.Emit/tests/MethodBuilder/MethodBuilderByRefs.cs diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index f98789424343b..04831004f8b83 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -903,8 +903,8 @@ Cannot use function evaluation to create a TypedReference object. - - Cannot get TypeToken for a ByRef type. + + Cannot use ByRef type. Cannot set parent to an interface. diff --git a/src/libraries/System.Reflection.Emit/tests/MethodBuilder/MethodBuilderByRefs.cs b/src/libraries/System.Reflection.Emit/tests/MethodBuilder/MethodBuilderByRefs.cs new file mode 100644 index 0000000000000..01baa3bc010d9 --- /dev/null +++ b/src/libraries/System.Reflection.Emit/tests/MethodBuilder/MethodBuilderByRefs.cs @@ -0,0 +1,21 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Linq; +using Xunit; + +namespace System.Reflection.Emit.Tests +{ + public class MethodBuilderByRefs + { + [Fact] + public void ByRef_Ldtoken() + { + TypeBuilder type = Helpers.DynamicType(TypeAttributes.Public); + MethodBuilder method = type.DefineMethod("TestMethod", MethodAttributes.Public, typeof(Type), Type.EmptyTypes); + ILGenerator ilg = method.GetILGenerator(); + ilg.Emit(OpCodes.Ldtoken, typeof(int).MakeByRefType()); + ilg.Emit(OpCodes.Ret); + } + } +} \ No newline at end of file diff --git a/src/libraries/System.Reflection.Emit/tests/System.Reflection.Emit.Tests.csproj b/src/libraries/System.Reflection.Emit/tests/System.Reflection.Emit.Tests.csproj index e5d46e991da82..d47bdd3466c79 100644 --- a/src/libraries/System.Reflection.Emit/tests/System.Reflection.Emit.Tests.csproj +++ b/src/libraries/System.Reflection.Emit/tests/System.Reflection.Emit.Tests.csproj @@ -33,6 +33,7 @@ + diff --git a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderAddInterfaceImplementaion.cs b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderAddInterfaceImplementaion.cs index 8d9adab4662e0..2e18694395eb3 100644 --- a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderAddInterfaceImplementaion.cs +++ b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderAddInterfaceImplementaion.cs @@ -62,7 +62,7 @@ public void AddInterfaceImplementation_NullInterfaceType_ThrowsArgumentNullExcep } [Fact] - public void AddInterfaceImplementation_ByRefInterfaceType_ThrowsArgumentExceptioN() + public void AddInterfaceImplementation_ByRefInterfaceType_ThrowsArgumentException() { TypeBuilder type = Helpers.DynamicType(TypeAttributes.Public); AssertExtensions.Throws(null, () => type.AddInterfaceImplementation(typeof(int).MakeByRefType())); From 4ff87f966b935b80f97a258301f42724b15fae55 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Thu, 20 Jan 2022 09:16:04 -0800 Subject: [PATCH 07/25] mono/ --- .../src/System/Reflection/Emit/TypeBuilder.Mono.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs index 992ccf8302b02..42ff2ed9e4391 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs @@ -274,7 +274,7 @@ public void AddInterfaceImplementation([DynamicallyAccessedMembers(DynamicallyAc if (interfaceType == null) throw new ArgumentNullException(nameof(interfaceType)); if (interfaceType.IsByRef) - throw new ArgumentException(SR.Argument_CannotGetTypeTokenForByRef); + throw new ArgumentException(SR.Argument_CannotUseByRefType); check_not_created(); if (interfaces != null) @@ -1572,7 +1572,7 @@ public EventBuilder DefineEvent(string name, EventAttributes attributes, Type ev throw new ArgumentNullException(nameof(eventtype)); check_not_created(); if (eventtype.IsByRef) - throw new ArgumentException(SR.Argument_CannotGetTypeTokenForByRef); + throw new ArgumentException(SR.Argument_CannotUseByRefType); EventBuilder res = new EventBuilder(this, name, attributes, eventtype); if (events != null) { From eafc0a891fc1d36bb9b3031a55c043239db31bd6 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Thu, 20 Jan 2022 09:16:38 -0800 Subject: [PATCH 08/25] tests/ Add test for TypedReference as a ref field. --- .../classloader/RefFields/InvalidCSharp.il | 31 +++++++++++++++++++ .../Loader/classloader/RefFields/Validate.cs | 14 +++++++-- 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/src/tests/Loader/classloader/RefFields/InvalidCSharp.il b/src/tests/Loader/classloader/RefFields/InvalidCSharp.il index 76b3417e0f554..b976227547def 100644 --- a/src/tests/Loader/classloader/RefFields/InvalidCSharp.il +++ b/src/tests/Loader/classloader/RefFields/InvalidCSharp.il @@ -89,4 +89,35 @@ ceq ret } +} + +.class public auto ansi sealed beforefieldinit InvalidCSharp.WithTypedReferenceField`1 + extends [System.Runtime]System.ValueType +{ + .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( + 01 00 00 00 + ) + .field public typedref Field + + .method public hidebysig specialname rtspecialname + instance void .ctor ( + !T + ) cil managed + { + ldarg.0 + ldarga.s 1 + mkrefany !T + stfld typedref InvalidCSharp.WithTypedReferenceField`1::Field + ret + } + + .method public hidebysig + instance class [System.Runtime]System.Type GetFieldType () cil managed + { + ldarg.0 + ldfld typedref InvalidCSharp.WithTypedReferenceField`1::Field + refanytype + call class [System.Runtime]System.Type [System.Runtime]System.Type::GetTypeFromHandle(valuetype [System.Runtime]System.RuntimeTypeHandle ) + ret + } } \ No newline at end of file diff --git a/src/tests/Loader/classloader/RefFields/Validate.cs b/src/tests/Loader/classloader/RefFields/Validate.cs index 45ef4fe28ef40..637ad7ede7290 100644 --- a/src/tests/Loader/classloader/RefFields/Validate.cs +++ b/src/tests/Loader/classloader/RefFields/Validate.cs @@ -9,8 +9,8 @@ class Validate { - [Fact] - [SkipOnMono("Mono doesn't validate ref field state during type load")] + // [Fact] + // [SkipOnMono("Mono doesn't validate ref field state during type load")] public static void Validate_Invalid_RefField_Fails() { Console.WriteLine($"{nameof(Validate_Invalid_RefField_Fails)}..."); @@ -47,4 +47,14 @@ public static void Validate_Create_RefStructField() WithRefStructField t = new(ref s); Assert.True(t.ConfirmFieldInstance(ref s)); } + + [Fact] + public static void Validate_Create_TypedReferenceRefField() + { + Console.WriteLine($"{nameof(Validate_Create_TypedReferenceRefField)}..."); + + Validate v = new(); + WithTypedReferenceField s = new(v); + Assert.Equal(typeof(Validate), s.GetFieldType()); + } } \ No newline at end of file From acff92a57e69b05d00b56285d5eca8d8d633ef35 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Thu, 20 Jan 2022 14:36:36 -0800 Subject: [PATCH 09/25] mono/ Handle error cases in type loading for ref fields. --- src/mono/mono/metadata/class-init.c | 96 ++++++++++++++++------------- src/mono/mono/metadata/class.c | 4 +- src/mono/mono/metadata/object.c | 1 + 3 files changed, 56 insertions(+), 45 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 6164476ba9a91..32d40dcb298c9 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -96,7 +96,7 @@ To load TextBoxBase<> to do so it must resolve the parent which is TextInput it must resolve TextInput<> and TextBox. To load TextBox it must resolve the parent which is TextBoxBase. -At this point the runtime must instantiate TextBoxBase. Both types are partially loaded +At this point the runtime must instantiate TextBoxBase. Both types are partially loaded at this point, iow, both are registered in the type map and both and a NULL parent. This means that the resulting generic instance will have a NULL parent, which is wrong and will cause breakage. @@ -108,7 +108,7 @@ static int record_gclass_instantiation; static GSList *gclass_recorded_list; typedef gboolean (*gclass_record_func) (MonoClass*, void*); -/* +/* * LOCKING: loader lock must be held until pairing disable_gclass_recording is called. */ static void @@ -117,7 +117,7 @@ enable_gclass_recording (void) ++record_gclass_instantiation; } -/* +/* * LOCKING: loader lock must be held since pairing enable_gclass_recording was called. */ static void @@ -364,6 +364,14 @@ mono_class_setup_fields (MonoClass *klass) g_free (type_name); break; } + if (field->type->byref__) { + if (!m_class_is_byreflike (klass)) { + char *class_name = mono_type_get_full_name (klass); + mono_class_set_type_load_failure (klass, "Type %s is not a ByRefLike type so ref field, '%s', is invalid", class_name, field->name); + g_free (class_name); + break; + } + } /* The def_value of fields is compute lazily during vtable creation */ } @@ -426,7 +434,7 @@ mono_class_create_from_typedef (MonoImage *image, guint32 type_token, MonoError guint tidx = mono_metadata_token_index (type_token); MonoGenericContext *context = NULL; const char *name, *nspace; - guint icount = 0; + guint icount = 0; MonoClass **interfaces; guint32 field_last, method_last; guint32 nesting_tokeen; @@ -447,7 +455,7 @@ mono_class_create_from_typedef (MonoImage *image, guint32 type_token, MonoError } mono_metadata_decode_row (tt, tidx - 1, cols, MONO_TYPEDEF_SIZE); - + name = mono_metadata_string_heap (image, cols [MONO_TYPEDEF_NAME]); nspace = mono_metadata_string_heap (image, cols [MONO_TYPEDEF_NAMESPACE]); @@ -528,7 +536,7 @@ mono_class_create_from_typedef (MonoImage *image, guint32 type_token, MonoError if (mono_class_is_gtd (klass)) disable_gclass_recording (fix_gclass_incomplete_instantiation, klass); - /* + /* * This might access klass->_byval_arg for recursion generated by generic constraints, * so it has to come after setup_mono_type (). */ @@ -621,7 +629,7 @@ mono_class_create_from_typedef (MonoImage *image, guint32 type_token, MonoError first_method_idx = cols [MONO_TYPEDEF_METHOD_LIST] - 1; mono_class_set_first_method_idx (klass, first_method_idx); - if (table_info_get_rows (tt) > tidx){ + if (table_info_get_rows (tt) > tidx){ mono_metadata_decode_row (tt, tidx, cols_next, MONO_TYPEDEF_SIZE); field_last = cols_next [MONO_TYPEDEF_FIELD_LIST] - 1; method_last = cols_next [MONO_TYPEDEF_METHOD_LIST] - 1; @@ -630,7 +638,7 @@ mono_class_create_from_typedef (MonoImage *image, guint32 type_token, MonoError method_last = table_info_get_rows (&image->tables [MONO_TABLE_METHOD]); } - if (cols [MONO_TYPEDEF_FIELD_LIST] && + if (cols [MONO_TYPEDEF_FIELD_LIST] && cols [MONO_TYPEDEF_FIELD_LIST] <= table_info_get_rows (&image->tables [MONO_TABLE_FIELD])) mono_class_set_field_count (klass, field_last - first_field_idx); if (cols [MONO_TYPEDEF_METHOD_LIST] <= table_info_get_rows (&image->tables [MONO_TABLE_METHOD])) @@ -680,8 +688,8 @@ mono_class_create_from_typedef (MonoImage *image, guint32 type_token, MonoError // compute is_byreflike if (m_class_is_valuetype (klass)) if (class_has_isbyreflike_attribute (klass)) - klass->is_byreflike = 1; - + klass->is_byreflike = 1; + mono_loader_unlock (); MONO_PROFILER_RAISE (class_loaded, (klass)); @@ -835,7 +843,7 @@ mono_class_create_generic_inst (MonoGenericClass *gclass) klass->name = gklass->name; klass->name_space = gklass->name_space; - + klass->image = gklass->image; klass->type_token = gklass->type_token; @@ -883,7 +891,7 @@ mono_class_create_generic_inst (MonoGenericClass *gclass) if (klass->enumtype) { /* - * For enums, gklass->fields might not been set, but instance_size etc. is + * For enums, gklass->fields might not been set, but instance_size etc. is * already set in mono_reflection_create_internal_class (). For non-enums, * these will be computed normally in mono_class_layout_fields (). */ @@ -930,7 +938,7 @@ mono_class_create_generic_inst (MonoGenericClass *gclass) ++class_ginst_count; inflated_classes_size += sizeof (MonoClassGenericInst); - + mono_loader_unlock (); return klass; @@ -988,10 +996,10 @@ class_kind_may_contain_generic_instances (MonoTypeKind kind) /** * mono_class_create_bounded_array: - * \param element_class element class + * \param element_class element class * \param rank the dimension of the array class * \param bounded whenever the array has non-zero bounds - * \returns A class object describing the array with element type \p element_type and + * \returns A class object describing the array with element type \p element_type and * dimension \p rank. */ MonoClass * @@ -1237,9 +1245,9 @@ mono_class_create_bounded_array (MonoClass *eclass, guint32 rank, gboolean bound /** * mono_class_create_array: - * \param element_class element class + * \param element_class element class * \param rank the dimension of the array class - * \returns A class object describing the array with element type \p element_type and + * \returns A class object describing the array with element type \p element_type and * dimension \p rank. */ MonoClass * @@ -1434,7 +1442,7 @@ mono_class_create_ptr (MonoType *type) } mono_image_unlock (image); } - + result = mm ? (MonoClass *)mono_mem_manager_alloc0 (mm, sizeof (MonoClassPointer)) : (MonoClass *)mono_image_alloc0 (image, sizeof (MonoClassPointer)); UnlockedAdd (&classes_size, sizeof (MonoClassPointer)); @@ -1559,7 +1567,7 @@ mono_class_create_fnptr (MonoMethodSignature *sig) } -static gboolean +static gboolean method_is_reabstracted (guint16 flags) { if ((flags & METHOD_ATTRIBUTE_ABSTRACT && flags & METHOD_ATTRIBUTE_FINAL)) @@ -1874,7 +1882,7 @@ mono_class_is_gparam_with_nonblittable_parent (MonoClass *klass) * \param field_offsets Offsets of the klass' fields relative to the start of layout_check * \param field_count Count of klass fields * \param invalid_field_offset When the layout is invalid it will be set to the offset of the field which is invalid - * + * * \return True if the layout of the struct is valid, otherwise false. */ static gboolean @@ -2026,7 +2034,7 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_ /* * Enable GC aware auto layout: in this mode, reference - * fields are grouped together inside objects, increasing collector + * fields are grouped together inside objects, increasing collector * performance. * Requires that all classes whose layout is known to native code be annotated * with [StructLayout (LayoutKind.Sequential)] @@ -2083,7 +2091,7 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_ blittable = FALSE; /* Compute klass->has_references */ - /* + /* * Process non-static fields first, since static fields might recursively * refer to the class itself. */ @@ -2159,7 +2167,7 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_ } size = mono_type_size (field->type, &align); - + /* FIXME (LAMESPEC): should we also change the min alignment according to pack? */ align = packing_size ? MIN (packing_size, align): align; /* if the field has managed references, we need to force-align it @@ -2180,7 +2188,7 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_ } instance_size = MAX (real_size, instance_size); - + if (instance_size & (min_align - 1)) { instance_size += min_align - 1; instance_size &= ~(min_align - 1); @@ -2260,7 +2268,7 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_ * enable it when a MONO_DEBUG property is set. * * For small structs, set min_align to at least the struct size to improve - * performance, and since the JIT memset/memcpy code assumes this and generates + * performance, and since the JIT memset/memcpy code assumes this and generates * unaligned accesses otherwise. See #78990 for a testcase. */ if (mono_align_small_structs && top) { @@ -2326,7 +2334,7 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_ guint32 size; field = &klass->fields [i]; - + if (!(field->type->attrs & FIELD_ATTRIBUTE_STATIC) || field->type->attrs & FIELD_ATTRIBUTE_LITERAL) continue; if (mono_field_is_deleted (field)) @@ -2613,7 +2621,7 @@ generic_array_methods (MonoClass *klass) if (!iface || !check_method_exists (iface, mname)) continue; - + generic_array_method_info [i].array_method = m; name = (gchar *)mono_image_alloc (mono_defaults.corlib, strlen (iname) + strlen (mname) + 1); @@ -2719,7 +2727,7 @@ static guint32 mono_get_unique_iid (MonoClass *klass) { int iid; - + g_assert (MONO_CLASS_IS_INTERFACE_INTERNAL (klass)); classes_lock (); @@ -2780,8 +2788,8 @@ mono_get_unique_iid (MonoClass *klass) * mono_class_init_internal: * \param klass the class to initialize * - * Compute the \c instance_size, \c class_size and other infos that cannot be - * computed at \c mono_class_get time. Also compute vtable_size if possible. + * Compute the \c instance_size, \c class_size and other infos that cannot be + * computed at \c mono_class_get time. Also compute vtable_size if possible. * Initializes the following fields in \p klass: * - all the fields initialized by \c mono_class_init_sizes * - has_cctor @@ -2803,7 +2811,7 @@ mono_class_init_internal (MonoClass *klass) gboolean ghcimpl = FALSE; gboolean has_cctor = FALSE; int first_iface_slot = 0; - + g_assert (klass); /* Double-checking locking pattern */ @@ -2834,7 +2842,7 @@ mono_class_init_internal (MonoClass *klass) MonoClass *element_class = klass->element_class; MonoClass *cast_class = klass->cast_class; - if (!element_class->inited) + if (!element_class->inited) mono_class_init_internal (element_class); if (mono_class_set_type_load_failure_causedby_class (klass, element_class, "Could not load array element class")) goto leave; @@ -2871,7 +2879,7 @@ mono_class_init_internal (MonoClass *klass) initialize_object_slots (klass); - /* + /* * Initialize the rest of the data without creating a generic vtable if possible. * If possible, also compute vtable_size, so mono_class_create_runtime_vtable () can * also avoid computing a generic vtable. @@ -2942,7 +2950,7 @@ mono_class_init_internal (MonoClass *klass) int mcount = mono_class_get_method_count (klass); for (i = 0; i < mcount; ++i) { MonoMethod *method = klass->methods [i]; - if ((method->flags & METHOD_ATTRIBUTE_SPECIAL_NAME) && + if ((method->flags & METHOD_ATTRIBUTE_SPECIAL_NAME) && (strcmp (".cctor", method->name) == 0)) { has_cctor = 1; break; @@ -3042,7 +3050,7 @@ mono_class_init_checked (MonoClass *klass, MonoError *error) /* * COM initialization is delayed until needed. * However when a [ComImport] attribute is present on a type it will trigger - * the initialization. This is not a problem unless the BCL being executed + * the initialization. This is not a problem unless the BCL being executed * lacks the types that COM depends on (e.g. Variant on Silverlight). */ static void @@ -3106,13 +3114,13 @@ mono_class_setup_parent (MonoClass *klass, MonoClass *parent) if (MONO_CLASS_IS_IMPORT (klass) || mono_class_is_com_object (parent)) mono_class_set_is_com_object (klass); - + if (system_namespace) { - if (klass->name [0] == 'D' && !strcmp (klass->name, "Delegate")) + if (klass->name [0] == 'D' && !strcmp (klass->name, "Delegate")) klass->delegate = 1; } - if (klass->parent->enumtype || (mono_is_corlib_image (klass->parent->image) && (strcmp (klass->parent->name, "ValueType") == 0) && + if (klass->parent->enumtype || (mono_is_corlib_image (klass->parent->image) && (strcmp (klass->parent->name, "ValueType") == 0) && (strcmp (klass->parent->name_space, "System") == 0))) klass->valuetype = 1; if (mono_is_corlib_image (klass->parent->image) && ((strcmp (klass->parent->name, "Enum") == 0) && (strcmp (klass->parent->name_space, "System") == 0))) { @@ -3206,7 +3214,7 @@ mono_class_setup_mono_type (MonoClass *klass) t = MONO_TYPE_BOOLEAN; } else if (!strcmp(name, "Byte")) { t = MONO_TYPE_U1; - klass->blittable = TRUE; + klass->blittable = TRUE; } break; case 'C': @@ -3217,7 +3225,7 @@ mono_class_setup_mono_type (MonoClass *klass) case 'D': if (!strcmp (name, "Double")) { t = MONO_TYPE_R8; - klass->blittable = TRUE; + klass->blittable = TRUE; } break; case 'I': @@ -3238,7 +3246,7 @@ mono_class_setup_mono_type (MonoClass *klass) case 'S': if (!strcmp (name, "Single")) { t = MONO_TYPE_R4; - klass->blittable = TRUE; + klass->blittable = TRUE; } else if (!strcmp(name, "SByte")) { t = MONO_TYPE_I1; klass->blittable = TRUE; @@ -3307,7 +3315,7 @@ create_array_method (MonoClass *klass, const char *name, MonoMethodSignature *si * @class: a class * * Initializes the 'methods' array in CLASS. - * Calling this method should be avoided if possible since it allocates a lot + * Calling this method should be avoided if possible since it allocates a lot * of long-living MonoMethod structures. * Methods belonging to an interface are assigned a sequential slot starting * from 0. @@ -3346,7 +3354,7 @@ mono_class_setup_methods (MonoClass *klass) g_free (method); mono_error_cleanup (error); - return; + return; } } } else if (klass->rank) { @@ -3919,7 +3927,7 @@ mono_class_setup_has_finalizer (MonoClass *klass) * hierarchy + 1 * - supertypes: array of classes: each element has a class in the hierarchy * starting from @class up to System.Object - * + * * LOCKING: Acquires the loader lock. */ void diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index 04af2642f252d..2e366612a6184 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -4627,7 +4627,9 @@ mono_ldtoken_checked (MonoImage *image, guint32 token, MonoClass **handle_class, if (!type) return NULL; - mono_class_init_internal (mono_class_from_mono_type_internal (type)); + if (!mono_class_init_internal (mono_class_from_mono_type_internal (type))) + return NULL; + /* We return a MonoType* as handle */ return type; } diff --git a/src/mono/mono/metadata/object.c b/src/mono/mono/metadata/object.c index 284e15b1ade8e..e7a1e9a47c174 100644 --- a/src/mono/mono/metadata/object.c +++ b/src/mono/mono/metadata/object.c @@ -886,6 +886,7 @@ compute_class_bitmap (MonoClass *klass, gsize *bitmap, int size, int offset, int } else { /* fall through */ } + case MONO_TYPE_TYPEDBYREF: case MONO_TYPE_VALUETYPE: { MonoClass *fclass = mono_class_from_mono_type_internal (field->type); if (m_class_has_references (fclass)) { From f2db03bab62375716de2987d5ed4e5c91c8272e4 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Thu, 20 Jan 2022 14:36:52 -0800 Subject: [PATCH 10/25] tests/ --- src/tests/Loader/classloader/RefFields/Validate.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/tests/Loader/classloader/RefFields/Validate.cs b/src/tests/Loader/classloader/RefFields/Validate.cs index 637ad7ede7290..bffcd53de2f00 100644 --- a/src/tests/Loader/classloader/RefFields/Validate.cs +++ b/src/tests/Loader/classloader/RefFields/Validate.cs @@ -9,8 +9,7 @@ class Validate { - // [Fact] - // [SkipOnMono("Mono doesn't validate ref field state during type load")] + [Fact] public static void Validate_Invalid_RefField_Fails() { Console.WriteLine($"{nameof(Validate_Invalid_RefField_Fails)}..."); From 091a48bb172033ec8475ec2e4d3b44628c644094 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Thu, 20 Jan 2022 14:47:45 -0800 Subject: [PATCH 11/25] mono/ Remove IsByRef check for opcode emit. --- .../src/System/Reflection/Emit/ILGenerator.Mono.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ILGenerator.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ILGenerator.Mono.cs index 51edf3f644875..64585857fffc0 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ILGenerator.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ILGenerator.Mono.cs @@ -819,9 +819,6 @@ public virtual void Emit(OpCode opcode, string str) public virtual void Emit(OpCode opcode, Type cls) { - if (cls != null && cls.IsByRef) - throw new ArgumentException("Cannot get TypeToken for a ByRef type."); - make_room(6); ll_emit(opcode); int token = token_gen.GetToken(cls!, opcode != OpCodes.Ldtoken); From a4604e66fea4620e707e28138be93ec75cbcae6d Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Thu, 20 Jan 2022 16:36:19 -0800 Subject: [PATCH 12/25] mono/ Remove block for byref field. --- .../src/System/Reflection/Emit/TypeBuilder.Mono.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs index 42ff2ed9e4391..7a6cc2f462b20 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs @@ -883,8 +883,6 @@ private bool has_ctor_method() { if (fb == null) continue; - if (fb.FieldType.IsByRef) - throw new TypeLoadException(); } } From f2ec70c3cf36144ae2b9c68558db8ab02fe2ca3b Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Thu, 20 Jan 2022 19:00:03 -0800 Subject: [PATCH 13/25] mono/ Validate ref fields in SRE. --- .../src/System/Reflection/Emit/TypeBuilder.Mono.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs index 7a6cc2f462b20..962b92e9ee511 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs @@ -80,6 +80,7 @@ public sealed partial class TypeBuilder : TypeInfo private ITypeName fullname; private bool createTypeCalled; + private bool isByRefLike; private Type? underlying_type; public const int UnspecifiedTypeSize = 0; @@ -879,11 +880,18 @@ private bool has_ctor_method() if (fields != null) { + bool containsRefField = false; foreach (FieldBuilder fb in fields) { if (fb == null) continue; + containsRefField = fb.FieldType.IsByRef; } + + // If a type has a ref field but isn't a ByRefLike + // value type, then the type is invalid. + if (containsRefField && !(IsValueType && isByRefLike)) + throw new TypeLoadException(); } if (methods != null) @@ -1542,6 +1550,10 @@ public void SetCustomAttribute(CustomAttributeBuilder customBuilder) { attrs |= TypeAttributes.HasSecurity; } + else if (attrname == "System.Runtime.CompilerServices.IsByRefLikeAttribute") + { + isByRefLike = true; + } if (cattrs != null) { From 3702e57baf85d0c42fbbe401670d58f8ee00d57c Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 21 Jan 2022 07:01:30 -0800 Subject: [PATCH 14/25] mono/ Return error when class init fails during TYPEDEF, TYPEREF, TYPESPEC. --- src/mono/mono/metadata/class.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index bfdccc51b1e4b..eceb20cf4d0f6 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -4626,14 +4626,19 @@ mono_ldtoken_checked (MonoImage *image, guint32 token, MonoClass **handle_class, case MONO_TOKEN_TYPE_REF: case MONO_TOKEN_TYPE_SPEC: { MonoType *type; + MonoClass *klass; if (handle_class) *handle_class = mono_defaults.typehandle_class; type = mono_type_get_checked (image, token, context, error); if (!type) return NULL; - if (!mono_class_init_internal (mono_class_from_mono_type_internal (type))) + klass = mono_class_from_mono_type_internal (type); + mono_class_init_internal (klass); + if (mono_class_has_failure (klass)) { + mono_error_set_for_class_failure (error, klass); return NULL; + } /* We return a MonoType* as handle */ return type; From 87a3e30eb937ab1df92c1766cb7cae447e32207a Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 21 Jan 2022 09:34:51 -0800 Subject: [PATCH 15/25] mono/ Using helper instead of checking field. --- src/mono/mono/metadata/class-init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index e7be9577017b9..e94234fa48f3e 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -364,7 +364,7 @@ mono_class_setup_fields (MonoClass *klass) g_free (type_name); break; } - if (field->type->byref__) { + if (m_type_is_byref (field->type)) { if (!m_class_is_byreflike (klass)) { char *class_name = mono_type_get_full_name (klass); mono_class_set_type_load_failure (klass, "Type %s is not a ByRefLike type so ref field, '%s', is invalid", class_name, field->name); From 4fe2045e11b2ca9a90b591eca6da5d1cd097c759 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 21 Jan 2022 12:43:00 -0800 Subject: [PATCH 16/25] mono/ In SRE, propagate the IsByRefLike value during Type creation. --- .../Reflection/Emit/TypeBuilder.Mono.cs | 6 ++-- src/mono/mono/metadata/object-internals.h | 35 ++++++++++--------- src/mono/mono/metadata/sre.c | 1 + 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs index 962b92e9ee511..971d70ca0e846 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs @@ -74,13 +74,13 @@ public sealed partial class TypeBuilder : TypeInfo [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] private TypeInfo? created; + private bool is_byreflike; private int state; #endregion private ITypeName fullname; private bool createTypeCalled; - private bool isByRefLike; private Type? underlying_type; public const int UnspecifiedTypeSize = 0; @@ -890,7 +890,7 @@ private bool has_ctor_method() // If a type has a ref field but isn't a ByRefLike // value type, then the type is invalid. - if (containsRefField && !(IsValueType && isByRefLike)) + if (containsRefField && !(IsValueType && is_byreflike)) throw new TypeLoadException(); } @@ -1552,7 +1552,7 @@ public void SetCustomAttribute(CustomAttributeBuilder customBuilder) } else if (attrname == "System.Runtime.CompilerServices.IsByRefLikeAttribute") { - isByRefLike = true; + is_byreflike = true; } if (cattrs != null) diff --git a/src/mono/mono/metadata/object-internals.h b/src/mono/mono/metadata/object-internals.h index c1c4d939df2a1..2dde023dd8385 100644 --- a/src/mono/mono/metadata/object-internals.h +++ b/src/mono/mono/metadata/object-internals.h @@ -93,7 +93,7 @@ mono_array_new_specific_handle (MonoVTable *vtable, uintptr_t n, MonoError *erro MonoArray* mono_array_new_specific_checked (MonoVTable *vtable, uintptr_t n, MonoError *error); -/* +/* * Macros which cache. * These should be used instead of the original versions. */ @@ -170,7 +170,7 @@ struct _MonoArray { /* bounds is NULL for szarrays */ MonoArrayBounds *bounds; /* total number of elements of the array */ - mono_array_size_t max_length; + mono_array_size_t max_length; /* we use mono_64bitaligned_t to ensure proper alignment on platforms that need it */ mono_64bitaligned_t vector [MONO_ZERO_LEN_ARRAY]; }; @@ -469,7 +469,7 @@ typedef struct _MonoQCallAssemblyHandle MonoQCallAssemblyHandle; typedef struct { MonoObject object; - MonoReflectionType *class_to_proxy; + MonoReflectionType *class_to_proxy; MonoObject *context; MonoObject *unwrapped_server; gint32 target_domain_id; @@ -523,7 +523,7 @@ TYPED_HANDLE_DECL (MonoComInteropProxy); typedef struct { MonoObject object; - MonoRealProxy *rp; + MonoRealProxy *rp; MonoRemoteClass *remote_class; MonoBoolean custom_type_info; } MonoTransparentProxy; @@ -534,9 +534,9 @@ TYPED_HANDLE_DECL (MonoTransparentProxy); typedef struct { MonoObject obj; MonoReflectionMethod *method; - MonoArray *args; - MonoArray *names; - MonoArray *arg_types; + MonoArray *args; + MonoArray *names; + MonoArray *arg_types; MonoObject *ctx; MonoObject *rval; MonoObject *exc; @@ -664,7 +664,7 @@ TYPED_HANDLE_DECL (MonoDelegate); typedef void (*InterpJitInfoFunc) (MonoJitInfo *ji, gpointer user_data); -/* +/* * Callbacks supplied by the runtime and called by the modules in metadata/ * This interface is easier to extend than adding a new function type + * a new 'install' function for every callback. @@ -780,9 +780,9 @@ mono_domain_get_tls_offset (void); /* * Handling System.Type objects: * - * Fields defined as System.Type in managed code should be defined as MonoObject* - * in unmanaged structures, and the monotype_cast () function should be used for - * casting them to MonoReflectionType* to avoid crashes/security issues when + * Fields defined as System.Type in managed code should be defined as MonoObject* + * in unmanaged structures, and the monotype_cast () function should be used for + * casting them to MonoReflectionType* to avoid crashes/security issues when * encountering instances of user defined subclasses of System.Type. */ @@ -830,8 +830,8 @@ struct _MonoDelegate { gpointer delegate_trampoline; /* Extra argument passed to the target method in llvmonly mode */ gpointer extra_arg; - /* - * If non-NULL, this points to a memory location which stores the address of + /* + * If non-NULL, this points to a memory location which stores the address of * the compiled code of the method, or NULL if it is not yet compiled. */ guint8 **method_code; @@ -1138,7 +1138,7 @@ typedef struct { MonoArray *modopt; } MonoReflectionFieldBuilder; -/* Safely access System.Reflection.Emit.FieldBuilder from native code */ +/* Safely access System.Reflection.Emit.FieldBuilder from native code */ TYPED_HANDLE_DECL (MonoReflectionFieldBuilder); typedef struct { @@ -1222,6 +1222,7 @@ struct _MonoReflectionTypeBuilder { MonoGenericContainer *generic_container; MonoArray *generic_params; MonoReflectionType *created; + gboolean is_byreflike; gint32 state; }; @@ -1344,7 +1345,7 @@ typedef struct { MonoArray *refs; GSList *referenced_by; MonoReflectionType *owner; -} MonoReflectionDynamicMethod; +} MonoReflectionDynamicMethod; /* Safely access System.Reflection.Emit.DynamicMethod from native code */ TYPED_HANDLE_DECL (MonoReflectionDynamicMethod); @@ -1678,7 +1679,7 @@ mono_object_new_specific_checked (MonoVTable *vtable, MonoError *error); ICALL_EXPORT MonoObject * ves_icall_object_new (MonoClass *klass); - + ICALL_EXPORT MonoObject * ves_icall_object_new_specific (MonoVTable *vtable); @@ -1886,7 +1887,7 @@ MonoObject* mono_runtime_invoke_span_checked (MonoMethod *method, void *obj, MonoSpanOfObjects *params, MonoError *error); -void* +void* mono_compile_method_checked (MonoMethod *method, MonoError *error); MonoObject* diff --git a/src/mono/mono/metadata/sre.c b/src/mono/mono/metadata/sre.c index 912c4a9e94578..da0f0f829cab7 100644 --- a/src/mono/mono/metadata/sre.c +++ b/src/mono/mono/metadata/sre.c @@ -2554,6 +2554,7 @@ reflection_setup_internal_class_internal (MonoReflectionTypeBuilderHandle ref_tb goto_if_nok (error, leave); klass->type_token = MONO_TOKEN_TYPE_DEF | table_idx; mono_class_set_flags (klass, MONO_HANDLE_GETVAL (ref_tb, attrs)); + klass->is_byreflike = MONO_HANDLE_GETVAL (MONO_HANDLE_CAST (MonoReflectionTypeBuilder, ref_tb), is_byreflike); MONO_PROFILER_RAISE (class_loading, (klass)); From 66e774722a4185658bc78d822811d98afe992329 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 21 Jan 2022 12:43:19 -0800 Subject: [PATCH 17/25] Add test for SRE activation of types with ref fields. --- .../TypeBuilder/TypeBuilderDefineField.cs | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineField.cs b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineField.cs index 6126c5744e5f0..a09e69f6738ad 100644 --- a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineField.cs +++ b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineField.cs @@ -166,18 +166,47 @@ public void DefineByRefField_ValueType_NonByRefLike_ThrowsTypeLoadExceptionOnCre public void DefineByRefField_ValueType_ByRefLike() { TypeBuilder type = Helpers.DynamicType(TypeAttributes.Public, baseType: typeof(ValueType)); - type.DefineField("Name", typeof(int).MakeByRefType(), FieldAttributes.Public); // Define type to be ByRefLike CustomAttributeBuilder ca = new(typeof(IsByRefLikeAttribute).GetConstructors()[0], new object[] { }); type.SetCustomAttribute(ca); + type.DefineField("Name", typeof(int).MakeByRefType(), FieldAttributes.Public); + Type createdType = type.CreateTypeInfo().AsType(); FieldInfo[] fields = createdType.GetFields(); Assert.Equal(1, fields.Length); Assert.True(fields[0].FieldType.IsByRef); } + [Fact] + [ActiveIssue("https://github.com/dotnet/runtime/issues/45152")] + public void Instantiate_ValueType_With_ByRefField() + { + TypeBuilder type = Helpers.DynamicType(TypeAttributes.Public, baseType: typeof(ValueType)); + + // Define type to be ByRefLike + CustomAttributeBuilder ca = new(typeof(IsByRefLikeAttribute).GetConstructors()[0], new object[] { }); + type.SetCustomAttribute(ca); + + var field = type.DefineField("Name", typeof(int).MakeByRefType(), FieldAttributes.Public); + + var ctor = type.DefineConstructor(MethodAttributes.Public, CallingConventions.HasThis, new Type[] { typeof(string) }); + { + ILGenerator il = ctor.GetILGenerator(); + il.Emit(OpCodes.Ldarg_0); + il.Emit(OpCodes.Ldarga_S, 1); + il.Emit(OpCodes.Stfld, field); + il.Emit(OpCodes.Ret); + } + + Type createdType = type.CreateTypeInfo().AsType(); + + var ctorToCall = createdType.GetConstructor(BindingFlags.Public | BindingFlags.Instance, new[] { typeof(string) }); + var str = "12345"; + ctorToCall.Invoke(new[] { str }); + } + [Fact] public void GetField_TypeNotCreated_ThrowsNotSupportedException() { From f49344170299eff385810e26c85ab54787ead7e5 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 21 Jan 2022 17:55:17 -0800 Subject: [PATCH 18/25] libraries/ Update TypeBuilder tests. --- .../TypeBuilder/TypeBuilderAddInterfaceImplementaion.cs | 7 ------- .../tests/TypeBuilder/TypeBuilderDefineEvent.cs | 6 +++--- .../tests/TypeBuilder/TypeBuilderDefineNestedType.cs | 8 +------- .../tests/TypeBuilder/TypeBuilderSetParent.cs | 4 ++-- 4 files changed, 6 insertions(+), 19 deletions(-) diff --git a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderAddInterfaceImplementaion.cs b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderAddInterfaceImplementaion.cs index 2e18694395eb3..da3e695dcd389 100644 --- a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderAddInterfaceImplementaion.cs +++ b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderAddInterfaceImplementaion.cs @@ -61,13 +61,6 @@ public void AddInterfaceImplementation_NullInterfaceType_ThrowsArgumentNullExcep AssertExtensions.Throws("interfaceType", () => type.AddInterfaceImplementation(null)); } - [Fact] - public void AddInterfaceImplementation_ByRefInterfaceType_ThrowsArgumentException() - { - TypeBuilder type = Helpers.DynamicType(TypeAttributes.Public); - AssertExtensions.Throws(null, () => type.AddInterfaceImplementation(typeof(int).MakeByRefType())); - } - [Fact] public void AddInterfaceImplementation_TypeAlreadyCreated_ThrowsInvalidOperationException() { diff --git a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineEvent.cs b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineEvent.cs index 2c7452339c9e8..59ebb7fc77bb4 100644 --- a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineEvent.cs +++ b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineEvent.cs @@ -106,11 +106,11 @@ public void DefineEvent_Invalid(string name, Type eventType, Type exceptionType) } [Fact] - public void DefineEvent_ByRefEventType_ThrowsArgumentException() + public void DefineEvent_ByRefEventType() { TypeBuilder type = Helpers.DynamicType(TypeAttributes.Class | TypeAttributes.Public); - - AssertExtensions.Throws(null, () => type.DefineEvent("Name", EventAttributes.None, typeof(int).MakeByRefType())); + type.DefineEvent("Name", EventAttributes.None, typeof(int).MakeByRefType()); + type.CreateTypeInfo().AsType(); } [Fact] diff --git a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineNestedType.cs b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineNestedType.cs index cf24e8d87adad..d71506544525d 100644 --- a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineNestedType.cs +++ b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineNestedType.cs @@ -210,15 +210,9 @@ public void DefineNestedType_NullInterface_ThrowsArgumentNullException() AssertExtensions.Throws("interfaces", () => type.DefineNestedType("Name", TypeAttributes.NestedPublic, typeof(object), new Type[] { null })); } - [Fact] - public void DefineNestedType_ByRefInterfaceType_ThrowsArgumentException() - { - TypeBuilder type = Helpers.DynamicType(TypeAttributes.Public); - AssertExtensions.Throws(null, () => type.DefineNestedType("Name", TypeAttributes.NestedPublic, typeof(object), new Type[] { typeof(int).MakeByRefType() })); - } - public static IEnumerable InvalidInterfaceType_TestData() { + yield return new object[] { typeof(int).MakeByRefType() }; yield return new object[] { typeof(EmptyNonGenericClass) }; yield return new object[] { typeof(EmptyNonGenericStruct) }; yield return new object[] { typeof(EmptyGenericClass) }; diff --git a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderSetParent.cs b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderSetParent.cs index 8a4e5ace9ec8d..5c985d52a4676 100644 --- a/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderSetParent.cs +++ b/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderSetParent.cs @@ -56,14 +56,14 @@ public void SetParent_InterfaceType_ThrowsArgumentException(TypeAttributes attri } [Fact] - public void SetParent_ByRefType_ThrowsArgumentExceptionOnCreation() + public void SetParent_ByRefType_ThrowsNotSupportedExceptionOnCreation() { TypeBuilder type = Helpers.DynamicType(TypeAttributes.Public); type.SetParent(typeof(int).MakeByRefType()); Assert.Equal(typeof(int).MakeByRefType(), type.BaseType); - AssertExtensions.Throws(null, () => type.CreateTypeInfo()); + Assert.Throws(() => type.CreateTypeInfo()); } [Fact] From 9804d2a5d1ae35cc570635a28a77cf168262c283 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 21 Jan 2022 17:55:41 -0800 Subject: [PATCH 19/25] coreclr/ Remove IsByRefLike checks in Reflection Emit code paths. --- .../src/System/Reflection/Emit/TypeBuilder.cs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs index 234b946d67a48..a242ec948896b 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs @@ -518,10 +518,6 @@ internal TypeBuilder( // cannot contain null in the interface list throw new ArgumentNullException(nameof(interfaces)); } - if (interfaces[i].IsByRef) - { - throw new ArgumentException(SR.Argument_CannotUseByRefType); - } } interfaceTokens = new int[interfaces.Length + 1]; for (i = 0; i < interfaces.Length; i++) @@ -1867,11 +1863,6 @@ private EventBuilder DefineEventNoLock(string name, EventAttributes attributes, tkType = m_module.GetTypeTokenInternal(eventtype); - // For compat, check for ByRef after acquiring the token. The primary - // compat issue here is throwing on a null instance. - if (eventtype.IsByRef) - throw new ArgumentException(SR.Argument_CannotUseByRefType); - // Internal helpers to define property records ModuleBuilder module = m_module; evToken = DefineEvent( @@ -1932,9 +1923,6 @@ private EventBuilder DefineEventNoLock(string name, EventAttributes attributes, int tkParent = 0; if (m_typeParent != null) { - if (m_typeParent.IsByRef) - throw new ArgumentException(SR.Argument_CannotUseByRefType); - tkParent = m_module.GetTypeTokenInternal(m_typeParent); } @@ -2146,10 +2134,6 @@ public void AddInterfaceImplementation([DynamicallyAccessedMembers(DynamicallyAc { throw new ArgumentNullException(nameof(interfaceType)); } - if (interfaceType.IsByRef) - { - throw new ArgumentException(SR.Argument_CannotUseByRefType); - } AssemblyBuilder.CheckContext(interfaceType); From 166e4f2d64bba4f20a4ac99e451d71ce914ff947 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 21 Jan 2022 17:56:30 -0800 Subject: [PATCH 20/25] Exclude the RefFields tests from running on llvmfullaot --- src/tests/issues.targets | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/tests/issues.targets b/src/tests/issues.targets index f6ffb0f93efa2..bceb8dc494d95 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -1329,7 +1329,7 @@ - + @@ -2586,6 +2586,9 @@ expected failure: overlapped structs fail at AOT compile time, not runtime + + expected failure: unsupported type with ref field fails at AOT compile time, not runtime + https://github.com/dotnet/runtime/issues/57361 From d6e7c0fda845bec01de47aa868b8ae1115be166c Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 21 Jan 2022 18:47:10 -0800 Subject: [PATCH 21/25] Instead of dropping the byref, compute a token from a typespec. --- .../System/Reflection/Emit/ModuleBuilder.cs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs index 416f7e06d2e64..2b74ec2004a6d 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs @@ -222,8 +222,6 @@ private int GetTokenFromTypeSpec(byte[] signature, int length) private int GetTypeRefNested(Type type, Module? refedModule, string? strRefedModuleFileName) { - type = type.IsByRef ? type.GetElementType()! : type; - // This function will generate correct TypeRef token for top level type and nested type. Type? enclosingType = type.DeclaringType; int tkResolution = 0; @@ -1082,14 +1080,16 @@ private int GetTypeTokenWorkerNoLock(Type type, bool getGenericDefinition) // instructions. Tokens are always relative to the Module. For example, // the token value for System.String is likely to be different from // Module to Module. Calling GetTypeToken will cause a reference to be - // added to the Module. This reference becomes a perminate part of the Module, - // multiple calles to this method with the same class have no additional side affects. - // This function is optimized to use the TypeDef token if Type is within the same module. - // We should also be aware of multiple dynamic modules and multiple implementation of Type!!! - if ((type.IsGenericType && (!type.IsGenericTypeDefinition || !getGenericDefinition)) || - type.IsGenericParameter || - type.IsArray || - type.IsPointer) + // added to the Module. This reference becomes a permanent part of the Module, + // multiple calls to this method with the same class have no additional side-effects. + // This function is optimized to use the TypeDef token if the Type is within the + // same module. We should also be aware of multiple dynamic modules and multiple + // implementations of a Type. + if ((type.IsGenericType && (!type.IsGenericTypeDefinition || !getGenericDefinition)) + || type.IsGenericParameter + || type.IsArray + || type.IsPointer + || type.IsByRef) { byte[] sig = SignatureHelper.GetTypeSigToken(this, type).InternalGetSignature(out int length); return GetTokenFromTypeSpec(sig, length); From 3059e77d215aa03bee8fd5900900968ab46ddea6 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 21 Jan 2022 18:54:12 -0800 Subject: [PATCH 22/25] mono/ Remove checks in SRE to match CoreCLR behavior and unit tests. --- .../src/Resources/Strings.resx | 3 --- .../Reflection/Emit/TypeBuilder.Mono.cs | 22 ++----------------- 2 files changed, 2 insertions(+), 23 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index 04831004f8b83..e9d5e447ee90f 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -903,9 +903,6 @@ Cannot use function evaluation to create a TypedReference object. - - Cannot use ByRef type. - Cannot set parent to an interface. diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs index 971d70ca0e846..fdc2d33140ebb 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs @@ -274,8 +274,7 @@ public void AddInterfaceImplementation([DynamicallyAccessedMembers(DynamicallyAc { if (interfaceType == null) throw new ArgumentNullException(nameof(interfaceType)); - if (interfaceType.IsByRef) - throw new ArgumentException(SR.Argument_CannotUseByRefType); + check_not_created(); if (interfaces != null) @@ -878,22 +877,6 @@ private bool has_ctor_method() } } - if (fields != null) - { - bool containsRefField = false; - foreach (FieldBuilder fb in fields) - { - if (fb == null) - continue; - containsRefField = fb.FieldType.IsByRef; - } - - // If a type has a ref field but isn't a ByRefLike - // value type, then the type is invalid. - if (containsRefField && !(IsValueType && is_byreflike)) - throw new TypeLoadException(); - } - if (methods != null) { bool is_concrete = !IsAbstract; @@ -1581,8 +1564,7 @@ public EventBuilder DefineEvent(string name, EventAttributes attributes, Type ev if (eventtype == null) throw new ArgumentNullException(nameof(eventtype)); check_not_created(); - if (eventtype.IsByRef) - throw new ArgumentException(SR.Argument_CannotUseByRefType); + EventBuilder res = new EventBuilder(this, name, attributes, eventtype); if (events != null) { From 8dca52e67055ab267b4cb92f4a93329557452ea0 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 21 Jan 2022 19:07:32 -0800 Subject: [PATCH 23/25] coreclr/ Add back assert. --- .../src/System/Reflection/Emit/ModuleBuilder.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs index 2b74ec2004a6d..f8b8c1efd35d0 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs @@ -233,6 +233,7 @@ private int GetTypeRefNested(Type type, Module? refedModule, string? strRefedMod typeName = UnmangleTypeName(typeName); } + Debug.Assert(!type.IsByRef, "Must not be ByRef. Get token from TypeSpec."); Debug.Assert(!type.IsGenericType || type.IsGenericTypeDefinition, "Must not have generic arguments."); ModuleBuilder thisModule = this; From 6a5ac574a3572b570c2e95ca42ebc59791b6210f Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Sat, 22 Jan 2022 14:00:11 -0800 Subject: [PATCH 24/25] mono/ Perform ref field validation in native SRE type loading logic. --- .../src/System/Reflection/Emit/TypeBuilder.Mono.cs | 6 ++++-- src/mono/mono/metadata/sre.c | 7 ++++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs index fdc2d33140ebb..26ae235064833 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs @@ -838,7 +838,7 @@ private bool has_ctor_method() if (parent != null) { if (parent.IsByRef) - throw new ArgumentException(); + throw new NotSupportedException(); if (IsInterface) throw new TypeLoadException(); } @@ -1535,7 +1535,9 @@ public void SetCustomAttribute(CustomAttributeBuilder customBuilder) } else if (attrname == "System.Runtime.CompilerServices.IsByRefLikeAttribute") { - is_byreflike = true; + // The IsByRefLike attribute only applies to value types and enums. + // This matches CoreCLR behavior. + is_byreflike = IsValueType || IsEnum; } if (cattrs != null) diff --git a/src/mono/mono/metadata/sre.c b/src/mono/mono/metadata/sre.c index d63fed42618f5..492494a844506 100644 --- a/src/mono/mono/metadata/sre.c +++ b/src/mono/mono/metadata/sre.c @@ -2555,7 +2555,7 @@ reflection_setup_internal_class_internal (MonoReflectionTypeBuilderHandle ref_tb klass->type_token = MONO_TOKEN_TYPE_DEF | table_idx; mono_class_set_flags (klass, MONO_HANDLE_GETVAL (ref_tb, attrs)); klass->is_byreflike = MONO_HANDLE_GETVAL (MONO_HANDLE_CAST (MonoReflectionTypeBuilder, ref_tb), is_byreflike); - + MONO_PROFILER_RAISE (class_loading, (klass)); klass->element_class = klass; @@ -3557,6 +3557,11 @@ typebuilder_setup_one_field (MonoDynamicImage *dynamic_image, MonoClass *klass, } } + if (m_type_is_byref (field->type) && !m_class_is_byreflike (klass)) { + mono_error_set_type_load_name (error, NULL, NULL, "Field '%s' is a byref in a non-byref-like type", field->name); + goto leave; + } + if ((fb->attrs & FIELD_ATTRIBUTE_HAS_FIELD_RVA) && (rva_data = fb->rva_data)) { char *base = mono_array_addr_internal (rva_data, char, 0); size_t size = mono_array_length_internal (rva_data); From 62e06ed5d3c1a6212dca1121e585e8c8dd936088 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Sat, 22 Jan 2022 14:19:25 -0800 Subject: [PATCH 25/25] mono/ Move IsByRefLike decision into native SRE code. --- .../src/System/Reflection/Emit/TypeBuilder.Mono.cs | 6 ++---- src/mono/mono/metadata/object-internals.h | 2 +- src/mono/mono/metadata/sre.c | 7 ++++++- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs index 26ae235064833..1209f517736a4 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs @@ -74,7 +74,7 @@ public sealed partial class TypeBuilder : TypeInfo [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] private TypeInfo? created; - private bool is_byreflike; + private bool is_byreflike_set; private int state; #endregion @@ -1535,9 +1535,7 @@ public void SetCustomAttribute(CustomAttributeBuilder customBuilder) } else if (attrname == "System.Runtime.CompilerServices.IsByRefLikeAttribute") { - // The IsByRefLike attribute only applies to value types and enums. - // This matches CoreCLR behavior. - is_byreflike = IsValueType || IsEnum; + is_byreflike_set = true; } if (cattrs != null) diff --git a/src/mono/mono/metadata/object-internals.h b/src/mono/mono/metadata/object-internals.h index 2dde023dd8385..efa4b0624548d 100644 --- a/src/mono/mono/metadata/object-internals.h +++ b/src/mono/mono/metadata/object-internals.h @@ -1222,7 +1222,7 @@ struct _MonoReflectionTypeBuilder { MonoGenericContainer *generic_container; MonoArray *generic_params; MonoReflectionType *created; - gboolean is_byreflike; + gboolean is_byreflike_set; gint32 state; }; diff --git a/src/mono/mono/metadata/sre.c b/src/mono/mono/metadata/sre.c index 492494a844506..62254b3fa2c3e 100644 --- a/src/mono/mono/metadata/sre.c +++ b/src/mono/mono/metadata/sre.c @@ -2554,7 +2554,6 @@ reflection_setup_internal_class_internal (MonoReflectionTypeBuilderHandle ref_tb goto_if_nok (error, leave); klass->type_token = MONO_TOKEN_TYPE_DEF | table_idx; mono_class_set_flags (klass, MONO_HANDLE_GETVAL (ref_tb, attrs)); - klass->is_byreflike = MONO_HANDLE_GETVAL (MONO_HANDLE_CAST (MonoReflectionTypeBuilder, ref_tb), is_byreflike); MONO_PROFILER_RAISE (class_loading, (klass)); @@ -3863,6 +3862,12 @@ ves_icall_TypeBuilder_create_runtime_class (MonoReflectionTypeBuilderHandle ref_ mono_class_setup_supertypes (klass); mono_class_setup_mono_type (klass); + /* Check if the type is marked as byreflike. + * The IsByRefLike attribute only applies to value types and enums. This matches CoreCLR behavior. + */ + if (klass->enumtype || klass->valuetype) + klass->is_byreflike = MONO_HANDLE_GETVAL (MONO_HANDLE_CAST (MonoReflectionTypeBuilder, ref_tb), is_byreflike_set); + /* enums are done right away */ if (!klass->enumtype) if (!ensure_runtime_vtable (klass, error))