Fix use-out-of-scope in signature comparison (#88928)

* Propagate token list to callers by copying instead of passing an out-of-scope address

* Create a temp token list when we create a temp compare state.

* Update the default constructor of CompareState to set a more useful empty state that won't segfault.

* Revert "Update the default constructor of CompareState to set a more useful empty state that won't segfault."

This reverts commit 28b3fd5dbd66acb9f524d87758fc8efef78cb17d.

* Fix last case of using the default constructor in prestub.cpp

diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp
index eecc539..2985432 100644 (file)
--- a/src/coreclr/vm/prestub.cpp
+++ b/src/coreclr/vm/prestub.cpp
@@ -1283,7 +1283,8 @@ namespace
                 continue;
 
             // Check signature
-            MetaSig::CompareState state{};
+            TokenPairList list { nullptr };
+            MetaSig::CompareState state{ &list };
             state.IgnoreCustomModifiers = ignoreCustomModifiers;
             if (!DoesMethodMatchUnsafeAccessorDeclaration(cxt, curr, state))
                 continue;

diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp
index b64bc86..0d1e161 100644 (file)
--- a/src/coreclr/vm/siginfo.cpp
+++ b/src/coreclr/vm/siginfo.cpp
@@ -3662,7 +3662,8 @@ MetaSig::CompareElementType(
     }
     CONTRACTL_END
 
-    CompareState temp{};
+    TokenPairList tempList { nullptr };
+    CompareState temp{ &tempList };
     if (state == NULL)
         state = &temp;
 
@@ -3967,7 +3968,7 @@ MetaSig::CompareElementType(
             argCnt1++;
 
             TokenPairList newVisited = TokenPairList::AdjustForTypeEquivalenceForbiddenScope(state->Visited);
-            state->Visited = &newVisited;
+            *state->Visited = newVisited;
 
             // Compare all parameters, incl. return parameter
             while (argCnt1 > 0)
@@ -3998,7 +3999,7 @@ MetaSig::CompareElementType(
                 pSig1 - 1,
                 (DWORD)(pEndSig1 - pSig1) + 1);
             TokenPairList newVisitedAlwaysForbidden = TokenPairList::AdjustForTypeEquivalenceForbiddenScope(state->Visited);
-            state->Visited = &newVisitedAlwaysForbidden;
+            *state->Visited = newVisitedAlwaysForbidden;
 
             // Type constructors - The actual type is never permitted to participate in type equivalence.
             if (!CompareElementType(
@@ -4024,7 +4025,7 @@ MetaSig::CompareElementType(
                 return FALSE;
             }
 
-            state->Visited = &newVisited;
+            *state->Visited = newVisited;
             while (argCnt1 > 0)
             {
                 if (!CompareElementType(
@@ -4203,7 +4204,8 @@ MetaSig::CompareTypeDefsUnderSubstitutions(
     SigPointer inst1 = pSubst1->GetInst();
     SigPointer inst2 = pSubst2->GetInst();
 
-    CompareState state{ pVisited };
+    TokenPairList visited { pVisited };
+    CompareState state{ &visited };
     for (DWORD i = 0; i < pTypeDef1->GetNumGenericArgs(); i++)
     {
         PCCOR_SIGNATURE startInst1 = inst1.GetPtr();
@@ -4409,6 +4411,8 @@ MetaSig::CompareMethodSigs(
     IfFailThrow(CorSigUncompressData_EndPtr(pSig1, pEndSig1, &ArgCount1));
     IfFailThrow(CorSigUncompressData_EndPtr(pSig2, pEndSig2, &ArgCount2));
 
+    TokenPairList visited{ pVisited };
+
     if (ArgCount1 != ArgCount2)
     {
         if ((callConv & IMAGE_CEE_CS_CALLCONV_MASK) != IMAGE_CEE_CS_CALLCONV_VARARG)
@@ -4430,7 +4434,7 @@ MetaSig::CompareMethodSigs(
         // to correctly handle overloads, where there are a number of varargs methods
         // to pick from, like m1(int,...) and m2(int,int,...), etc.
 
-        CompareState state{ pVisited };
+        CompareState state{ &visited };
         // <= because we want to include a check of the return value!
         for (i = 0; i <= ArgCount1; i++)
         {
@@ -4486,7 +4490,7 @@ MetaSig::CompareMethodSigs(
     }
 
     // do return type as well
-    CompareState state{ pVisited };
+    CompareState state{ &visited };
     for (i = 0; i <= ArgCount1; i++)
     {
         if (i == 0 && skipReturnTypeSig)
@@ -4551,7 +4555,8 @@ BOOL MetaSig::CompareFieldSigs(
     pEndSig1 = pSig1 + cSig1;
     pEndSig2 = pSig2 + cSig2;
 
-    CompareState state{ pVisited };
+    TokenPairList visited { pVisited };
+    CompareState state{ &visited };
     return(CompareElementType(++pSig1, ++pSig2, pEndSig1, pEndSig2, pModule1, pModule2, NULL, NULL, &state));
 }
 
@@ -4865,11 +4870,12 @@ BOOL MetaSig::CompareVariableConstraints(const Substitution *pSubst1,
         // because they
         // a) are vacuous, and
         // b) may be implicit (ie. absent) in the overridden variable's declaration
+        TokenPairList newVisited { nullptr };
         if (!(CompareTypeDefOrRefOrSpec(pModule1, tkConstraintType1, NULL,
-                                       CoreLibBinder::GetModule(), g_pObjectClass->GetCl(), NULL, NULL) ||
+                                       CoreLibBinder::GetModule(), g_pObjectClass->GetCl(), NULL, &newVisited) ||
           (((specialConstraints1 & gpNotNullableValueTypeConstraint) != 0) &&
            (CompareTypeDefOrRefOrSpec(pModule1, tkConstraintType1, NULL,
-                      CoreLibBinder::GetModule(), g_pValueTypeClass->GetCl(), NULL, NULL)))))
+                      CoreLibBinder::GetModule(), g_pValueTypeClass->GetCl(), NULL, &newVisited)))))
         {
             HENUMInternalHolder hEnum2(pInternalImport2);
             mdGenericParamConstraint tkConstraint2;
@@ -4882,7 +4888,7 @@ BOOL MetaSig::CompareVariableConstraints(const Substitution *pSubst1,
                 IfFailThrow(pInternalImport2->GetGenericParamConstraintProps(tkConstraint2, &tkParam2, &tkConstraintType2));
                 _ASSERTE(tkParam2 == tok2);
 
-                found = CompareTypeDefOrRefOrSpec(pModule1, tkConstraintType1, pSubst1, pModule2, tkConstraintType2, pSubst2, NULL);
+                found = CompareTypeDefOrRefOrSpec(pModule1, tkConstraintType1, pSubst1, pModule2, tkConstraintType2, pSubst2, &newVisited);
             }
             if (!found)
             {

diff --git a/src/coreclr/vm/siginfo.hpp b/src/coreclr/vm/siginfo.hpp
index 7ea7ada..d33cde5 100644 (file)
--- a/src/coreclr/vm/siginfo.hpp
+++ b/src/coreclr/vm/siginfo.hpp
@@ -432,9 +432,10 @@ public:
 // infinite recursion when types refer to each other in a cycle, e.g. a delegate that takes itself as
 // a parameter or a struct that declares a field of itself (illegal but we don't know at this point).
 //
-class TokenPairList
+class TokenPairList final
 {
 public:
+
     // Chain using this constructor when comparing two typedefs for equivalence.
     TokenPairList(mdToken token1, ModuleBase *pModule1, mdToken token2, ModuleBase *pModule2, TokenPairList *pNext)
         : m_token1(token1), m_token2(token2),
@@ -470,7 +471,6 @@ public:
     static TokenPairList AdjustForTypeSpec(TokenPairList *pTemplate, ModuleBase *pTypeSpecModule, PCCOR_SIGNATURE pTypeSpecSig, DWORD cbTypeSpecSig);
     static TokenPairList AdjustForTypeEquivalenceForbiddenScope(TokenPairList *pTemplate);
 
-private:
     TokenPairList(TokenPairList *pTemplate)
         : m_token1(pTemplate ? pTemplate->m_token1 : mdTokenNil),
           m_token2(pTemplate ? pTemplate->m_token2 : mdTokenNil),
@@ -480,6 +480,7 @@ private:
           m_pNext(pTemplate ? pTemplate->m_pNext : NULL)
     { LIMITED_METHOD_CONTRACT; }
 
+private:
     mdToken m_token1, m_token2;
     ModuleBase *m_pModule1, *m_pModule2;
     BOOL m_bInTypeEquivalenceForbiddenScope;