From 6773a9269d348be8372d80e24994d6dcc007be3d Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Wed, 15 Nov 2023 23:19:47 -0800 Subject: [PATCH] Disallow arrays of System.Void The CoreCLR type loader allowed creating arrays of System.Void. Many operations with these invalid array types failed, often in inscrutable ways. For example, GetInterfaces() call failed with type load exception of IEnumerable type. The exact failure modes are different between runtimes. It is better to disallow creating these invalid array types in the first place, across all runtimes, to make the behavior robust and consistent. Related to #88620 Fixes #94086 --- src/coreclr/dlls/mscorrc/mscorrc.rc | 1 + src/coreclr/dlls/mscorrc/resource.h | 1 + .../src/System.Private.CoreLib.csproj | 1 - .../Runtime/General/RuntimeTypeHandleKey.cs | 37 ------------------- .../Reflection/Runtime/General/TypeUnifier.cs | 2 +- .../Runtime/TypeInfos/RuntimeTypeInfo.cs | 2 + .../CompilerTypeSystemContext.Validation.cs | 7 +++- src/coreclr/vm/clsload.cpp | 15 ++++---- src/coreclr/vm/gchelpers.cpp | 25 ++----------- .../System/ActivatorTests.cs | 1 - .../System/Type/TypeTests.cs | 12 ++---- src/mono/mono/metadata/class-init.c | 8 +--- src/mono/mono/metadata/icall.c | 4 +- .../CLR-x86-EJIT/v1-m10/b02353/b02353.cs | 3 -- 14 files changed, 30 insertions(+), 89 deletions(-) delete mode 100644 src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/RuntimeTypeHandleKey.cs diff --git a/src/coreclr/dlls/mscorrc/mscorrc.rc b/src/coreclr/dlls/mscorrc/mscorrc.rc index 0fbec6efabad13..3b9d7997bfbdc9 100644 --- a/src/coreclr/dlls/mscorrc/mscorrc.rc +++ b/src/coreclr/dlls/mscorrc/mscorrc.rc @@ -325,6 +325,7 @@ BEGIN IDS_CLASSLOAD_BADFORMAT "Could not load type '%1' from assembly '%2' because the format is invalid." IDS_CLASSLOAD_BYREFARRAY "Could not create array type '%1' from assembly '%2' because the element type is ByRef." IDS_CLASSLOAD_BYREFLIKEARRAY "Could not create array type '%1' from assembly '%2' because the element type is ByRef-like." + IDS_CLASSLOAD_VOIDARRAY "Could not create array type '%1' from assembly '%2' because the element type is System.Void." IDS_CLASSLOAD_FIELDTOOLARGE "Size of field of type '%1' from assembly '%2' is too large." IDS_CLASSLOAD_CANTEXTEND "Type '%1' from assembly '%2' cannot extend from any other type." IDS_CLASSLOAD_STATICVIRTUAL "Method '%3' in type '%1' from assembly '%2' cannot be a static and a virtual." diff --git a/src/coreclr/dlls/mscorrc/resource.h b/src/coreclr/dlls/mscorrc/resource.h index 5b7eb00ccba8e5..1a1466b854a0a8 100644 --- a/src/coreclr/dlls/mscorrc/resource.h +++ b/src/coreclr/dlls/mscorrc/resource.h @@ -123,6 +123,7 @@ #define IDS_CLASSLOAD_BADFORMAT 0x1774 #define IDS_CLASSLOAD_BYREFARRAY 0x1775 #define IDS_CLASSLOAD_BYREFLIKEARRAY 0x1776 +#define IDS_CLASSLOAD_VOIDARRAY 0x1777 #define IDS_CLASSLOAD_STATICVIRTUAL 0x1778 #define IDS_CLASSLOAD_REDUCEACCESS 0x1779 #define IDS_CLASSLOAD_BADPINVOKE 0x177a diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj b/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj index bd6f9a3127a34e..547aa6abaec3a1 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj @@ -423,7 +423,6 @@ - diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/RuntimeTypeHandleKey.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/RuntimeTypeHandleKey.cs deleted file mode 100644 index 7a109a5565beda..00000000000000 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/RuntimeTypeHandleKey.cs +++ /dev/null @@ -1,37 +0,0 @@ -// 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.Collections.Generic; -using System.Diagnostics; -using System.Reflection; - -namespace System.Reflection.Runtime.General -{ - internal struct RuntimeTypeHandleKey : IEquatable - { - public RuntimeTypeHandleKey(RuntimeTypeHandle typeHandle) - { - TypeHandle = typeHandle; - } - - public RuntimeTypeHandle TypeHandle { get; } - - public override bool Equals(object obj) - { - if (!(obj is RuntimeTypeHandleKey other)) - return false; - return Equals(other); - } - - public bool Equals(RuntimeTypeHandleKey other) - { - return TypeHandle.Equals(other.TypeHandle); - } - - public override int GetHashCode() - { - return TypeHandle.GetHashCode(); - } - } -} diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/TypeUnifier.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/TypeUnifier.cs index 61b55f25a4d193..390dae1da0f55e 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/TypeUnifier.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/TypeUnifier.cs @@ -222,7 +222,7 @@ private static void ValidateElementType(RuntimeTypeInfo elementType, bool multiD { Debug.Assert(multiDim || rank == 1); - if (elementType.IsByRef) + if (elementType.IsByRef || elementType.IsVoid) throw new TypeLoadException(SR.Format(SR.ArgumentException_InvalidArrayElementType, elementType)); } } diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/TypeInfos/RuntimeTypeInfo.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/TypeInfos/RuntimeTypeInfo.cs index ae4d21b5c4fe01..52b37ee97b239b 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/TypeInfos/RuntimeTypeInfo.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/TypeInfos/RuntimeTypeInfo.cs @@ -57,6 +57,8 @@ protected RuntimeTypeInfo() public bool IsGenericType => IsGenericTypeDefinition || IsConstructedGenericType; + public bool IsVoid => InternalTypeHandleIfAvailable.Equals(typeof(void).TypeHandle); + public abstract string Name { get; } public abstract Assembly Assembly { get; } diff --git a/src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Validation.cs b/src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Validation.cs index ff4404eba19377..9326fe5978d6fc 100644 --- a/src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Validation.cs +++ b/src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Validation.cs @@ -283,8 +283,11 @@ private static TypeDesc EnsureLoadableTypeUncached(TypeDesc type) ThrowHelper.ThrowTypeLoadException(ExceptionStringID.ClassLoadGeneral, type); } - // It might seem reasonable to disallow array of void, but the CLR doesn't prevent that too hard. - // E.g. "newarr void" will fail, but "newarr void[]" or "ldtoken void[]" will succeed. + if (parameterType.IsVoid) + { + // Arrays of System.Void are not allowed + ThrowHelper.ThrowTypeLoadException(ExceptionStringID.ClassLoadGeneral, type); + } } } else if (type.IsFunctionPointer) diff --git a/src/coreclr/vm/clsload.cpp b/src/coreclr/vm/clsload.cpp index 06775e004629d6..ae4aa4f71c9f30 100644 --- a/src/coreclr/vm/clsload.cpp +++ b/src/coreclr/vm/clsload.cpp @@ -2984,19 +2984,20 @@ TypeHandle ClassLoader::CreateTypeHandleForTypeKey(TypeKey* pKey, AllocMemTracke THROW_BAD_FORMAT_MAYBE((kind != ELEMENT_TYPE_SZARRAY) || rank == 1, BFA_SDARRAY_BADRANK, pLoaderModule); // Arrays of BYREFS not allowed - if (paramType.GetInternalCorElementType() == ELEMENT_TYPE_BYREF) + if (paramType.IsByRef()) { ThrowTypeLoadException(pKey, IDS_CLASSLOAD_BYREFARRAY); } // Arrays of ByRefLike types not allowed - MethodTable* pMT = paramType.GetMethodTable(); - if (pMT != NULL) + if (paramType.IsByRefLike()) { - if (pMT->IsByRefLike()) - { - ThrowTypeLoadException(pKey, IDS_CLASSLOAD_BYREFLIKEARRAY); - } + ThrowTypeLoadException(pKey, IDS_CLASSLOAD_BYREFLIKEARRAY); + } + + if (paramType.GetSignatureCorElementType() == ELEMENT_TYPE_VOID) + { + ThrowTypeLoadException(pKey, IDS_CLASSLOAD_VOIDARRAY); } if (rank > MAX_RANK) diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index 81839c06c5f7d1..6fa5e61c71a516 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -380,12 +380,6 @@ OBJECTREF AllocateSzArray(MethodTable* pArrayMT, INT32 cElements, GC_ALLOC_FLAGS _ASSERTE(pArrayMT->CheckInstanceActivated()); _ASSERTE(pArrayMT->GetInternalCorElementType() == ELEMENT_TYPE_SZARRAY); - CorElementType elemType = pArrayMT->GetArrayElementType(); - - // Disallow the creation of void[] (an array of System.Void) - if (elemType == ELEMENT_TYPE_VOID) - COMPlusThrow(kArgumentException); - if (cElements < 0) COMPlusThrow(kOverflowException); @@ -408,7 +402,7 @@ OBJECTREF AllocateSzArray(MethodTable* pArrayMT, INT32 cElements, GC_ALLOC_FLAGS #endif #ifdef FEATURE_DOUBLE_ALIGNMENT_HINT - if ((elemType == ELEMENT_TYPE_R8) && + if ((pArrayMT->GetArrayElementType() == ELEMENT_TYPE_R8) && ((DWORD)cElements >= g_pConfig->GetDoubleArrayToLargeObjectHeapThreshold())) { STRESS_LOG2(LF_GC, LL_INFO10, "Allocating double MD array of size %d and length %d to large object heap\n", totalSize, cElements); @@ -431,7 +425,7 @@ OBJECTREF AllocateSzArray(MethodTable* pArrayMT, INT32 cElements, GC_ALLOC_FLAGS else { #ifndef FEATURE_64BIT_ALIGNMENT - if ((DATA_ALIGNMENT < sizeof(double)) && (elemType == ELEMENT_TYPE_R8) && + if ((DATA_ALIGNMENT < sizeof(double)) && (pArrayMT->GetArrayElementType() == ELEMENT_TYPE_R8) && (totalSize < g_pConfig->GetGCLOHThreshold() - MIN_OBJECT_SIZE)) { // Creation of an array of doubles, not in the large object heap. @@ -508,18 +502,12 @@ OBJECTREF TryAllocateFrozenSzArray(MethodTable* pArrayMT, INT32 cElements) // The initial validation is copied from AllocateSzArray impl - CorElementType elemType = pArrayMT->GetArrayElementType(); - if (pArrayMT->ContainsPointers() && cElements > 0) { // For arrays with GC pointers we can only work with empty arrays return NULL; } - // Disallow the creation of void[] (an array of System.Void) - if (elemType == ELEMENT_TYPE_VOID) - COMPlusThrow(kArgumentException); - if (cElements < 0) COMPlusThrow(kOverflowException); @@ -542,7 +530,7 @@ OBJECTREF TryAllocateFrozenSzArray(MethodTable* pArrayMT, INT32 cElements) // FrozenObjectHeapManager doesn't yet support objects with a custom alignment, // so we give up on arrays of value types requiring 8 byte alignment on 32bit platforms. - if ((DATA_ALIGNMENT < sizeof(double)) && (elemType == ELEMENT_TYPE_R8)) + if ((DATA_ALIGNMENT < sizeof(double)) && (pArrayMT->GetArrayElementType() == ELEMENT_TYPE_R8)) { return NULL; } @@ -637,11 +625,6 @@ OBJECTREF AllocateArrayEx(MethodTable *pArrayMT, INT32 *pArgs, DWORD dwNumArgs, CorElementType kind = pArrayMT->GetInternalCorElementType(); _ASSERTE(kind == ELEMENT_TYPE_ARRAY || kind == ELEMENT_TYPE_SZARRAY); - CorElementType elemType = pArrayMT->GetArrayElementType(); - // Disallow the creation of void[,] (a multi-dim array of System.Void) - if (elemType == ELEMENT_TYPE_VOID) - COMPlusThrow(kArgumentException); - // Calculate the total number of elements in the array UINT32 cElements; bool maxArrayDimensionLengthOverflow = false; @@ -715,7 +698,7 @@ OBJECTREF AllocateArrayEx(MethodTable *pArrayMT, INT32 *pArgs, DWORD dwNumArgs, #endif #ifdef FEATURE_DOUBLE_ALIGNMENT_HINT - if ((elemType == ELEMENT_TYPE_R8) && + if ((pArrayMT->GetArrayElementType() == ELEMENT_TYPE_R8) && (cElements >= g_pConfig->GetDoubleArrayToLargeObjectHeapThreshold())) { STRESS_LOG2(LF_GC, LL_INFO10, "Allocating double MD array of size %d and length %d to large object heap\n", totalSize, cElements); diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ActivatorTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ActivatorTests.cs index af251a5e7fa985..7f575d04fd823e 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ActivatorTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ActivatorTests.cs @@ -154,7 +154,6 @@ public void CreateInstance_ContainsGenericParameters_ThrowsArgumentException(Typ public static IEnumerable CreateInstance_InvalidType_TestData() { yield return new object[] { typeof(void) }; - yield return new object[] { typeof(void).MakeArrayType() }; yield return new object[] { typeof(ArgIterator) }; } diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Type/TypeTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Type/TypeTests.cs index 9ef71efd77a68c..d604b588621ed9 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Type/TypeTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Type/TypeTests.cs @@ -47,7 +47,6 @@ static TypeTests() NonArrayBaseTypes = new List() { typeof(int), - typeof(void), typeof(int*), typeof(Outside), typeof(Outside), @@ -244,7 +243,6 @@ public void MakeArrayType_Invoke_ReturnsExpected(Type t, Type tArrayExpected) public static IEnumerable MakeArray_UnusualTypes_TestData() { yield return new object[] { typeof(StaticClass) }; - yield return new object[] { typeof(void) }; yield return new object[] { typeof(GenericClass<>) }; yield return new object[] { typeof(GenericClass<>).MakeGenericType(typeof(GenericClass<>)) }; yield return new object[] { typeof(GenericClass<>).GetTypeInfo().GetGenericArguments()[0] }; @@ -252,7 +250,6 @@ public static IEnumerable MakeArray_UnusualTypes_TestData() [Theory] [MemberData(nameof(MakeArray_UnusualTypes_TestData))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/52072", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)] public void MakeArrayType_UnusualTypes_ReturnsExpected(Type t) { Type tArray = t.MakeArrayType(); @@ -298,18 +295,19 @@ public void MakeArrayType_InvokeRankTwo_ReturnsExpected(Type t, Type tArrayExpec Assert.Equal(t.ToString() + "[,]", tArray.ToString()); } - public static IEnumerable MakeArrayType_ByRef_TestData() + public static IEnumerable MakeArrayType_InvalidElementType_TestData() { yield return new object[] { typeof(int).MakeByRefType() }; yield return new object[] { typeof(TypedReference) }; yield return new object[] { typeof(ArgIterator) }; yield return new object[] { typeof(RuntimeArgumentHandle) }; yield return new object[] { typeof(Span) }; + yield return new object[] { typeof(void) }; } [Theory] - [MemberData(nameof(MakeArrayType_ByRef_TestData))] - public void MakeArrayType_ByRef_ThrowsTypeLoadException(Type t) + [MemberData(nameof(MakeArrayType_InvalidElementType_TestData))] + public void MakeArrayType_InvalidElementType_ThrowsTypeLoadException(Type t) { Assert.Throws(() => t.MakeArrayType()); } @@ -607,7 +605,6 @@ public void IsSZArray_FalseForNonArrayTypes() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/52072", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)] public void IsSZArray_TrueForSZArrayTypes() { foreach (Type type in NonArrayBaseTypes.Select(nonArrayBaseType => nonArrayBaseType.MakeArrayType())) @@ -659,7 +656,6 @@ public void IsVariableBoundArray_FalseForNonArrayTypes() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/52072", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)] public void IsVariableBoundArray_FalseForSZArrayTypes() { foreach (Type type in NonArrayBaseTypes.Select(nonArrayBaseType => nonArrayBaseType.MakeArrayType())) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 995406e86fc430..ff73588c613d2d 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -1170,12 +1170,8 @@ mono_class_create_bounded_array (MonoClass *eclass, guint32 rank, gboolean bound klass->rank = GUINT32_TO_UINT8 (rank); klass->element_class = eclass; - if (m_class_get_byval_arg (eclass)->type == MONO_TYPE_TYPEDBYREF) { - /*Arrays of those two types are invalid.*/ - ERROR_DECL (prepared_error); - mono_error_set_invalid_program (prepared_error, "Arrays of System.TypedReference types are invalid."); - mono_class_set_failure (klass, mono_error_box (prepared_error, klass->image)); - mono_error_cleanup (prepared_error); + if (MONO_TYPE_IS_VOID (m_class_get_byval_arg (eclass))) { + mono_class_set_type_load_failure (klass, "Arrays of System.Void types are invalid."); } else if (m_class_is_byreflike (eclass)) { /* .NET Core throws a type load exception: "Could not create array type 'fullname[]'" */ char *full_name = mono_type_get_full_name (eclass); diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index 75d06bace0fe8c..9ac5ff79ebead2 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -5966,9 +5966,9 @@ check_for_invalid_array_type (MonoType *type, MonoError *error) gboolean allowed = TRUE; char *name; - if (m_type_is_byref (type)) + if (MONO_TYPE_IS_VOID (type)) allowed = FALSE; - else if (type->type == MONO_TYPE_TYPEDBYREF) + else if (m_type_is_byref (type)) allowed = FALSE; MonoClass *klass = mono_class_from_mono_type_internal (type); diff --git a/src/tests/JIT/Regression/CLR-x86-EJIT/v1-m10/b02353/b02353.cs b/src/tests/JIT/Regression/CLR-x86-EJIT/v1-m10/b02353/b02353.cs index 544e827d8bf1f6..0a3a820bf30963 100644 --- a/src/tests/JIT/Regression/CLR-x86-EJIT/v1-m10/b02353/b02353.cs +++ b/src/tests/JIT/Regression/CLR-x86-EJIT/v1-m10/b02353/b02353.cs @@ -42,7 +42,6 @@ public class Bug Type.GetType("System.Object"), Type.GetType("Simple"), Type.GetType("System.Empty[]"), - Type.GetType("System.Void[]"), Type.GetType("System.Boolean[]"), Type.GetType("System.Char[]"), Type.GetType("System.SByte[]"), @@ -66,7 +65,6 @@ public class Bug Type.GetType("System.Object[]"), Type.GetType("Simple[]"), Type.GetType("System.Empty[][]"), - Type.GetType("System.Void[][]"), Type.GetType("System.Boolean[][]"), Type.GetType("System.Char[][]"), Type.GetType("System.SByte[][]"), @@ -90,7 +88,6 @@ public class Bug Type.GetType("System.Object[][]"), Type.GetType("Simple[][]"), Type.GetType("System.Empty[][][]"), - Type.GetType("System.Void[][][]"), Type.GetType("System.Boolean[][][]"), Type.GetType("System.Char[][][]"), Type.GetType("System.SByte[][][]"),