Skip to content

Commit

Permalink
Fix race condition caused by updating interface map with exact target (
Browse files Browse the repository at this point in the history
…#55308)

- Change interface map reading to reliably perform a volatile load (without barrier)
- When performing scan for exact interface match/match trhough SpecialMarkerTypeForGenericCasting, be careful to only read the map once and use that value for all comparisons
- All other uses of InterfaceMapIterator have been audited, and look to be safe
  • Loading branch information
davidwrighton committed Jul 8, 2021
1 parent f30c848 commit 9270b34
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 6 deletions.
6 changes: 4 additions & 2 deletions src/coreclr/vm/methodtable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -713,8 +713,10 @@ PTR_MethodTable InterfaceInfo_t::GetApproxMethodTable(Module * pContainingModule

return pItfMT;
}
#endif
MethodTable * pItfMT = m_pMethodTable.GetValue();
#else
MethodTable * pItfMT = GetMethodTable();
#endif
ClassLoader::EnsureLoaded(TypeHandle(pItfMT), CLASS_LOAD_APPROXPARENTS);
return pItfMT;
}
Expand Down Expand Up @@ -9269,7 +9271,7 @@ MethodTable::ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc*
{
if (it.CurrentInterfaceMatches(this, pInterfaceType))
{
// This is the variant interface check logic, skip the
// This is the variant interface check logic, skip the exact matches as they were handled above
continue;
}

Expand Down
24 changes: 21 additions & 3 deletions src/coreclr/vm/methodtable.h
Original file line number Diff line number Diff line change
Expand Up @@ -105,24 +105,36 @@ struct InterfaceInfo_t
#endif

// Method table of the interface
#ifdef FEATURE_PREJIT
#if defined(FEATURE_NGEN_RELOCS_OPTIMIZATIONS)
RelativeFixupPointer<PTR_MethodTable> m_pMethodTable;
#else
FixupPointer<PTR_MethodTable> m_pMethodTable;
#endif
#else
PTR_MethodTable m_pMethodTable;
#endif

public:
FORCEINLINE PTR_MethodTable GetMethodTable()
{
LIMITED_METHOD_CONTRACT;
#ifdef FEATURE_PREJIT
return ReadPointerMaybeNull(this, &InterfaceInfo_t::m_pMethodTable);
#else
return VolatileLoadWithoutBarrier(&m_pMethodTable);
#endif
}

#ifndef DACCESS_COMPILE
void SetMethodTable(MethodTable * pMT)
{
LIMITED_METHOD_CONTRACT;
#ifdef FEATURE_PREJIT
m_pMethodTable.SetValueMaybeNull(pMT);
#else
return VolatileStoreWithoutBarrier(&m_pMethodTable, pMT);
#endif
}

// Get approximate method table. This is used by the type loader before the type is fully loaded.
Expand All @@ -132,7 +144,11 @@ struct InterfaceInfo_t
#ifndef DACCESS_COMPILE
InterfaceInfo_t(InterfaceInfo_t &right)
{
#ifdef FEATURE_PREJIT
m_pMethodTable.SetValueMaybeNull(right.m_pMethodTable.GetValueMaybeNull());
#else
VolatileStoreWithoutBarrier(&m_pMethodTable, VolatileLoadWithoutBarrier(&right.m_pMethodTable));
#endif
}
#else // !DACCESS_COMPILE
private:
Expand Down Expand Up @@ -2222,12 +2238,14 @@ class MethodTable
}
CONTRACT_END;

bool exactMatch = m_pMap->GetMethodTable() == pMT;
MethodTable *pCurrentMethodTable = m_pMap->GetMethodTable();

bool exactMatch = pCurrentMethodTable == pMT;
if (!exactMatch)
{
if (m_pMap->GetMethodTable()->HasSameTypeDefAs(pMT) &&
if (pCurrentMethodTable->HasSameTypeDefAs(pMT) &&
pMT->HasInstantiation() &&
m_pMap->GetMethodTable()->IsSpecialMarkerTypeForGenericCasting() &&
pCurrentMethodTable->IsSpecialMarkerTypeForGenericCasting() &&
pMT->GetInstantiation().ContainsAllOneType(pMTOwner))
{
exactMatch = true;
Expand Down
11 changes: 10 additions & 1 deletion src/coreclr/vm/methodtable.inl
Original file line number Diff line number Diff line change
Expand Up @@ -1578,7 +1578,16 @@ FORCEINLINE BOOL MethodTable::ImplementsInterfaceInline(MethodTable *pInterface)

do
{
if (pInfo->GetMethodTable()->HasSameTypeDefAs(pInterface) && pInfo->GetMethodTable()->IsSpecialMarkerTypeForGenericCasting())
MethodTable *pInterfaceInMap = pInfo->GetMethodTable();
if (pInterfaceInMap == pInterface)
{
// Since there is no locking on updating the interface with an exact match
// It is possible to reach here for a match which would ideally have been handled above
// GetMethodTable uses a VolatileLoadWithoutBarrier to prevent compiler optimizations
// from interfering with this check
return TRUE;
}
if (pInterfaceInMap->HasSameTypeDefAs(pInterface) && pInterfaceInMap->IsSpecialMarkerTypeForGenericCasting())
{
// Extensible RCW's need to be handled specially because they can have interfaces
// in their map that are added at runtime. These interfaces will have a start offset
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/vm/siginfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1417,11 +1417,13 @@ TypeHandle SigPointer::GetTypeHandleThrowing(

Instantiation genericLoadInst(thisinst, ntypars);

#ifndef FEATURE_PREJIT // PREJIT doesn't support the volatile update semantics in the interface map that this requires
if (pMTInterfaceMapOwner != NULL && genericLoadInst.ContainsAllOneType(pMTInterfaceMapOwner))
{
thRet = ClassLoader::LoadTypeDefThrowing(pGenericTypeModule, tkGenericType, ClassLoader::ThrowIfNotFound, ClassLoader::PermitUninstDefOrRef, 0, level);
}
else
#endif // FEATURE_PREJIT
{
// Group together the current signature type context and substitution chain, which
// we may later use to instantiate constraints of type arguments that turn out to be
Expand Down

0 comments on commit 9270b34

Please sign in to comment.