Validate invalid base type declarations in ILVerify

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) <TypeSpec>` is permitted in practice
+### 2. `(CMOD_OPT | CMOD_REQ) <TypeSpec>` 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<int>^`
-in C++/CLI becomes
-``[mscorlib]System.ValueType modopt([mscorlib]System.Nullable`1<int>) 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 <TypeSpec #1> 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.
+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.

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

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<EntityHandle, NotFoundBehavior, TypeDesc> _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,20 +271,33 @@ 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;
                 }
 
                 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

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<class SimpleClass/PrivateNestedClass>()
+        call      void SimpleClass::GenericMethod<class SimpleClass/PrivateNestedClass>()
         ret
     }
 

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<T>
+    extends [System.Runtime]System.Object
+{
+}
+
+.class public auto ansi beforefieldinit GenericClassTypeSpecBase_ValidType_Valid
+    extends class GenericBase`1<int32>
+{
+}
+
+.class public auto ansi beforefieldinit GenericOpenClassTypeSpecBase_ValidType_Valid`1<T>
+    extends class GenericBase`1<!0>
+{
+}
+
+.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<T>
+    extends [System.Runtime]System.ValueType
+{
+}
+
+.class public auto ansi beforefieldinit GenericValueTypeSpecBase_InvalidType_InvalidBaseType
+    extends valuetype GenericValueTypeBase`1<int32>
+{
+}

src/coreclr/tools/ILVerification.Tests/ILTests/BaseTypeTests.ilproj

@@ -0,0 +1,9 @@
+<Project Sdk="Microsoft.NET.Sdk.IL">
+  <PropertyGroup>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).il" />
+  </ItemGroup>
+</Project>

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
     }
 

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<T>
+    extends [System.Runtime]System.Object
+{
+}
+
+.class public auto ansi beforefieldinit CustomModifierTypeSpecBase_InvalidType_None
+    extends class GenericBase`1<int32 modopt(int32)>
+{
+}
+
+.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
+    }
+}

src/coreclr/tools/ILVerification.Tests/ILTests/MalformedTypeSpecTests.ilproj

@@ -0,0 +1,9 @@
+<Project Sdk="Microsoft.NET.Sdk.IL">
+  <PropertyGroup>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).il" />
+  </ItemGroup>
+</Project>

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();
+        }
+    }
+}

src/coreclr/tools/ILVerification/ILVerification.projitems

@@ -1,6 +1,7 @@
 <Project>
   <ItemGroup>
     <Compile Include="$(MSBuildThisFileDirectory)AccessVerificationHelpers.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)EcmaSignatureParser.ILVerify.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)ILImporter.StackValue.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)ILImporter.Verify.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)ILVerifyTypeSystemContext.cs" />
@@ -13,6 +14,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)VerifierError.cs" />
   </ItemGroup>
   <PropertyGroup>
+    <DefineConstants>ILVERIFICATION;$(DefineConstants)</DefineConstants>
     <ToolsCommonPath>$(MSBuildThisFileDirectory)..\Common\</ToolsCommonPath>
   </PropertyGroup>
   <ItemGroup>

src/coreclr/tools/ILVerification/Strings.resx

@@ -453,4 +453,7 @@
   <data name="LocallocStackNotEmpty" xml:space="preserve">
     <value>Stack must be empty before localloc, except for the size item.</value>
   </data>
+  <data name="InvalidBaseType" xml:space="preserve">
+    <value>Type '{0}' has invalid base type '{1}'.</value>
+  </data>
 </root>