From 5af82871432a210fb3f9641b480292e0ddb38d35 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Fri, 12 Jun 2020 12:19:02 -0700 Subject: [PATCH 1/3] Fix detection of static readonly field re-initialization via reflection Move the check for static readonly field re-initialization after the static constructor is triggered. Fixes #37796 --- src/coreclr/src/vm/invokeutil.cpp | 58 ++++++++++--------- src/coreclr/src/vm/reflectioninvocation.cpp | 36 +----------- src/coreclr/tests/issues.targets | 3 + .../SetValue/TrySetReadonlyStaticField2.cs | 51 ++++++++++++++++ .../TrySetReadonlyStaticField2.csproj | 9 +++ 5 files changed, 96 insertions(+), 61 deletions(-) create mode 100644 src/coreclr/tests/src/reflection/SetValue/TrySetReadonlyStaticField2.cs create mode 100644 src/coreclr/tests/src/reflection/SetValue/TrySetReadonlyStaticField2.csproj diff --git a/src/coreclr/src/vm/invokeutil.cpp b/src/coreclr/src/vm/invokeutil.cpp index 3d78bc685a625d..1345db095a2837 100644 --- a/src/coreclr/src/vm/invokeutil.cpp +++ b/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 (hasValue and value) - // Because you can't independantly set them for this type. - if (!declaringType.IsNull() && Nullable::IsNullableType(declaringType.GetMethodTable())) - COMPlusThrow(kNotSupportedException); - // call the OBJECTREF Throwable = NULL; @@ -954,27 +949,28 @@ void InvokeUtil::SetValidField(CorElementType fldType, { pDeclMT = declaringType.GetMethodTable(); + // We don't allow setting the field of nullable (hasValue and value) + // Because you can't independantly 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 insure + // 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) { diff --git a/src/coreclr/src/vm/reflectioninvocation.cpp b/src/coreclr/src/vm/reflectioninvocation.cpp index c530786aa38796..d25c06a275629d 100644 --- a/src/coreclr/src/vm/reflectioninvocation.cpp +++ b/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()) { diff --git a/src/coreclr/tests/issues.targets b/src/coreclr/tests/issues.targets index 7ddfdc09fed5f0..7dd418451e01e1 100644 --- a/src/coreclr/tests/issues.targets +++ b/src/coreclr/tests/issues.targets @@ -118,6 +118,9 @@ https://github.com/dotnet/runtime/issues/36850 + + https://github.com/dotnet/runtime/issues/37899 + This test is to verify we are running mono, and therefore only makes sense on mono. diff --git a/src/coreclr/tests/src/reflection/SetValue/TrySetReadonlyStaticField2.cs b/src/coreclr/tests/src/reflection/SetValue/TrySetReadonlyStaticField2.cs new file mode 100644 index 00000000000000..80b9b497b315a3 --- /dev/null +++ b/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; + } +} + + diff --git a/src/coreclr/tests/src/reflection/SetValue/TrySetReadonlyStaticField2.csproj b/src/coreclr/tests/src/reflection/SetValue/TrySetReadonlyStaticField2.csproj new file mode 100644 index 00000000000000..2e607efdde537a --- /dev/null +++ b/src/coreclr/tests/src/reflection/SetValue/TrySetReadonlyStaticField2.csproj @@ -0,0 +1,9 @@ + + + Exe + true + + + + + From 2a580ac57dbf9a995b418188953ff95ca147456c Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 15 Jun 2020 07:40:48 -0700 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Jan Vorlicek --- src/coreclr/src/vm/invokeutil.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/vm/invokeutil.cpp b/src/coreclr/src/vm/invokeutil.cpp index 1345db095a2837..a42f638cc84d99 100644 --- a/src/coreclr/src/vm/invokeutil.cpp +++ b/src/coreclr/src/vm/invokeutil.cpp @@ -950,7 +950,7 @@ void InvokeUtil::SetValidField(CorElementType fldType, pDeclMT = declaringType.GetMethodTable(); // We don't allow setting the field of nullable (hasValue and value) - // Because you can't independantly set them for this type. + // Because you can't independently set them for this type. if (Nullable::IsNullableType(pDeclMT)) COMPlusThrow(kNotSupportedException); @@ -1171,7 +1171,7 @@ 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 insure + // 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); From c0555679cda86cbb8a8d613ef385aebb3e8d46d6 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 15 Jun 2020 10:06:36 -0700 Subject: [PATCH 3/3] Fix test disable --- src/coreclr/tests/issues.targets | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/tests/issues.targets b/src/coreclr/tests/issues.targets index 7dd418451e01e1..fc52aac5a77813 100644 --- a/src/coreclr/tests/issues.targets +++ b/src/coreclr/tests/issues.targets @@ -118,9 +118,6 @@ https://github.com/dotnet/runtime/issues/36850 - - https://github.com/dotnet/runtime/issues/37899 - This test is to verify we are running mono, and therefore only makes sense on mono. @@ -1574,6 +1571,9 @@ needs triage + + https://github.com/dotnet/runtime/issues/37899 + needs triage