Delete reflection blocking on GetThreadDeserializationTracker (#46607)

Fixes https://github.com/mono/mono/issues/15112

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 5ae142e..1b79747 100644 (file)
--- 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 @@ namespace System.Threading
         [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);
-
         /// <summary>Returns true if the thread has been started and is not dead.</summary>
         public extern bool IsAlive
         {

diff --git a/src/coreclr/vm/comsynchronizable.cpp b/src/coreclr/vm/comsynchronizable.cpp
index 9dceeb6..e2bd606 100644 (file)
--- 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 514a12f..e996820 100644 (file)
--- a/src/coreclr/vm/comsynchronizable.h
+++ b/src/coreclr/vm/comsynchronizable.h
@@ -97,7 +97,6 @@ public:
 #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/corelib.h b/src/coreclr/vm/corelib.h
index 412f5e3..bf1b815 100644 (file)
--- 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/ecalllist.h b/src/coreclr/vm/ecalllist.h
index cd366a2..d72d88e 100644 (file)
--- 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/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp
index 1ee0061..3d83157 100644 (file)
--- 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 6f04f2b..200b7c5 100644 (file)
--- a/src/coreclr/vm/threads.h
+++ b/src/coreclr/vm/threads.h
@@ -4606,12 +4606,6 @@ public:
 #endif // FEATURE_HIJACK
 
 public:
-    OBJECTHANDLE GetOrCreateDeserializationTracker();
-
-private:
-    OBJECTHANDLE m_DeserializationTracker;
-
-public:
     static uint64_t dead_threads_non_alloc_bytes;
 
 #ifndef DACCESS_COMPILE

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 5ea0f2f..6cc4c80 100644 (file)
--- 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 @@ namespace System.Runtime.Serialization
     {
         internal static AsyncLocal<bool> AsyncDeserializationInProgress { get; } = new AsyncLocal<bool>();
 
-#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 @@ namespace System.Runtime.Serialization
                     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 @@ namespace System.Runtime.Serialization
         // 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 1d10621..3132957 100644 (file)
--- a/src/libraries/System.Runtime.Serialization.Formatters/tests/SerializationGuardTests.cs
+++ b/src/libraries/System.Runtime.Serialization.Formatters/tests/SerializationGuardTests.cs
@@ -34,33 +34,6 @@ namespace System.Runtime.Serialization.Formatters.Tests
         }
 
         [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<ArgumentException>("stackMark", () => throw baseEx);
-            }
-        }
-
-        [Fact]
         public static void BlockAsyncDodging()
         {
             TryPayload(new AsyncDodger());