From ce8a4713a4d5fe95945dad1a203aff9d8c28cc67 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 5 Jan 2021 16:58:34 -0800 Subject: [PATCH 1/2] Delete reflection blocking on GetThreadDeserializationTracker Fixes https://github.com/mono/mono/issues/15112 --- .../src/System/Threading/Thread.CoreCLR.cs | 3 --- src/coreclr/vm/comsynchronizable.cpp | 24 ----------------- src/coreclr/vm/comsynchronizable.h | 1 - src/coreclr/vm/ecalllist.h | 1 - .../SerializationInfo.SerializationGuard.cs | 20 -------------- .../tests/SerializationGuardTests.cs | 27 ------------------- 6 files changed, 76 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs index 5ae142eda77ed3..1b79747dc0c9aa 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs @@ -162,9 +162,6 @@ partial void ThreadNameChanged(string? value) [DllImport(RuntimeHelpers.QCall, CharSet = CharSet.Unicode)] private static extern void InformThreadNameChange(ThreadHandle t, string? name, int len); - [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern DeserializationTracker GetThreadDeserializationTracker(ref StackCrawlMark stackMark); - /// Returns true if the thread has been started and is not dead. public extern bool IsAlive { diff --git a/src/coreclr/vm/comsynchronizable.cpp b/src/coreclr/vm/comsynchronizable.cpp index 9dceeb629de814..e2bd606886b0bb 100644 --- a/src/coreclr/vm/comsynchronizable.cpp +++ b/src/coreclr/vm/comsynchronizable.cpp @@ -1158,30 +1158,6 @@ BOOL QCALLTYPE ThreadNative::YieldThread() return ret; } -FCIMPL1(Object*, ThreadNative::GetThreadDeserializationTracker, StackCrawlMark* stackMark) -{ - FCALL_CONTRACT; - OBJECTREF refRetVal = NULL; - HELPER_METHOD_FRAME_BEGIN_RET_1(refRetVal) - - // To avoid reflection trying to bypass deserialization tracking, check the caller - // and only allow SerializationInfo to call into this method. - MethodTable* pCallerMT = SystemDomain::GetCallersType(stackMark); - if (pCallerMT != CoreLibBinder::GetClass(CLASS__SERIALIZATION_INFO)) - { - COMPlusThrowArgumentException(W("stackMark"), NULL); - } - - Thread* pThread = GetThread(); - - refRetVal = ObjectFromHandle(pThread->GetOrCreateDeserializationTracker()); - - HELPER_METHOD_FRAME_END(); - - return OBJECTREFToObject(refRetVal); -} -FCIMPLEND - FCIMPL0(INT32, ThreadNative::GetCurrentProcessorNumber) { FCALL_CONTRACT; diff --git a/src/coreclr/vm/comsynchronizable.h b/src/coreclr/vm/comsynchronizable.h index 514a12f7778060..e9968201b8bc20 100644 --- a/src/coreclr/vm/comsynchronizable.h +++ b/src/coreclr/vm/comsynchronizable.h @@ -97,7 +97,6 @@ friend class ThreadBaseObject; #endif //FEATURE_COMINTEROP static FCDECL1(FC_BOOL_RET,IsThreadpoolThread, ThreadBaseObject* thread); static FCDECL1(void, SetIsThreadpoolThread, ThreadBaseObject* thread); - static FCDECL1(Object*, GetThreadDeserializationTracker, StackCrawlMark* stackMark); static FCDECL0(INT32, GetCurrentProcessorNumber); diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index cd366a2ec14b94..d72d88e0c4a6f7 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -610,7 +610,6 @@ FCFuncStart(gThreadFuncs) FCFuncElement("Join", ThreadNative::Join) QCFuncElement("GetOptimalMaxSpinWaitsPerSpinIterationInternal", ThreadNative::GetOptimalMaxSpinWaitsPerSpinIteration) FCFuncElement("GetCurrentProcessorNumber", ThreadNative::GetCurrentProcessorNumber) - FCFuncElement("GetThreadDeserializationTracker", ThreadNative::GetThreadDeserializationTracker) FCFuncEnd() FCFuncStart(gThreadPoolFuncs) diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/Serialization/SerializationInfo.SerializationGuard.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/Serialization/SerializationInfo.SerializationGuard.cs index 5ea0f2f57ee8cf..6cc4c808a5ecfa 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/Serialization/SerializationInfo.SerializationGuard.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/Serialization/SerializationInfo.SerializationGuard.cs @@ -12,22 +12,15 @@ public sealed partial class SerializationInfo { internal static AsyncLocal AsyncDeserializationInProgress { get; } = new AsyncLocal(); -#if !CORECLR - // On AoT, assume private members are reflection blocked, so there's no further protection required - // for the thread's DeserializationTracker [ThreadStatic] private static DeserializationTracker? t_deserializationTracker; private static DeserializationTracker GetThreadDeserializationTracker() => t_deserializationTracker ??= new DeserializationTracker(); -#endif // !CORECLR // Returns true if deserialization is currently in progress public static bool DeserializationInProgress { -#if CORECLR - [DynamicSecurityMethod] // Methods containing StackCrawlMark local var must be marked DynamicSecurityMethod -#endif get { if (AsyncDeserializationInProgress.Value) @@ -35,12 +28,7 @@ public static bool DeserializationInProgress return true; } -#if CORECLR - StackCrawlMark stackMark = StackCrawlMark.LookForMe; - DeserializationTracker tracker = Thread.GetThreadDeserializationTracker(ref stackMark); -#else DeserializationTracker tracker = GetThreadDeserializationTracker(); -#endif bool result = tracker.DeserializationInProgress; return result; } @@ -100,19 +88,11 @@ public static void ThrowIfDeserializationInProgress(string switchSuffix, ref int // In this state, if the SerializationGuard or other related AppContext switches are set, // actions likely to be dangerous during deserialization, such as starting a process will be blocked. // Returns a DeserializationToken that must be disposed to remove the deserialization state. -#if CORECLR - [DynamicSecurityMethod] // Methods containing StackCrawlMark local var must be marked DynamicSecurityMethod -#endif public static DeserializationToken StartDeserialization() { if (LocalAppContextSwitches.SerializationGuard) { -#if CORECLR - StackCrawlMark stackMark = StackCrawlMark.LookForMe; - DeserializationTracker tracker = Thread.GetThreadDeserializationTracker(ref stackMark); -#else DeserializationTracker tracker = GetThreadDeserializationTracker(); -#endif if (!tracker.DeserializationInProgress) { lock (tracker) diff --git a/src/libraries/System.Runtime.Serialization.Formatters/tests/SerializationGuardTests.cs b/src/libraries/System.Runtime.Serialization.Formatters/tests/SerializationGuardTests.cs index 1d106217c0d7dc..31329577fb6374 100644 --- a/src/libraries/System.Runtime.Serialization.Formatters/tests/SerializationGuardTests.cs +++ b/src/libraries/System.Runtime.Serialization.Formatters/tests/SerializationGuardTests.cs @@ -33,33 +33,6 @@ public static void BlockFileWrites() TryPayload(new FileWriter()); } - [Fact] - [ActiveIssue("https://github.com/mono/mono/issues/15112", TestRuntimes.Mono)] - public static void BlockReflectionDodging() - { - // Ensure that the deserialization tracker cannot be called by reflection. - MethodInfo trackerMethod = typeof(Thread).GetMethod( - "GetThreadDeserializationTracker", - BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static); - - Assert.NotNull(trackerMethod); - - Assert.Equal(1, trackerMethod.GetParameters().Length); - object[] args = new object[1]; - args[0] = Enum.ToObject(typeof(Thread).Assembly.GetType("System.Threading.StackCrawlMark"), 0); - - try - { - object tracker = trackerMethod.Invoke(null, args); - throw new InvalidOperationException(tracker?.ToString() ?? "(null tracker returned)"); - } - catch (TargetInvocationException ex) - { - Exception baseEx = ex.GetBaseException(); - AssertExtensions.Throws("stackMark", () => throw baseEx); - } - } - [Fact] public static void BlockAsyncDodging() { From b4840c00a62f9554ba8da3bf750d2b8de8e49c90 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 5 Jan 2021 17:07:00 -0800 Subject: [PATCH 2/2] Delete more --- src/coreclr/vm/corelib.h | 3 --- src/coreclr/vm/threads.cpp | 33 --------------------------------- src/coreclr/vm/threads.h | 6 ------ 3 files changed, 42 deletions(-) diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index 412f5e3ecbc2a0..bf1b815cf238ca 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -446,9 +446,6 @@ DEFINE_METHOD(COMWRAPPERS, RELEASE_OBJECTS, CallReleaseObjects, DEFINE_METHOD(COMWRAPPERS, CALL_ICUSTOMQUERYINTERFACE, CallICustomQueryInterface, SM_Obj_RefGuid_RefIntPtr_RetInt) #endif //FEATURE_COMINTEROP -DEFINE_CLASS(SERIALIZATION_INFO, Serialization, SerializationInfo) -DEFINE_CLASS(DESERIALIZATION_TRACKER, Serialization, DeserializationTracker) - DEFINE_CLASS(IENUMERATOR, Collections, IEnumerator) DEFINE_CLASS(IENUMERABLE, Collections, IEnumerable) diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index 1ee00617fa11de..3d83157cc4026a 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -1593,7 +1593,6 @@ Thread::Thread() memset(&m_activityId, 0, sizeof(m_activityId)); #endif // FEATURE_PERFTRACING m_HijackReturnKind = RT_Illegal; - m_DeserializationTracker = NULL; m_currentPrepareCodeConfig = nullptr; m_isInForbidSuspendForDebuggerRegion = false; @@ -2630,11 +2629,6 @@ Thread::~Thread() // Destroy any handles that we're using to hold onto exception objects SafeSetThrowables(NULL); - if (m_DeserializationTracker != NULL) - { - DestroyGlobalStrongHandle(m_DeserializationTracker); - } - DestroyShortWeakHandle(m_ExposedObject); DestroyStrongHandle(m_StrongHndToExposedObject); } @@ -8544,30 +8538,3 @@ ThreadStore::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) } #endif // #ifdef DACCESS_COMPILE - -OBJECTHANDLE Thread::GetOrCreateDeserializationTracker() -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - } - CONTRACTL_END; - -#if !defined (DACCESS_COMPILE) - if (m_DeserializationTracker != NULL) - { - return m_DeserializationTracker; - } - - _ASSERTE(this == GetThread()); - - MethodTable* pMT = CoreLibBinder::GetClass(CLASS__DESERIALIZATION_TRACKER); - m_DeserializationTracker = CreateGlobalStrongHandle(AllocateObject(pMT)); - - _ASSERTE(m_DeserializationTracker != NULL); -#endif // !defined (DACCESS_COMPILE) - - return m_DeserializationTracker; -} diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 6f04f2b66b891b..200b7c59b29368 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -4605,12 +4605,6 @@ class Thread } #endif // FEATURE_HIJACK -public: - OBJECTHANDLE GetOrCreateDeserializationTracker(); - -private: - OBJECTHANDLE m_DeserializationTracker; - public: static uint64_t dead_threads_non_alloc_bytes;