From 08f43654e9a7a41371b6e2a44d6716b8c9ecca27 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 3 Aug 2020 15:55:21 -0700 Subject: [PATCH 1/4] Run Cctor in Reflection Invoke cases correctly --- .../src/System/Reflection/INVOCATION_FLAGS.cs | 3 +- .../Reflection/RuntimeConstructorInfo.cs | 11 ++++++- .../tests/ConstructorInfoTests.cs | 33 +++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/INVOCATION_FLAGS.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/INVOCATION_FLAGS.cs index e7d9b97ac5c6c5..b6a55518914a22 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/INVOCATION_FLAGS.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/INVOCATION_FLAGS.cs @@ -14,7 +14,8 @@ internal enum INVOCATION_FLAGS : uint INVOCATION_FLAGS_INITIALIZED = 0x00000001, // it's used for both method and field to signify that no access is allowed INVOCATION_FLAGS_NO_INVOKE = 0x00000002, - /* unused 0x00000004 */ + // Set for static ctors, to ensure that the static ctor is run as a static ctor before it is explicitly executed via reflection + INVOCATION_FLAGS_RUN_CLASS_CONSTRUCTOR = 0x00000004, // Set for static ctors and ctors on abstract types, which // can be invoked only if the "this" object is provided (even if it's null). INVOCATION_FLAGS_NO_CTOR_INVOKE = 0x00000008, diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs index 9d0086ca442536..dc2faa2f692ce2 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs @@ -5,6 +5,7 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Globalization; +using System.Runtime.CompilerServices; using System.Text; using RuntimeTypeCache = System.RuntimeType.RuntimeTypeCache; @@ -47,7 +48,12 @@ internal INVOCATION_FLAGS InvocationFlags // We don't need other flags if this method cannot be invoked invocationFlags |= INVOCATION_FLAGS.INVOCATION_FLAGS_NO_INVOKE; } - else if (IsStatic || declaringType != null && declaringType.IsAbstract) + else if (IsStatic) + { + invocationFlags |= INVOCATION_FLAGS.INVOCATION_FLAGS_RUN_CLASS_CONSTRUCTOR | + INVOCATION_FLAGS.INVOCATION_FLAGS_NO_CTOR_INVOKE; + } + else if (declaringType != null && declaringType.IsAbstract) { invocationFlags |= INVOCATION_FLAGS.INVOCATION_FLAGS_NO_CTOR_INVOKE; } @@ -277,6 +283,9 @@ internal void ThrowNoInvokeException() if ((invocationFlags & INVOCATION_FLAGS.INVOCATION_FLAGS_NO_INVOKE) != 0) ThrowNoInvokeException(); + if ((invocationFlags & INVOCATION_FLAGS.INVOCATION_FLAGS_RUN_CLASS_CONSTRUCTOR) != 0) + RuntimeHelpers.RunClassConstructor(DeclaringType!.TypeHandle); + // check basic method consistency. This call will throw if there are problems in the target/method relationship CheckConsistency(obj); diff --git a/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs b/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs index 38a5e460718551..fea6915ca21ef3 100644 --- a/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs +++ b/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs @@ -68,6 +68,25 @@ public void Invoke_StaticConstructor_NullObject_NullParameters() Assert.Null(obj); } + [Fact] + public void Invoke_StaticConstructorMultipleTimes() + { + ConstructorInfo[] constructors = GetConstructors(typeof(ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection)); + Assert.Equal(1, constructors.Length); + // The first time the static cctor is called, it should run the cctor twice + // Once to initialize run the cctor as a cctor + // The second to run it as a method which is invoked. + Assert.Equal(0, ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection.VisibleStatics.s_cctorCallCount); + object obj = constructors[0].Invoke(null, new object[] { }); + Assert.Null(obj); + Assert.Equal(2, ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection.VisibleStatics.s_cctorCallCount); + + // Subsequent invocations of the static cctor should run the cctor only once + obj = constructors[0].Invoke(null, new object[] { }); + Assert.Null(obj); + Assert.Equal(3, ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection.VisibleStatics.s_cctorCallCount); + } + [Fact] [ActiveIssue("https://github.com/mono/mono/issues/15024", TestRuntimes.Mono)] public void Invoke_StaticConstructor_ThrowsMemberAccessException() @@ -236,6 +255,20 @@ public static class ClassWithStaticConstructor static ClassWithStaticConstructor() { } } + // Use this class only from the Invoke_StaticConstructorMultipleTimes method + public static class ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection + { + public static class VisibleStatics + { + public static int s_cctorCallCount; + } + + static ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection() + { + VisibleStatics.s_cctorCallCount++; + } + } + public struct StructWith1Constructor { public int x; From e1947e530b8d43d9717091daa54ea68c4fbde352 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 3 Aug 2020 17:50:10 -0700 Subject: [PATCH 2/4] Code Review feedback --- .../src/System/Reflection/RuntimeConstructorInfo.cs | 11 ++++++++--- .../System.Reflection/tests/ConstructorInfoTests.cs | 7 ++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs index dc2faa2f692ce2..2cddcb144bb177 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs @@ -283,12 +283,17 @@ internal void ThrowNoInvokeException() if ((invocationFlags & INVOCATION_FLAGS.INVOCATION_FLAGS_NO_INVOKE) != 0) ThrowNoInvokeException(); - if ((invocationFlags & INVOCATION_FLAGS.INVOCATION_FLAGS_RUN_CLASS_CONSTRUCTOR) != 0) - RuntimeHelpers.RunClassConstructor(DeclaringType!.TypeHandle); - // check basic method consistency. This call will throw if there are problems in the target/method relationship CheckConsistency(obj); + if ((invocationFlags & INVOCATION_FLAGS.INVOCATION_FLAGS_RUN_CLASS_CONSTRUCTOR) != 0) + { + // Run the class constructor through the class constructor mechanism instead of the Invoke path. + // This avoids allowing mutation of readonly static fields, and initializes the type correctly. + RuntimeHelpers.RunClassConstructor(DeclaringType!.TypeHandle); + return; + } + Signature sig = Signature; // get the signature diff --git a/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs b/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs index fea6915ca21ef3..ec9e56ecf3067a 100644 --- a/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs +++ b/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs @@ -79,12 +79,13 @@ public void Invoke_StaticConstructorMultipleTimes() Assert.Equal(0, ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection.VisibleStatics.s_cctorCallCount); object obj = constructors[0].Invoke(null, new object[] { }); Assert.Null(obj); - Assert.Equal(2, ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection.VisibleStatics.s_cctorCallCount); + Assert.Equal(1, ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection.VisibleStatics.s_cctorCallCount); - // Subsequent invocations of the static cctor should run the cctor only once + // Subsequent invocations of the static cctor should not run the cctor at all, as it has already executed + // and running multiple times opens up the possibility of modifying read only static data obj = constructors[0].Invoke(null, new object[] { }); Assert.Null(obj); - Assert.Equal(3, ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection.VisibleStatics.s_cctorCallCount); + Assert.Equal(1, ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection.VisibleStatics.s_cctorCallCount); } [Fact] From 66223d9c81649da66a642029a0edc73025f03c52 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 4 Aug 2020 11:34:39 -0700 Subject: [PATCH 3/4] More code review feedback --- .../src/System/Reflection/RuntimeConstructorInfo.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs index 2cddcb144bb177..7a8e2e71855e72 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs @@ -290,8 +290,14 @@ internal void ThrowNoInvokeException() { // Run the class constructor through the class constructor mechanism instead of the Invoke path. // This avoids allowing mutation of readonly static fields, and initializes the type correctly. - RuntimeHelpers.RunClassConstructor(DeclaringType!.TypeHandle); - return; + + var declaringType = DeclaringType; + + // Handle module ctors, by not running them. They are always executed automatically + if (declaringType != null) + RuntimeHelpers.RunClassConstructor(declaringType.TypeHandle); + + return null; } Signature sig = Signature; From e466a037e1c4f29e9961893c1e60373b4e4eabff Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 4 Aug 2020 16:51:22 -0700 Subject: [PATCH 4/4] Handle module ctors, and disable test on Mono --- .../src/System/Reflection/RuntimeConstructorInfo.cs | 3 ++- src/libraries/System.Reflection/tests/ConstructorInfoTests.cs | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs index 7a8e2e71855e72..e06251e6ad851b 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs @@ -293,9 +293,10 @@ internal void ThrowNoInvokeException() var declaringType = DeclaringType; - // Handle module ctors, by not running them. They are always executed automatically if (declaringType != null) RuntimeHelpers.RunClassConstructor(declaringType.TypeHandle); + else + RuntimeHelpers.RunModuleConstructor(Module.ModuleHandle); return null; } diff --git a/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs b/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs index ec9e56ecf3067a..a1042452810748 100644 --- a/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs +++ b/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs @@ -69,6 +69,7 @@ public void Invoke_StaticConstructor_NullObject_NullParameters() } [Fact] + [ActiveIssue("https://github.com/dotnet/runtime/issues/40351", TestRuntimes.Mono)] public void Invoke_StaticConstructorMultipleTimes() { ConstructorInfo[] constructors = GetConstructors(typeof(ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection));