From ef248616966bda94756dfba571583002d16a07ae Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 14 Jul 2023 11:57:35 -0700 Subject: [PATCH 1/5] Propagate token list to callers by copying instead of passing an out-of-scope address --- src/coreclr/vm/siginfo.cpp | 25 +++++++++++++++---------- src/coreclr/vm/siginfo.hpp | 5 +++-- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index b64bc861b0c1c0..e777966f4994b4 100644 --- a/src/coreclr/vm/siginfo.cpp +++ b/src/coreclr/vm/siginfo.cpp @@ -3967,7 +3967,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 +3998,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 +4024,7 @@ MetaSig::CompareElementType( return FALSE; } - state->Visited = &newVisited; + *state->Visited = newVisited; while (argCnt1 > 0) { if (!CompareElementType( @@ -4203,7 +4203,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 +4410,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 +4433,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 +4489,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 +4554,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 +4869,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 +4887,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 7ea7ada7cb9e2f..d33cde56c99dde 100644 --- a/src/coreclr/vm/siginfo.hpp +++ b/src/coreclr/vm/siginfo.hpp @@ -432,9 +432,10 @@ class Substitution // 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 @@ class TokenPairList 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 @@ class TokenPairList m_pNext(pTemplate ? pTemplate->m_pNext : NULL) { LIMITED_METHOD_CONTRACT; } +private: mdToken m_token1, m_token2; ModuleBase *m_pModule1, *m_pModule2; BOOL m_bInTypeEquivalenceForbiddenScope; From 3a37f90dc6c69bf20900fd99f202f9b66cd681a3 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 14 Jul 2023 13:51:58 -0700 Subject: [PATCH 2/5] Create a temp token list when we create a temp compare state. --- src/coreclr/vm/siginfo.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index e777966f4994b4..0d1e161c559e64 100644 --- 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; From 28b3fd5dbd66acb9f524d87758fc8efef78cb17d Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 14 Jul 2023 15:56:40 -0700 Subject: [PATCH 3/5] Update the default constructor of CompareState to set a more useful empty state that won't segfault. --- src/coreclr/vm/siginfo.cpp | 3 +-- src/coreclr/vm/siginfo.hpp | 5 ++++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index 0d1e161c559e64..230953acc97ded 100644 --- a/src/coreclr/vm/siginfo.cpp +++ b/src/coreclr/vm/siginfo.cpp @@ -3662,8 +3662,7 @@ MetaSig::CompareElementType( } CONTRACTL_END - TokenPairList tempList { nullptr }; - CompareState temp{ &tempList }; + CompareState temp{ }; if (state == NULL) state = &temp; diff --git a/src/coreclr/vm/siginfo.hpp b/src/coreclr/vm/siginfo.hpp index d33cde56c99dde..fc824209dcf0c7 100644 --- a/src/coreclr/vm/siginfo.hpp +++ b/src/coreclr/vm/siginfo.hpp @@ -957,7 +957,10 @@ class MetaSig // be compared. bool IgnoreCustomModifiers; - CompareState() = default; + CompareState() + : Visited{ nullptr } + , IgnoreCustomModifiers{ false } + { } CompareState(TokenPairList* list) : Visited{ list } From 28810e01c2d0ea445634c239617ffc7d46c0f9e0 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 14 Jul 2023 16:11:30 -0700 Subject: [PATCH 4/5] Revert "Update the default constructor of CompareState to set a more useful empty state that won't segfault." This reverts commit 28b3fd5dbd66acb9f524d87758fc8efef78cb17d. --- src/coreclr/vm/siginfo.cpp | 3 ++- src/coreclr/vm/siginfo.hpp | 5 +---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index 230953acc97ded..0d1e161c559e64 100644 --- 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; diff --git a/src/coreclr/vm/siginfo.hpp b/src/coreclr/vm/siginfo.hpp index fc824209dcf0c7..d33cde56c99dde 100644 --- a/src/coreclr/vm/siginfo.hpp +++ b/src/coreclr/vm/siginfo.hpp @@ -957,10 +957,7 @@ class MetaSig // be compared. bool IgnoreCustomModifiers; - CompareState() - : Visited{ nullptr } - , IgnoreCustomModifiers{ false } - { } + CompareState() = default; CompareState(TokenPairList* list) : Visited{ list } From 7fb3e0c6d7ca27fab37c8ee40ea0a3814e9becab Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 14 Jul 2023 16:13:05 -0700 Subject: [PATCH 5/5] Fix last case of using the default constructor in prestub.cpp --- src/coreclr/vm/prestub.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp index a9b56f611744fb..96665dd3ac3889 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -1277,7 +1277,8 @@ namespace continue; // Check signature - MetaSig::CompareState state{}; + TokenPairList list { nullptr }; + MetaSig::CompareState state{ &list }; state.IgnoreCustomModifiers = ignoreCustomModifiers; if (!DoesMethodMatchUnsafeAccessorDeclaration(cxt, curr, state)) continue;