Fix detection of static readonly field re-initialization via reflection

src/coreclr/src/vm/invokeutil.cpp

@@ -941,11 +941,6 @@ void InvokeUtil::SetValidField(CorElementType fldType,
     }
     CONTRACTL_END;
 
-    // We don't allow setting the field of nullable<T> (hasValue and value)
-    // Because you can't independantly set them for this type.
-    if (!declaringType.IsNull() && Nullable::IsNullableType(declaringType.GetMethodTable()))
-        COMPlusThrow(kNotSupportedException);
-
     // call the <cinit>
     OBJECTREF Throwable = NULL;
 
@@ -954,27 +949,28 @@ void InvokeUtil::SetValidField(CorElementType fldType,
     {
         pDeclMT = declaringType.GetMethodTable();
 
+        // We don't allow setting the field of nullable<T> (hasValue and value)
+        // Because you can't independently set them for this type.
+        if (Nullable::IsNullableType(pDeclMT))
+            COMPlusThrow(kNotSupportedException);
+
         if (pDeclMT->IsSharedByGenericInstantiations())
             COMPlusThrow(kNotSupportedException, W("NotSupported_Type"));
     }
+    else
+    {
+        pDeclMT = pField->GetModule()->GetGlobalMethodTable();
+    }
 
     if (*pDomainInitialized == FALSE)
     {
         EX_TRY
-        {
-        if (declaringType.IsNull())
-        {
-            pField->GetModule()->GetGlobalMethodTable()->EnsureInstanceActive();
-            pField->GetModule()->GetGlobalMethodTable()->CheckRunClassInitThrowing();
-        }
-        else
         {
             pDeclMT->EnsureInstanceActive();
             pDeclMT->CheckRunClassInitThrowing();
 
             *pDomainInitialized = TRUE;
         }
-        }
         EX_CATCH_THROWABLE(&Throwable);
     }
 #ifdef _DEBUG
@@ -990,6 +986,18 @@ void InvokeUtil::SetValidField(CorElementType fldType,
         GCPROTECT_END();
     }
 
+    // Verify we're not trying to set the value of a static initonly field
+    // once the class has been initialized.
+    if (pField->IsStatic() && pDeclMT->IsClassInited() && IsFdInitOnly(pField->GetAttributes()))
+    {
+        DefineFullyQualifiedNameForClassW();
+        SString ssFieldName(SString::Utf8, pField->GetName());
+        COMPlusThrow(kFieldAccessException,
+            IDS_EE_CANNOT_SET_INITONLY_STATIC_FIELD,
+            ssFieldName.GetUnicode(),
+            GetFullyQualifiedNameForClassW(pDeclMT));
+    }
+
     // Set the field
     ARG_SLOT value;
 
@@ -1162,27 +1170,29 @@ OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJ
     {
         pDeclMT = declaringType.GetMethodTable();
 
+        // We don't allow getting the field just so we don't have more specical
+        // cases than we need to.  Then we need at least the throw check to ensure
+        // we don't allow data corruption.
+        if (Nullable::IsNullableType(pDeclMT))
+            COMPlusThrow(kNotSupportedException);
+
         if (pDeclMT->IsSharedByGenericInstantiations())
             COMPlusThrow(kNotSupportedException, W("NotSupported_Type"));
     }
+    else
+    {
+        pDeclMT = pField->GetModule()->GetGlobalMethodTable();
+    }
 
     if (*pDomainInitialized == FALSE)
     {
         EX_TRY
-        {
-        if (declaringType.IsNull())
-        {
-            pField->GetModule()->GetGlobalMethodTable()->EnsureInstanceActive();
-            pField->GetModule()->GetGlobalMethodTable()->CheckRunClassInitThrowing();
-        }
-        else
         {
             pDeclMT->EnsureInstanceActive();
             pDeclMT->CheckRunClassInitThrowing();
 
             *pDomainInitialized = TRUE;
         }
-        }
         EX_CATCH_THROWABLE(&Throwable);
     }
 #ifdef _DEBUG
@@ -1199,12 +1209,6 @@ OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJ
         GCPROTECT_END();
     }
 
-    // We don't allow getting the field just so we don't have more specical
-    // cases than we need to.  The we need at least the throw check to insure
-    // we don't allow data corruption, but
-    if (!declaringType.IsNull() && Nullable::IsNullableType(pDeclMT))
-        COMPlusThrow(kNotSupportedException);
-
     CorElementType fieldElementType = pField->GetFieldType();
 
     switch (fieldElementType) {

src/coreclr/src/vm/reflectioninvocation.cpp

@@ -328,23 +328,6 @@ FCIMPL7(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Ob
 
     HELPER_METHOD_FRAME_BEGIN_PROTECT(gc);
 
-    // Verify we're not trying to set the value of a static initonly field
-    // once the class has been initialized.
-    if (pFieldDesc->IsStatic())
-    {
-        MethodTable* pEnclosingMT = pFieldDesc->GetEnclosingMethodTable();
-        if (pEnclosingMT->IsClassInited() && IsFdInitOnly(pFieldDesc->GetAttributes()))
-        {
-            DefineFullyQualifiedNameForClassW();
-            SString ssFieldName(SString::Utf8, pFieldDesc->GetName());
-            COMPlusThrow(kFieldAccessException,
-                IDS_EE_CANNOT_SET_INITONLY_STATIC_FIELD,
-                ssFieldName.GetUnicode(),
-                GetFullyQualifiedNameForClassW(pEnclosingMT));
-        }
-    }
-
-    //TODO: cleanup this function
     InvokeUtil::SetValidField(fieldType.GetSignatureCorElementType(), fieldType, pFieldDesc, &gc.target, &gc.value, declaringType, pDomainInitialized);
 
     HELPER_METHOD_FRAME_END();
@@ -1582,7 +1565,6 @@ static void DirectObjectFieldSet(FieldDesc *pField, TypeHandle fieldType, TypeHa
     // Validate the target/fld type relationship
     InvokeUtil::ValidateObjectTarget(pField, enclosingType, &objref);
 
-    InvokeUtil::ValidField(fieldType, pValue);
     InvokeUtil::SetValidField(pField->GetFieldType(), fieldType, pField, &objref, pValue, enclosingType, pDomainInitialized);
     GCPROTECT_END();
 }
@@ -1626,22 +1608,8 @@ FCIMPL5(void, RuntimeFieldHandle::SetValueDirect, ReflectFieldObject *pFieldUNSA
     TypeHandle targetType = pTarget->type;
     MethodTable *pEnclosingMT = contextType.GetMethodTable();
 
-    {
-        // Verify that the value passed can be widened into the target
-        InvokeUtil::ValidField(fieldType, &gc.oValue);
-
-        // Verify we're not trying to set the value of a static initonly field
-        // once the class has been initialized.
-        if (pField->IsStatic() && pEnclosingMT->IsClassInited() && IsFdInitOnly(pField->GetAttributes()))
-        {
-            DefineFullyQualifiedNameForClassW();
-            SString ssFieldName(SString::Utf8, pField->GetName());
-            COMPlusThrow(kFieldAccessException,
-                IDS_EE_CANNOT_SET_INITONLY_STATIC_FIELD,
-                ssFieldName.GetUnicode(),
-                GetFullyQualifiedNameForClassW(pEnclosingMT));
-        }
-    }
+    // Verify that the value passed can be widened into the target
+    InvokeUtil::ValidField(fieldType, &gc.oValue);
 
     CLR_BOOL domainInitialized = FALSE;
     if (pField->IsStatic() || !targetType.IsValueType()) {

src/coreclr/tests/issues.targets

@@ -1571,6 +1571,9 @@
         <ExcludeList Include="$(XunitTestBinBase)/reflection/SetValue/TrySetReadonlyStaticField/**">
             <Issue>needs triage</Issue>
         </ExcludeList>
+        <ExcludeList Include="$(XunitTestBinBase)/reflection/SetValue/TrySetReadonlyStaticField2/**">
+            <Issue>https://github.com/dotnet/runtime/issues/37899</Issue>
+        </ExcludeList>
         <ExcludeList Include="$(XunitTestBinBase)/Regressions/coreclr/15241/genericcontext/**">
             <Issue>needs triage</Issue>
         </ExcludeList>

src/coreclr/tests/src/reflection/SetValue/TrySetReadonlyStaticField2.cs

@@ -0,0 +1,51 @@
+// 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.
+
+using System;
+using System.Reflection;
+
+public class TestSetValue
+{
+    public static readonly long MagicNumber = 42;
+}
+
+public class TestSetValueDirect
+{
+    public static readonly string MagicString = "";
+}
+
+class Test
+{
+    public static int Main()
+    {
+        // Validate that the readonly static field cannot be set via reflection when the static constructor is triggered 
+        // by the reflection SetValue operation itself.
+
+        try
+        {
+            typeof(TestSetValue).GetField(nameof(TestSetValue.MagicNumber)).SetValue(null, 0x123456789);
+            Console.WriteLine("FAILED: TestSetValue - Exception expected");
+            return -1;
+        }
+        catch (FieldAccessException)
+        {
+            Console.WriteLine("TestSetValue - Caught expected exception");
+        }
+
+        try 
+        {
+            int i = 0;
+            typeof(TestSetValueDirect).GetField(nameof(TestSetValueDirect.MagicString)).SetValueDirect(__makeref(i), "Hello");
+            Console.WriteLine("FAILED: TestSetValueDirect - Exception expected");
+            return -1;
+        }
+        catch (FieldAccessException)
+        {
+            Console.WriteLine("TestSetValueDirect - Caught expected exception");
+        }
+        return 100;
+    }
+}
+
+

src/coreclr/tests/src/reflection/SetValue/TrySetReadonlyStaticField2.csproj

@@ -0,0 +1,9 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="TrySetReadonlyStaticField2.cs" />
+  </ItemGroup>
+</Project>