RVA fields can be set in Mono; should throw FieldAccessException instead

[steveharter]

In the Core runtime, a FieldAccessException is thrown.

Found by newly added tests in #97784

Repro

FieldInfo fieldInfo = typeof(MyStruct).GetField(nameof(MyStruct.s_rvaIntField));
fieldInfo.SetValue(null, MyStruct.s_rvaIntField);
// Result: no exception

public struct MyStruct
{
    public static readonly int[] s_rvaIntField = [1, 2];

    public MyStruct() { }
}

[lambdageek]

Note that CoreCLR allows using reflection to initialize a static readonly field from the static cctor:

using System.Reflection;

public class C {
    public static void Main() {
       Console.WriteLine ($"s_rvaIntField is {MyStruct.s_rvaIntField}");
   try {
FieldInfo fieldInfo = typeof(MyStruct).GetField(nameof(MyStruct.s_rvaIntField));
fieldInfo.SetValue(null, MyStruct.s_rvaIntField);
// Result: no exception
    } catch (FieldAccessException exn) {
      Console.WriteLine ($"Caught: {exn}");
    }
    try {
       FieldInfo fieldInfo = typeof(MyStruct).GetField(nameof(MyStruct.s_Two));
       fieldInfo.SetValue(null, MyStruct.s_Two);
    } catch (FieldAccessException exn) {
       Console.WriteLine ($"Caught: {exn}");
    }
  }
}

public struct MyStruct
{
    public static readonly int s_rvaIntField;
    public const int s_Two = 42;

    public MyStruct() { }
    static MyStruct () {
       FieldInfo fieldInfo = typeof(MyStruct).GetField(nameof(MyStruct.s_rvaIntField));
       fieldInfo.SetValue(null, 37);
    }
}

The output is:

s_rvaIntField is 37
Caught: System.FieldAccessException: Cannot set initonly static field 's_rvaIntField' after type 'MyStruct' is initialized.

That is, the static constructor runs successfully and doesn't throw. And then Main runs and it does throw.

So my naive suggestion below won't work exactly how CoreCLR works. But we should still add a check for literal fields there.

---

Somewhere around here

runtime/src/mono/mono/metadata/icall.c

Lines 2134 to 2138 in df0778d

	ves_icall_RuntimeFieldInfo_SetValueInternal (MonoReflectionFieldHandle field, MonoObjectHandle obj, MonoObjectHandle value, MonoError *error)
	{
	MonoClassField *cf = MONO_HANDLE_GETVAL (field, field);
	MonoType *type = mono_field_get_type_checked (cf, error);
	return_if_nok (error);

we need to check for type->attrs & FIELD_ATTRIBUTE_INIT_ONLY (and also for FIELD_ATTRIBUTE_LITERAL ! CoreCLR throws for that one too) and disallow the assignment with a

mono_error_set_generic_error (error, "System", "FieldAccessException", "...", field->name);

The CoreCLR errors are:

System.FieldAccessException: Cannot set initonly static field 's_rvaIntField' after type 'MyStruct' is initialized.

and

System.FieldAccessException: Cannot set a constant field.

[lambdageek]

Also the example in the issue description isn't actually an RVA field. For whatever reason, roslyn ends up emitting an initializer for that field into the cctor. But with three elements in the array, you do get some RVA data. But it's not for s_rvaIntField - but for '<PrivateImplementationDetails>'.'4636993D3E1DA4E9D6B8F87B79E8F7C6D018580D52661950EABC3845C5897A4D'

s_rvaIntField is initialized from the <PID> field in the cctor for MyStruct

SharpLab: https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBLANgHwAEAGAAkIEYA6AJRgDNcYwNsIA7AbgFgAoPvoQDMpAM4YoAVxakAsgE8AyhOkY+Abz6lt5EZSSlYAQwAmHXPNLZ2GANoBdMQH0AKgHcIANSO5JMUaQAvKS2FGgATPY8vDq65BQGxmbsFlY2Ds5QAG5GAJI2AGLYMLgmQSFh4WhCUQIxOsJySiosABQAlKTqpAC+fH28QA

I guess we should also prevent someone from accessing <PrivateImplementationDetails> and assigning to '4636993D3E1DA4E9D6B8F87B79E8F7C6D018580D52661950EABC3845C5897A4D' - so we should also check for FIELD_ATTRIBUTE_HAS_FIELD_RVA I guess

[lambdageek]

/cc @mangod9

[lambdageek]

Even this works:

public struct MyStruct
{
    public static readonly int s_rvaIntField;
    public const int s_Two = 42;

    public MyStruct() { }
    static MyStruct () {
	SneakyInitializer();
   }
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void SneakyInitializer()
    {
	FieldInfo fieldInfo = typeof(MyStruct).GetField(nameof(MyStruct.s_rvaIntField));
	fieldInfo.SetValue(null, 37);
 
    }
}

@jkotas does CoreCLR use some runtime flag on a class with readonly static fields to decide if setting them is allowed? Is it the same mechanism that detects if a static cctor is invoked cyclically?

That is, in mono it would be a check like this:

static bool
allow_init_only_set_value (MonoClassVTable *vtable)
{
   bool allow = false;
   mono_type_initialization_lock ();
   if (!vtable->initialized) {
     TypeInitializatioinLock *lock = (TypeInitializationLock *)g_hash_table_lookup (type_initialization_hash, vtable);
     allow = lock && mono_native_thread_id_equals (lock->initializing_tid, current_tid);
  }
  mono_type_initialization_unlock ();
  return allow;
}

[jkotas]

@jkotas does CoreCLR use some runtime flag on a class with readonly static fields to decide if setting them is allowed? Is it the same mechanism that detects if a static cctor is invoked cyclically?

This issue is duplicate of #37903

Yes, the CoreCLR check piggy backs on the mechanism that detects if a static cctor is invoked cyclically:

1. Trigger the static initialization
2. Check whether the static initialization finished. If the static initialization finished, block the field set and throw exception. If the static initialization is not finished, it means that we are inside the static initialization - allow the field set.

[lambdageek]

This issue is duplicate of #37903

Yes, thanks.

I didn't take enough notes in #37903 - it looks like I thought we needed something in the JIT and possibly a separate flag. There might be two separate issues:

1. A check for reflection - mostly as described by this issue.
2. A check for the execution engines - need to try an IL sample that does a stsfld to an initonly field from a method that is not the cctor. I thought I saw some logic in method-to-ir to check for that case. But it's hard to verify with C# code as in Roslyn it's a compile time error to write to a read-only field outside the cctor

[jkotas]

A check for the execution engines - need to try an IL sample

That would be invalid IL. We do not try to detect and throw exceptions for all invalid IL in CoreCLR. Invalid IL is UB in CoreCLR.