Fix UnsafeAccessor scenario for modopts/modreqs when comparing field sigs. (#111648) (#111675)

* Consume custom modifiers and ByRef in RetType signature
prior to comparing field signature.

Author: AaronRobinsonMSFT

Diff for: src/coreclr/vm/prestub.cpp

@@ -1445,11 +1445,18 @@ namespace
 
         DWORD declArgCount;
         IfFailThrow(CorSigUncompressData_EndPtr(pSig1, pEndSig1, &declArgCount));
+        if (pSig1 >= pEndSig1)
+            ThrowHR(META_E_BAD_SIGNATURE);
 
-        // UnsafeAccessors for fields require return types be byref.
-        // This was explicitly checked in TryGenerateUnsafeAccessor().
+        // UnsafeAccessors for fields require return types be byref. However, we first need to
+        // consume any custom modifiers which are prior to the expected ELEMENT_TYPE_BYREF in
+        // the RetType signature (II.23.2.11).
+        _ASSERTE(state.IgnoreCustomModifiers); // We should always ignore custom modifiers for field look-up.
+        MetaSig::ConsumeCustomModifiers(pSig1, pEndSig1);
         if (pSig1 >= pEndSig1)
             ThrowHR(META_E_BAD_SIGNATURE);
+
+        // The ELEMENT_TYPE_BYREF was explicitly checked in TryGenerateUnsafeAccessor().
         CorElementType byRefType = CorSigUncompressElementType(pSig1);
         _ASSERTE(byRefType == ELEMENT_TYPE_BYREF);
 

Diff for: src/coreclr/vm/siginfo.cpp

@@ -3604,7 +3604,7 @@ BOOL CompareTypeTokens(mdToken tk1, mdToken tk2, ModuleBase *pModule1, ModuleBas
 #endif //!DACCESS_COMPILE
 } // CompareTypeTokens
 
-static void ConsumeCustomModifiers(PCCOR_SIGNATURE& pSig, PCCOR_SIGNATURE pEndSig)
+void MetaSig::ConsumeCustomModifiers(PCCOR_SIGNATURE& pSig, PCCOR_SIGNATURE pEndSig)
 {
     mdToken tk;
     CorElementType type;

Diff for: src/coreclr/vm/siginfo.hpp

@@ -945,6 +945,14 @@ class MetaSig
         //------------------------------------------------------------------
         CorElementType GetByRefType(TypeHandle* pTy) const;
 
+        //------------------------------------------------------------------
+        // Consume the custom modifiers, if any, in the current signature
+        // and update it.
+        // This is a non destructive operation if the current signature is not
+        // pointing at a sequence of ELEMENT_TYPE_CMOD_REQD or ELEMENT_TYPE_CMOD_OPT.
+        //------------------------------------------------------------------
+        static void ConsumeCustomModifiers(PCCOR_SIGNATURE& pSig, PCCOR_SIGNATURE pEndSig);
+
         // Struct used to capture in/out state during the comparison
         // of element types.
         struct CompareState

Diff for: src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs

@@ -328,28 +328,70 @@ public static void Verify_AccessAllFields_CorElementType()
         extern static ref delegate*<void> GetFPtr(ref AllFields f);
     }
 
-    // Contains fields that have modopts/modreqs
-    struct FieldsWithModifiers
+    // Contains fields that are volatile
+    struct VolatileFields
     {
         private static volatile int s_vInt;
         private volatile int _vInt;
     }
 
+    // Accessors for fields that are volatile
+    static class AccessorsVolatile
+    {
+        [UnsafeAccessor(UnsafeAccessorKind.StaticField, Name="s_vInt")]
+        public extern static ref int GetStaticVolatileInt(ref VolatileFields f);
+
+        [UnsafeAccessor(UnsafeAccessorKind.Field, Name="_vInt")]
+        public extern static ref int GetVolatileInt(ref VolatileFields f);
+    }
+
     [Fact]
-    public static void Verify_AccessFieldsWithModifiers()
+    public static void Verify_AccessFieldsWithVolatile()
     {
-        Console.WriteLine($"Running {nameof(Verify_AccessFieldsWithModifiers)}");
+        Console.WriteLine($"Running {nameof(Verify_AccessFieldsWithVolatile)}");
 
-        FieldsWithModifiers fieldsWithModifiers = default;
+        VolatileFields fieldsWithVolatile = default;
 
-        GetStaticVolatileInt(ref fieldsWithModifiers) = default;
-        GetVolatileInt(ref fieldsWithModifiers) = default;
+        AccessorsVolatile.GetStaticVolatileInt(ref fieldsWithVolatile) = default;
+        AccessorsVolatile.GetVolatileInt(ref fieldsWithVolatile) = default;
+    }
 
-        [UnsafeAccessor(UnsafeAccessorKind.StaticField, Name="s_vInt")]
-        extern static ref int GetStaticVolatileInt(ref FieldsWithModifiers f);
+    // Contains fields that are readonly
+    readonly struct ReadOnlyFields
+    {
+        public static readonly int s_rInt;
+        public readonly int _rInt;
+    }
 
-        [UnsafeAccessor(UnsafeAccessorKind.Field, Name="_vInt")]
-        extern static ref int GetVolatileInt(ref FieldsWithModifiers f);
+    // Accessors for fields that are readonly
+    static class AccessorsReadOnly
+    {
+        [UnsafeAccessor(UnsafeAccessorKind.StaticField, Name="s_rInt")]
+        public extern static ref readonly int GetStaticReadOnlyInt(ref readonly ReadOnlyFields f);
+
+        [UnsafeAccessor(UnsafeAccessorKind.Field, Name="_rInt")]
+        public extern static ref readonly int GetReadOnlyInt(ref readonly ReadOnlyFields f);
+    }
+
+    [Fact]
+    public static void Verify_AccessFieldsWithReadOnlyRefs()
+    {
+        Console.WriteLine($"Running {nameof(Verify_AccessFieldsWithReadOnlyRefs)}");
+
+        ReadOnlyFields readOnlyFields = default;
+
+        Assert.True(Unsafe.AreSame(in AccessorsReadOnly.GetStaticReadOnlyInt(in readOnlyFields), in ReadOnlyFields.s_rInt));
+        Assert.True(Unsafe.AreSame(in AccessorsReadOnly.GetReadOnlyInt(in readOnlyFields), in readOnlyFields._rInt));
+
+        // Test the local declaration of the signature since it places modopts/modreqs differently.
+        Assert.True(Unsafe.AreSame(in GetStaticReadOnlyIntLocal(in readOnlyFields), in ReadOnlyFields.s_rInt));
+        Assert.True(Unsafe.AreSame(in GetReadOnlyIntLocal(in readOnlyFields), in readOnlyFields._rInt));
+
+        [UnsafeAccessor(UnsafeAccessorKind.StaticField, Name="s_rInt")]
+        extern static ref readonly int GetStaticReadOnlyIntLocal(ref readonly ReadOnlyFields f);
+
+        [UnsafeAccessor(UnsafeAccessorKind.Field, Name="_rInt")]
+        extern static ref readonly int GetReadOnlyIntLocal(ref readonly ReadOnlyFields f);
     }
 
     [Fact]