From 15c2148000f3bf08e95b88f942e9875f74b64d1b Mon Sep 17 00:00:00 2001
From: Jan Kotas <jkotas@microsoft.com>
Date: Sat, 11 Feb 2017 14:25:04 -0800
Subject: [PATCH] Fix checks for methods that use StackCrawlMark (#9537)

* Fix checks for methods that use StackCrawlMark

My recent changed these methods to be marked using IsMdRequireSecObject instead, but some
places that have to check for it were not updated correctly.
---
 src/mscorlib/src/System/Security/Attributes.cs |  9 ++++--
 src/vm/jitinterface.cpp                        | 39 +++++++++-----------------
 2 files changed, 20 insertions(+), 28 deletions(-)

diff --git a/src/mscorlib/src/System/Security/Attributes.cs b/src/mscorlib/src/System/Security/Attributes.cs
index 9594013..81a1147 100644
--- a/src/mscorlib/src/System/Security/Attributes.cs
+++ b/src/mscorlib/src/System/Security/Attributes.cs
@@ -7,9 +7,12 @@ using  System.Runtime.InteropServices;
 namespace System.Security
 {
     // DynamicSecurityMethodAttribute:
-    //  Indicates that calling the target method requires space for a security
-    //  object to be allocated on the callers stack. This attribute is only ever
-    //  set on certain security methods defined within mscorlib.
+    //  All methods that use StackCrawlMark should be marked with this attribute. This attribute
+    //  disables inlining of the calling method to allow stackwalking to find the exact caller.
+    //
+    //  This attribute used to indicate that the target method requires space for a security object 
+    //  to be allocated on the callers stack. It is not used for this purpose anymore because of security 
+    //  stackwalks are not ever done in CoreCLR.
     [AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor, AllowMultiple = true, Inherited = false )] 
     sealed internal class DynamicSecurityMethodAttribute : System.Attribute
     {
diff --git a/src/vm/jitinterface.cpp b/src/vm/jitinterface.cpp
index e0c7143..62c3765 100644
--- a/src/vm/jitinterface.cpp
+++ b/src/vm/jitinterface.cpp
@@ -6732,23 +6732,21 @@ DWORD CEEInfo::getMethodAttribsInternal (CORINFO_METHOD_HANDLE ftn)
         result |= CORINFO_FLG_NOSECURITYWRAP;
     }
 
+    if (IsMdRequireSecObject(attribs))
+    {
+        // Assume all methods marked as DynamicSecurity are
+        // marked that way because they use StackCrawlMark to identify
+        // the caller.
+        // See comments in canInline or canTailCall
+        result |= CORINFO_FLG_DONT_INLINE_CALLER;
+    }
 
     // Check for an inlining directive.
     if (pMD->IsNotInline())
     {
         /* Function marked as not inlineable */
         result |= CORINFO_FLG_DONT_INLINE;
-
-        if (pMD->IsIL() && IsMdRequireSecObject(attribs))
-        {
-            // Assume all methods marked as DynamicSecurity inside mscorlib are
-            // marked that way because they use StackCrawlMark to identify
-            // the caller (not just the security info).
-            // See comments in canInline or canTailCall
-            result |= CORINFO_FLG_DONT_INLINE_CALLER;
-        }
     }
-
     // AggressiveInlining only makes sense for IL methods.
     else if (pMD->IsIL() && IsMiAggressiveInlining(pMD->GetImplAttrs()))
     {
@@ -8231,24 +8229,15 @@ bool CEEInfo::canTailCall (CORINFO_METHOD_HANDLE hCaller,
 
         // Methods with StackCrawlMark depend on finding their caller on the stack.
         // If we tail call one of these guys, they get confused.  For lack of
-        // a better way of identifying them, we look for methods marked as NoInlining
-        // inside mscorlib (StackCrawlMark is private), and assume it is one of these
-        // methods.  We have an assert in canInline that ensures all StackCrawlMark
+        // a better way of identifying them, we use DynamicSecurity attribute to identify
+        // them. We have an assert in canInline that ensures all StackCrawlMark
         // methods are appropriately marked.
         //
-        // NOTE that this is *NOT* a security issue because we check to ensure that
-        // the callee has the *SAME* security properties as the caller, it just might
-        // be from a different assembly which messes up APIs like Type.GetType, which
-        // for back-compat uses the assembly of it's caller to resolve unqualified
-        // typenames.
-        if ((pExactCallee != NULL) && pExactCallee->GetModule()->IsSystem() && pExactCallee->IsIL())
+        if ((pExactCallee != NULL) && IsMdRequireSecObject(pExactCallee->GetAttrs()))
         {
-            if (IsMiNoInlining(pExactCallee->GetImplAttrs()))
-            {
-                result = false;
-                szFailReason = "Callee might have a StackCrawlMark.LookForMyCaller";
-                goto exit;
-            }
+            result = false;
+            szFailReason = "Callee might have a StackCrawlMark.LookForMyCaller";
+            goto exit;
         }
     }
 
-- 
2.7.4