Detect ByRefLike types using attribute (#15745)
authorJan Kotas <jkotas@microsoft.com>
Fri, 5 Jan 2018 22:44:18 +0000 (14:44 -0800)
committerGitHub <noreply@github.com>
Fri, 5 Jan 2018 22:44:18 +0000 (14:44 -0800)
* Detect ByRefLike types using attribute and improve error messages for their invalid use

Fixes #11371 and #15458

src/dlls/mscorrc/mscorrc.rc
src/dlls/mscorrc/resource.h
src/mscorlib/src/System/ByReference.cs
src/vm/classnames.h
src/vm/jitinterface.cpp
src/vm/methodtablebuilder.cpp
src/vm/methodtablebuilder.h
tests/src/Loader/classloader/regressions/GitHub_11371/Negative_ByRefLikeType.il [new file with mode: 0644]
tests/src/Loader/classloader/regressions/GitHub_11371/Negative_ByRefLikeType.ilproj [new file with mode: 0644]

index bbf5a29..1f2423b 100644 (file)
@@ -1223,8 +1223,9 @@ BEGIN
     IDS_CLASSLOAD_NOTINTERFACE              "Could not load type '%1' from assembly '%2' because it attempts to implement a class as an interface."
     IDS_CLASSLOAD_VALUEINSTANCEFIELD        "Could not load the value type '%1' from assembly '%2' because it has an instance field of itself."
 
-    IDS_CLASSLOAD_BYREFLIKE_STATICFIELD     "A value type containing a by-ref instance field, such as Span<T>, cannot be used as the type for a static field."
-    IDS_CLASSLOAD_BYREFLIKE_NOTVALUECLASSFIELD "A value type containing a by-ref instance field, such as Span<T>, cannot be used as the type for a class instance field."
+    IDS_CLASSLOAD_BYREFLIKE_STATICFIELD     "A value type containing a ByRef-like instance field cannot be used as the type for a static field."
+    IDS_CLASSLOAD_BYREFLIKE_NOTVALUECLASSFIELD "A value type containing a ByRef-like instance field cannot be used as the type for a class instance field."
+    IDS_CLASSLOAD_NOTBYREFLIKE              "A value type containing a ByRef-like instance field must be ByRef-like type."
 
     IDS_CLASSLOAD_BAD_NAME                  "Type name '%1' from assembly '%2' is invalid."
     IDS_CLASSLOAD_RANK_TOOLARGE             "'%1' from assembly '%2' has too many dimensions."
index 053b1b4..45ddd2e 100644 (file)
 
 #define IDS_CLASSLOAD_BYREFLIKE_STATICFIELD        0x263b
 #define IDS_CLASSLOAD_BYREFLIKE_NOTVALUECLASSFIELD 0x263c
+#define IDS_CLASSLOAD_NOTBYREFLIKE                                0x263d
index 8079605..3e1fa8a 100644 (file)
@@ -11,7 +11,7 @@ namespace System
     // around lack of first class support for byref fields in C# and IL. The JIT and 
     // type loader has special handling for it that turns it into a thin wrapper around ref T.
     [NonVersionable]
-    internal struct ByReference<T>
+    internal ref struct ByReference<T>
     {
         private IntPtr _value;
 
index fc08737..6af229e 100644 (file)
 
 #define g_CompilerServicesFixedAddressValueTypeAttribute "System.Runtime.CompilerServices.FixedAddressValueTypeAttribute"
 #define g_CompilerServicesUnsafeValueTypeAttribute "System.Runtime.CompilerServices.UnsafeValueTypeAttribute"
+#define g_CompilerServicesIsByRefLikeAttribute "System.Runtime.CompilerServices.IsByRefLikeAttribute"
 #define g_CompilerServicesIntrinsicAttribute "System.Runtime.CompilerServices.IntrinsicAttribute"
 #define g_UnmanagedFunctionPointerAttribute "System.Runtime.InteropServices.UnmanagedFunctionPointerAttribute"
 #define g_DefaultDllImportSearchPathsAttribute "System.Runtime.InteropServices.DefaultDllImportSearchPathsAttribute"
index 671f7e8..af7e903 100644 (file)
@@ -6449,20 +6449,13 @@ CorInfoHelpFunc CEEInfo::getBoxHelper(CORINFO_CLASS_HANDLE clsHnd)
     }
     else 
     {
-        // Dev10 718281 - This has been functionally broken fora very long time (at least 2.0).
-        // The recent addition of the check for stack pointers has caused it to now AV instead
-        // of gracefully failing with an InvalidOperationException. Since nobody has noticed
-        // it being broken, we are choosing not to invest to fix it, and instead explicitly
-        // breaking it and failing early and consistently.
         if(VMClsHnd.IsTypeDesc())
-        {
             COMPlusThrow(kInvalidOperationException,W("InvalidOperation_TypeCannotBeBoxed"));
-        }
 
         // we shouldn't allow boxing of types that contains stack pointers
         // csc and vbc already disallow it.
         if (VMClsHnd.AsMethodTable()->IsByRefLike())
-            COMPlusThrow(kInvalidProgramException);
+            COMPlusThrow(kInvalidProgramException,W("NotSupported_ByRefLike"));
 
         result = CORINFO_HELP_BOX;
     }
index c13e220..4e8dd96 100644 (file)
@@ -1441,6 +1441,15 @@ MethodTableBuilder::BuildMethodTableThrowing(
         {
             SetUnsafeValueClass();
         }
+
+        hr = GetMDImport()->GetCustomAttributeByName(bmtInternal->pType->GetTypeDefToken(),
+            g_CompilerServicesIsByRefLikeAttribute,
+            NULL, NULL);
+        IfFailThrow(hr);
+        if (hr == S_OK)
+        {
+            bmtFP->fIsByRefLikeType = true;
+        }
     }
 
     // Check to see if the class is an enumeration. No fancy checks like the one immediately
@@ -1604,11 +1613,6 @@ MethodTableBuilder::BuildMethodTableThrowing(
         }
     }
 
-
-
-    // Set the contextful or marshalbyref flag if necessary
-    SetContextfulOrByRef();
-
     // NOTE: This appears to be the earliest point during class loading that other classes MUST be loaded
     // resolve unresolved interfaces, determine an upper bound on the size of the interface map,
     // and determine the size of the largest interface (in # slots)
@@ -4072,7 +4076,7 @@ VOID    MethodTableBuilder::InitializeFieldDescs(FieldDesc *pFieldDescList,
                     goto GOT_ELEMENT_TYPE;
                 }
                 
-                // Inherit IsByRefLike characteristic from fields
+                // Check ByRefLike fields
                 if (!IsSelfRef(pByValueClass) && pByValueClass->IsByRefLike())
                 {
                     if (fIsStatic)
@@ -4085,8 +4089,10 @@ VOID    MethodTableBuilder::InitializeFieldDescs(FieldDesc *pFieldDescList,
                         // Non-value-classes cannot contain by-ref-like instance fields
                         BuildMethodTableThrowException(IDS_CLASSLOAD_BYREFLIKE_NOTVALUECLASSFIELD);
                     }
-
-                    bmtFP->fIsByRefLikeType = true;
+                    if (!bmtFP->fIsByRefLikeType)
+                    {
+                        BuildMethodTableThrowException(IDS_CLASSLOAD_NOTBYREFLIKE);
+                    }
                 }
 
                 if (!IsSelfRef(pByValueClass) && pByValueClass->GetClass()->HasNonPublicFields())
@@ -9522,19 +9528,18 @@ void MethodTableBuilder::CheckForSystemTypes()
         _ASSERTE(g_pByReferenceClass != NULL);
         _ASSERTE(g_pByReferenceClass->IsByRefLike());
 
+#ifdef _TARGET_X86_
         if (GetCl() == g_pByReferenceClass->GetCl())
         {
-            pMT->SetIsByRefLike();
-#ifdef _TARGET_X86_
             // x86 by default treats the type of ByReference<T> as the actual type of its IntPtr field, see calls to
             // ComputeInternalCorElementTypeForValueType in this file. This is a special case where the struct needs to be
             // treated as a value type so that its field can be considered as a by-ref pointer.
             _ASSERTE(pMT->GetFlag(MethodTable::enum_flag_Category_Mask) == MethodTable::enum_flag_Category_PrimitiveValueType);
             pMT->ClearFlag(MethodTable::enum_flag_Category_Mask);
             pMT->SetInternalCorElementType(ELEMENT_TYPE_VALUETYPE);
-#endif
             return;
         }
+#endif
 
         _ASSERTE(g_pNullableClass->IsNullable());
 
@@ -9590,18 +9595,17 @@ void MethodTableBuilder::CheckForSystemTypes()
         {
             pMT->SetIsNullable();
         }
+#ifdef _TARGET_X86_
         else if (strcmp(name, g_ByReferenceName) == 0)
         {
-            pMT->SetIsByRefLike();
-#ifdef _TARGET_X86_
             // x86 by default treats the type of ByReference<T> as the actual type of its IntPtr field, see calls to
             // ComputeInternalCorElementTypeForValueType in this file. This is a special case where the struct needs to be
             // treated as a value type so that its field can be considered as a by-ref pointer.
             _ASSERTE(pMT->GetFlag(MethodTable::enum_flag_Category_Mask) == MethodTable::enum_flag_Category_PrimitiveValueType);
             pMT->ClearFlag(MethodTable::enum_flag_Category_Mask);
             pMT->SetInternalCorElementType(ELEMENT_TYPE_VALUETYPE);
-#endif
         }
+#endif
         else if (strcmp(name, g_ArgIteratorName) == 0)
         {
             // Mark the special types that have embeded stack pointers in them
@@ -11157,24 +11161,6 @@ BOOL MethodTableBuilder::NeedsAlignedBaseOffset()
 //
 // Used by BuildMethodTable
 //
-// Set the contextful or marshaledbyref flag on the attributes of the class
-//
-VOID MethodTableBuilder::SetContextfulOrByRef()
-{
-    CONTRACTL
-    {
-        STANDARD_VM_CHECK;
-        PRECONDITION(CheckPointer(this));
-        PRECONDITION(CheckPointer(bmtInternal));
-
-    }
-    CONTRACTL_END;
-
-}
-//*******************************************************************************
-//
-// Used by BuildMethodTable
-//
 // Set the HasFinalizer and HasCriticalFinalizer flags
 //
 VOID MethodTableBuilder::SetFinalizationSemantics()
index 9a05843..d736368 100644 (file)
@@ -2875,8 +2875,6 @@ private:
 
     VOID CheckForSpecialTypes();
 
-    VOID SetContextfulOrByRef();
-
 #ifdef FEATURE_READYTORUN
 
     VOID CheckLayoutDependsOnOtherModules(MethodTable * pDependencyMT);
diff --git a/tests/src/Loader/classloader/regressions/GitHub_11371/Negative_ByRefLikeType.il b/tests/src/Loader/classloader/regressions/GitHub_11371/Negative_ByRefLikeType.il
new file mode 100644 (file)
index 0000000..a231285
--- /dev/null
@@ -0,0 +1,186 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+.assembly extern System.Console { }
+.assembly extern System.Runtime { }
+.assembly Negative_ByRefLikeType { }
+
+.class sequential ansi sealed beforefieldinit MyByRefLikeType
+       extends [System.Runtime]System.ValueType
+{
+    .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( 01 00 00 00 ) 
+}
+
+.class sequential ansi sealed beforefieldinit ByRefLikeStaticField
+{
+  .field private static valuetype MyByRefLikeType s
+}
+
+.class sequential ansi sealed beforefieldinit ByRefLikeFieldInNonValueType
+{
+  .field private valuetype MyByRefLikeType f
+}
+
+.class sequential ansi sealed beforefieldinit ByRefLikeFieldInNonByRefLikeType
+{
+  .field private valuetype MyByRefLikeType f
+}
+
+.class public auto ansi beforefieldinit Test
+       extends [System.Runtime]System.Object
+{
+  .method private hidebysig static void ByRefLikeBoxing() cil managed
+  {
+    .maxstack  1
+    .locals init (valuetype MyByRefLikeType V_0)
+    ldloc.0
+    box valuetype MyByRefLikeType
+    pop
+    ret
+  }
+
+  .method private hidebysig static void ByRefLikeStaticField() cil managed
+  {
+    .maxstack  1
+    ldsfld valuetype MyByRefLikeType ByRefLikeStaticField::s
+    pop
+    ret
+  }
+
+  .method private hidebysig static void ByRefLikeFieldInNonValueType() cil managed
+  {
+    .maxstack  1
+    ldnull
+    ldfld valuetype MyByRefLikeType ByRefLikeFieldInNonValueType::f
+    pop
+    ret
+  }
+
+  .method private hidebysig static void ByRefLikeFieldInNonByRefLikeType() cil managed
+  {
+    .maxstack  1
+    .locals init (valuetype ByRefLikeFieldInNonByRefLikeType V_0)
+    ldloc.0
+    ldfld valuetype MyByRefLikeType ByRefLikeFieldInNonValueType::f
+    pop
+    ret
+  }
+
+  .method private hidebysig static void ByRefLikeArray() cil managed
+  {
+    .maxstack  1
+    ldc.i4.1
+    newarr     valuetype MyByRefLikeType
+    pop
+    ret
+  }
+
+  .method private hidebysig static void ByRefLikeGenericInstantiation() cil managed
+  {
+    .maxstack  1
+    newobj     instance void class [System.Runtime]System.Collections.Generic.List`1<valuetype MyByRefLikeType>::.ctor()
+    pop
+    ret
+  }
+
+  .method public hidebysig static int32 Main() cil managed
+  {
+    .entrypoint
+    .maxstack  1
+
+    ldstr "ByRefLikeBoxing"
+    call void [System.Console]System.Console::WriteLine(string)
+    .try
+    {
+        call void Test::ByRefLikeBoxing()
+        leave TestFailed
+    }
+    catch [System.Runtime]System.InvalidProgramException
+    {
+      pop
+      leave ByRefLikeBoxing_Done
+    }
+ByRefLikeBoxing_Done:
+
+    ldstr "ByRefLikeStaticField"
+    call void [System.Console]System.Console::WriteLine(string)
+    .try
+    {
+        call void Test::ByRefLikeStaticField()
+        leave TestFailed
+    }
+    catch [System.Runtime]System.TypeLoadException
+    {
+      pop
+      leave ByRefLikeStaticField_Done
+    }
+ByRefLikeStaticField_Done:
+
+    ldstr "ByRefLikeFieldInNonValueType"
+    call void [System.Console]System.Console::WriteLine(string)
+    .try
+    {
+        call void Test::ByRefLikeFieldInNonValueType()
+        leave TestFailed
+    }
+    catch [System.Runtime]System.TypeLoadException
+    {
+      pop
+      leave ByRefLikeFieldInNonValueType_Done
+    }
+ByRefLikeFieldInNonValueType_Done:
+
+    ldstr "ByRefLikeFieldInNonByRefLikeType"
+    call void [System.Console]System.Console::WriteLine(string)
+    .try
+    {
+        call void Test::ByRefLikeFieldInNonByRefLikeType()
+        leave TestFailed
+    }
+    catch [System.Runtime]System.TypeLoadException
+    {
+      pop
+      leave ByRefLikeFieldInNonByRefLikeType_Done
+    }
+ByRefLikeFieldInNonByRefLikeType_Done:
+
+    ldstr "ByRefLikeArray"
+    call void [System.Console]System.Console::WriteLine(string)
+    .try
+    {
+        call void Test::ByRefLikeArray()
+        leave TestFailed
+    }
+    catch [System.Runtime]System.TypeLoadException
+    {
+      pop
+      leave ByRefLikeArray_Done
+    }
+ByRefLikeArray_Done:
+
+    ldstr "ByRefLikeGenericInstantiation"
+    call void [System.Console]System.Console::WriteLine(string)
+    .try
+    {
+        call void Test::ByRefLikeGenericInstantiation()
+        leave TestFailed
+    }
+    catch [System.Runtime]System.TypeLoadException
+    {
+      pop
+      leave ByRefLikeGenericInstantiation_Done
+    }
+ByRefLikeGenericInstantiation_Done:
+
+    ldstr "All Tests Passed"
+    call void [System.Console]System.Console::WriteLine(string)
+    ldc.i4.s 100
+    ret
+  TestFailed:
+    ldstr "Test Failed"
+    call void [System.Console]System.Console::WriteLine(string)
+    ldc.i4.1
+    ret
+  }
+}
diff --git a/tests/src/Loader/classloader/regressions/GitHub_11371/Negative_ByRefLikeType.ilproj b/tests/src/Loader/classloader/regressions/GitHub_11371/Negative_ByRefLikeType.ilproj
new file mode 100644 (file)
index 0000000..deca7f1
--- /dev/null
@@ -0,0 +1,23 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <AssemblyName>Negative_ByRefLikeType</AssemblyName>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <OutputType>Exe</OutputType>
+    <CLRTestKind>BuildAndRun</CLRTestKind>
+    <CLRTestPriority>0</CLRTestPriority>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="Negative_ByRefLikeType.il" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+</Project>