diff --git a/docs/design/specs/Ecma-335-Augments.md b/docs/design/specs/Ecma-335-Augments.md index 1600e8e19ab6c3..52558651942e6e 100644 --- a/docs/design/specs/Ecma-335-Augments.md +++ b/docs/design/specs/Ecma-335-Augments.md @@ -146,33 +146,21 @@ TypeSpecBlob ::= 3. PEVerify, the CLR runtime and C# compiler prior to VS 2015 report an error when encountering an encoded TypeSpec in the positions described above. -### 2. `(CMOD_OPT | CMOD_REQ) ` is permitted in practice +### 2. `(CMOD_OPT | CMOD_REQ) ` is invalid In II.23.2.7, it is noted that CMOD_OPT or CMOD_REQD is followed -by a TypeRef or TypeDef metadata token, but TypeSpec tokens are -also allowed by ilasm, csc, peverify, and the CLR. +by a TypeRef or TypeDef metadata token. TypeSpec tokens in this +position should be treated as invalid metadata. -Note, in particular, that TypeSpecs are used there by C++/CLI to -represent strongly-typed boxing in C++/CLI. e.g. `Nullable^` -in C++/CLI becomes -``[mscorlib]System.ValueType modopt([mscorlib]System.Nullable`1) modopt([mscorlib]System.Runtime.CompilerServices.IsBoxed)`` -in IL. - -This tolerance adds a loophole to the rule above whereby cyclical -signatures are in fact possible, e.g.: - -* `TypeSpec #1: PTR CMOD_OPT I4` - -Such signatures can currently cause crashes in the runtime and various -tools, so if the spec is amended to permit TypeSpecs as modifiers, -then there should be a clarification that cycles are nonetheless not -permitted, and ideally readers would detect such cycles and handle the -error with a suitable message rather than a stack overflow. +Allowing TypeSpec custom modifier types creates a path for +recursive TypeSpec encodings that are complicated to detect. +Such metadata should be rejected rather than cycle-detected. Related issues: * https://github.com/dotnet/roslyn/issues/7971 * https://github.com/dotnet/runtime/issues/4945 +* https://github.com/dotnet/runtime/pull/123819#discussion_r2810397721 #### Proposed specification change @@ -185,9 +173,9 @@ for details with > The CMOD_OPT or CMOD_REQD is followed by a metadata token that indexes a row in the TypeDef -table, TypeRef table, or TypeSpec table. However, these tokens are encoded and compressed – -see §II.23.2.8 for details. Furthermore, if a row in the TypeSpec table is indicated, -it must not create cycle. +table or TypeRef table. However, these tokens are encoded and compressed – +see §II.23.2.8 for details. TypeSpec tokens are not valid in this position; this +restriction prevents custom modifiers from forming recursive TypeSpec encodings. ### 3. Custom modifiers can go in more places than specified @@ -323,17 +311,6 @@ sense in `TypeSpec` are `(CLASS | VALUETYPE) TypeDefOrRef` since `TypeDefOrRef` tokens can be used directly and the indirection through a `TypeSpec` would serve no purpose. -In the same way as `constrained.`, (assuming #2 is a spec bug and not -an ilasm/peverify/CLR quirk), custom modifiers can beget `TypeSpec`s -beyond what is allowed by II.23.2.14, e.g. `modopt(int32)` creates a -typespec with signature I4. - -Even more obscurely, this gives us a way to use `VOID`, `TYPEDBYREF`, -`CMOD_OPT`, and `CMOD_REQ` at the root of a `TypeSpec`, which are not even -specified as valid at the root of a `Type`: `modopt(int32 modopt(int32))`, -`modopt(void)`, and `modopt(typedref)` all work in -practice. `CMOD_OPT` and `CMOD_REQ` at the root can also be obtained by putting -a modifier on the type used with `constrained.`. ## Heap sizes @@ -1196,4 +1173,4 @@ In section II.23.1.15, the following row is added to the table: ## Implict argument coercion rules -Implicit argument coercion as defined in section III.1.6 does not match with existing practice in CLR runtimes. Notably, implicit argument coercion of an `int32` on the IL evaluation stack to a `native unsigned int` is a sign extending operation, not a zero-extending operation. \ No newline at end of file +Implicit argument coercion as defined in section III.1.6 does not match with existing practice in CLR runtimes. Notably, implicit argument coercion of an `int32` on the IL evaluation stack to a `native unsigned int` is a sign extending operation, not a zero-extending operation. diff --git a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs index 312d33f2fd5eb3..6ccaad846a2ae7 100644 --- a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs +++ b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs @@ -570,9 +570,10 @@ private object ResolveTypeSpecification(TypeSpecificationHandle handle) TypeSpecification typeSpecification = _metadataReader.GetTypeSpecification(handle); BlobReader signatureReader = _metadataReader.GetBlobReader(typeSpecification.Signature); + EcmaSignatureParser parser = new EcmaSignatureParser(this, signatureReader, NotFoundBehavior.ReturnResolutionFailure); - TypeDesc parsedType = parser.ParseType(); + TypeDesc parsedType = parser.ParseTypeSpec(); if (parsedType == null) return parser.ResolutionFailure; else diff --git a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureParser.cs b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureParser.cs index 20c041e47cc31c..e3ffaae380ebb2 100644 --- a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureParser.cs +++ b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureParser.cs @@ -9,7 +9,7 @@ namespace Internal.TypeSystem.Ecma { - public struct EcmaSignatureParser + public partial struct EcmaSignatureParser { private TypeSystemContext _tsc; private Func _typeResolver; @@ -53,6 +53,8 @@ private void SetResolutionFailure(ResolutionFailure failure) public ResolutionFailure ResolutionFailure => _resolutionFailure; + partial void ReportInvalidTypeSpec(); + private TypeDesc ResolveHandle(EntityHandle handle) { object resolvedValue; @@ -269,13 +271,21 @@ private SignatureTypeCode ParseTypeCode(bool skipPinned = true) private SignatureTypeCode ParseTypeCodeImpl(bool skipPinned = true) { - for (; ; ) - { - SignatureTypeCode typeCode = _reader.ReadSignatureTypeCode(); + return ParseTypeCodeImpl(_reader.ReadSignatureTypeCode(), skipPinned); + } + private SignatureTypeCode ParseTypeCodeImpl(SignatureTypeCode typeCode, bool skipPinned = true) + { + for (; ; typeCode = _reader.ReadSignatureTypeCode()) + { if (typeCode == SignatureTypeCode.RequiredModifier) { EntityHandle typeHandle = _reader.ReadTypeHandle(); + if (typeHandle.Kind == HandleKind.TypeSpecification) + { + ReportInvalidTypeSpec(); + } + _embeddedSignatureDataList?.Add(new EmbeddedSignatureData { index = string.Join(".", _indexStack), kind = EmbeddedSignatureDataKind.RequiredCustomModifier, type = ResolveHandle(typeHandle) }); continue; } @@ -283,6 +293,11 @@ private SignatureTypeCode ParseTypeCodeImpl(bool skipPinned = true) if (typeCode == SignatureTypeCode.OptionalModifier) { EntityHandle typeHandle = _reader.ReadTypeHandle(); + if (typeHandle.Kind == HandleKind.TypeSpecification) + { + ReportInvalidTypeSpec(); + } + _embeddedSignatureDataList?.Add(new EmbeddedSignatureData { index = string.Join(".", _indexStack), kind = EmbeddedSignatureDataKind.OptionalCustomModifier, type = ResolveHandle(typeHandle) }); continue; } @@ -316,6 +331,25 @@ private TypeDesc ParseTypeImpl() return ParseType(ParseTypeCode()); } + public TypeDesc ParseTypeSpec() + { + // ECMA-335 II.23.2.14 defines a narrower TypeSpecBlob grammar than what + // the .NET runtime accepts in practice. See the runtime ECMA-335 augment: + // https://github.com/dotnet/runtime/blob/main/docs/design/specs/Ecma-335-Augments.md#5-typespecs-can-encode-more-than-specified + // + // In practice, TypeSpec can encode primitives, VAR/MVAR, custom-modifier + // rooted forms, and other forms used by existing tools/runtimes. The only + // top-level TypeSpec form that does not make sense is a direct + // CLASS/VALUETYPE TypeDefOrRef, represented here as SignatureTypeCode.TypeHandle + SignatureTypeCode typeCode = _reader.ReadSignatureTypeCode(); + if (typeCode == SignatureTypeCode.TypeHandle) + { + ReportInvalidTypeSpec(); + } + + return ParseType(ParseTypeCodeImpl(typeCode)); + } + public bool IsFieldSignature { get diff --git a/src/coreclr/tools/ILVerification.Tests/ILTests/AccessTests.il b/src/coreclr/tools/ILVerification.Tests/ILTests/AccessTests.il index 1139e289e03719..4132dd2c34a9d7 100644 --- a/src/coreclr/tools/ILVerification.Tests/ILTests/AccessTests.il +++ b/src/coreclr/tools/ILVerification.Tests/ILTests/AccessTests.il @@ -264,7 +264,7 @@ .method public hidebysig instance void Call.GenericMethodWithPrivateNestedClass_Invalid_MethodAccess() cil managed { - call void class SimpleClass::GenericMethod() + call void SimpleClass::GenericMethod() ret } diff --git a/src/coreclr/tools/ILVerification.Tests/ILTests/BaseTypeTests.il b/src/coreclr/tools/ILVerification.Tests/ILTests/BaseTypeTests.il new file mode 100644 index 00000000000000..36384311540f79 --- /dev/null +++ b/src/coreclr/tools/ILVerification.Tests/ILTests/BaseTypeTests.il @@ -0,0 +1,72 @@ +// 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 +{ +} + +.assembly BaseTypeTests +{ +} + +.class public auto ansi beforefieldinit ObjectTypeSpecBase_InvalidType_InvalidBaseType + extends object +{ + .method public hidebysig specialname rtspecialname instance void .ctor() cil managed + { + .maxstack 8 + + ldarg.0 + call instance void [System.Runtime]System.Object::.ctor() + ret + } +} + +.class public auto ansi beforefieldinit ArrayTypeSpecBase_InvalidType_InvalidBaseType + extends int32[] +{ +} + +.class public auto ansi beforefieldinit PointerTypeSpecBase_InvalidType_InvalidBaseType + extends int32* +{ +} + +.class interface public auto ansi abstract NilBaseInterface_ValidType_Valid +{ +} + +.class public auto ansi beforefieldinit GenericBase`1 + extends [System.Runtime]System.Object +{ +} + +.class public auto ansi beforefieldinit GenericClassTypeSpecBase_ValidType_Valid + extends class GenericBase`1 +{ +} + +.class public auto ansi beforefieldinit GenericOpenClassTypeSpecBase_ValidType_Valid`1 + extends class GenericBase`1 +{ +} + +.class public sequential ansi sealed beforefieldinit ValueTypeBase + extends [System.Runtime]System.ValueType +{ +} + +.class public auto ansi beforefieldinit ValueTypeBase_InvalidType_InvalidBaseType + extends ValueTypeBase +{ +} + +.class public sequential ansi sealed beforefieldinit GenericValueTypeBase`1 + extends [System.Runtime]System.ValueType +{ +} + +.class public auto ansi beforefieldinit GenericValueTypeSpecBase_InvalidType_InvalidBaseType + extends valuetype GenericValueTypeBase`1 +{ +} diff --git a/src/coreclr/tools/ILVerification.Tests/ILTests/BaseTypeTests.ilproj b/src/coreclr/tools/ILVerification.Tests/ILTests/BaseTypeTests.ilproj new file mode 100644 index 00000000000000..356b4dcc778989 --- /dev/null +++ b/src/coreclr/tools/ILVerification.Tests/ILTests/BaseTypeTests.ilproj @@ -0,0 +1,9 @@ + + + $(MSBuildProjectName) + + + + + + diff --git a/src/coreclr/tools/ILVerification.Tests/ILTests/LoadStoreIndirectTests.il b/src/coreclr/tools/ILVerification.Tests/ILTests/LoadStoreIndirectTests.il index d8ca59cd5f1c76..67f99c03a97fcf 100644 --- a/src/coreclr/tools/ILVerification.Tests/ILTests/LoadStoreIndirectTests.il +++ b/src/coreclr/tools/ILVerification.Tests/ILTests/LoadStoreIndirectTests.il @@ -140,7 +140,7 @@ ldloca.s V_0 ldstr "Hello" - stobj string + stobj [System.Runtime]System.String ret } diff --git a/src/coreclr/tools/ILVerification.Tests/ILTests/MalformedTypeSpecTests.il b/src/coreclr/tools/ILVerification.Tests/ILTests/MalformedTypeSpecTests.il new file mode 100644 index 00000000000000..e572019c304f63 --- /dev/null +++ b/src/coreclr/tools/ILVerification.Tests/ILTests/MalformedTypeSpecTests.il @@ -0,0 +1,30 @@ +// 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 +{ +} + +.assembly MalformedTypeSpecTests +{ +} + +.class public auto ansi beforefieldinit GenericBase`1 + extends [System.Runtime]System.Object +{ +} + +.class public auto ansi beforefieldinit CustomModifierTypeSpecBase_InvalidType_None + extends class GenericBase`1 +{ +} + +.class public auto ansi beforefieldinit MethodSignatureTests + extends [System.Runtime]System.Object +{ + .method public static hidebysig int32 modopt(int32) CustomModifierTypeSpecReturn_Invalid_None() cil managed + { + ldc.i4.0 + ret + } +} diff --git a/src/coreclr/tools/ILVerification.Tests/ILTests/MalformedTypeSpecTests.ilproj b/src/coreclr/tools/ILVerification.Tests/ILTests/MalformedTypeSpecTests.ilproj new file mode 100644 index 00000000000000..356b4dcc778989 --- /dev/null +++ b/src/coreclr/tools/ILVerification.Tests/ILTests/MalformedTypeSpecTests.ilproj @@ -0,0 +1,9 @@ + + + $(MSBuildProjectName) + + + + + + diff --git a/src/coreclr/tools/ILVerification/EcmaSignatureParser.ILVerify.cs b/src/coreclr/tools/ILVerification/EcmaSignatureParser.ILVerify.cs new file mode 100644 index 00000000000000..40b0c8d42f018f --- /dev/null +++ b/src/coreclr/tools/ILVerification/EcmaSignatureParser.ILVerify.cs @@ -0,0 +1,13 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Internal.TypeSystem.Ecma +{ + public partial struct EcmaSignatureParser + { + partial void ReportInvalidTypeSpec() + { + ThrowHelper.ThrowBadImageFormatException(); + } + } +} diff --git a/src/coreclr/tools/ILVerification/ILVerification.projitems b/src/coreclr/tools/ILVerification/ILVerification.projitems index c301f2da1d298e..489c446b1fac4d 100644 --- a/src/coreclr/tools/ILVerification/ILVerification.projitems +++ b/src/coreclr/tools/ILVerification/ILVerification.projitems @@ -1,6 +1,7 @@ + @@ -13,6 +14,7 @@ + ILVERIFICATION;$(DefineConstants) $(MSBuildThisFileDirectory)..\Common\ diff --git a/src/coreclr/tools/ILVerification/Strings.resx b/src/coreclr/tools/ILVerification/Strings.resx index 02be5d1e00ff0c..fd31723fb735cb 100644 --- a/src/coreclr/tools/ILVerification/Strings.resx +++ b/src/coreclr/tools/ILVerification/Strings.resx @@ -453,4 +453,7 @@ Stack must be empty before localloc, except for the size item. + + Type '{0}' has invalid base type '{1}'. + diff --git a/src/coreclr/tools/ILVerification/TypeVerifier.cs b/src/coreclr/tools/ILVerification/TypeVerifier.cs index 4caec0c170c7eb..61ceb6f1f362ed 100644 --- a/src/coreclr/tools/ILVerification/TypeVerifier.cs +++ b/src/coreclr/tools/ILVerification/TypeVerifier.cs @@ -39,15 +39,48 @@ public TypeVerifier(EcmaModule module, TypeDefinitionHandle typeDefinitionHandle public void Verify() { + VerifyBaseType(); + VerifyInterfaces(); } + private void VerifyBaseType() + { + TypeDefinition typeDefinition = _module.MetadataReader.GetTypeDefinition(_typeDefinitionHandle); + EcmaType type = _module.GetType(_typeDefinitionHandle); + EntityHandle baseType = typeDefinition.BaseType; + if (baseType.IsNil) + { + if (!type.IsObject && !type.IsModuleType && !type.IsInterface) + { + VerificationError(VerifierError.InvalidBaseType, Format(type), Format(baseType)); + return; + } + } + else + { + TypeDesc resolvedBaseType = _module.GetType(baseType); + + // Arrays, pointers, and generic variables are valid TypeSpec forms in other + // metadata and IL token contexts, so GetType must continue to resolve them. A + // BaseType TypeSpec is narrower: it has to name a constructed generic class. + if ((baseType.Kind == HandleKind.TypeSpecification && !resolvedBaseType.HasInstantiation) || + resolvedBaseType.IsValueType || + resolvedBaseType.IsInterface || + !resolvedBaseType.IsDefType) + { + VerificationError(VerifierError.InvalidBaseType, Format(type), Format(baseType)); + } + } + } + public void VerifyInterfaces() { TypeDefinition typeDefinition = _module.MetadataReader.GetTypeDefinition(_typeDefinitionHandle); EcmaType type = _module.GetType(_typeDefinitionHandle); - if (type.IsInterface) + // Read the metadata bit directly to avoid resolving an invalid base type. + if ((typeDefinition.Attributes & System.Reflection.TypeAttributes.Interface) != 0) { return; } @@ -115,17 +148,37 @@ public void VerifyInterfaces() private string Format(TypeDesc type) { - if (_verifierOptions.IncludeMetadataTokensInErrorMessages) + TypeDesc typeDefinition = type.GetTypeDefinition(); + if (_verifierOptions.IncludeMetadataTokensInErrorMessages && typeDefinition is EcmaType ecmaType) { - TypeDesc typeDesc = type.GetTypeDefinition(); - EcmaModule module = (EcmaModule)((MetadataType)typeDesc).Module; + EcmaModule module = (EcmaModule)ecmaType.Module; + return string.Format("{0}([{1}]0x{2:X8})", type, module, module.MetadataReader.GetToken(ecmaType.Handle)); + } + + return type.ToString(); + } + + private string Format(EntityHandle handle) + { + if (handle.IsNil) + { + return "nil"; + } - return string.Format("{0}([{1}]0x{2:X8})", type, module, module.MetadataReader.GetToken(((EcmaType)type).Handle)); + try + { + return Format(_module.GetType(handle)); } - else + catch (BadImageFormatException) { - return type.ToString(); } + catch (TypeSystemException) + { + } + + return _verifierOptions.IncludeMetadataTokensInErrorMessages ? + string.Format("{0}([{1}]0x{2:X8})", handle.Kind, _module, _module.MetadataReader.GetToken(handle)) : + handle.Kind.ToString(); } private string Format(TypeDesc interfaceTypeDesc, EcmaModule module, InterfaceImplementation interfaceImplementation) diff --git a/src/coreclr/tools/ILVerification/VerifierError.cs b/src/coreclr/tools/ILVerification/VerifierError.cs index bc4821fbedb7d4..c9d4e8e18c8d55 100644 --- a/src/coreclr/tools/ILVerification/VerifierError.cs +++ b/src/coreclr/tools/ILVerification/VerifierError.cs @@ -190,5 +190,6 @@ public enum VerifierError InterfaceImplHasDuplicate, // InterfaceImpl has a duplicate InterfaceMethodNotImplemented, // Class implements interface but not method LocallocStackNotEmpty, // localloc requires that stack must be empty, except for 'size' argument + InvalidBaseType, // Type has an invalid base type. } }