[release/10.0] Add LOCKREADIFFAILRET to unlocked MDInternalRW read methods (#128671)

Backport of #127958 to release/10.0

/cc @elinor-fung @kevingosse

## Customer Impact

- [x] Customer reported
- [ ] Found internally

Issue: #127957

Some read operations for metadata tables were not done under a lock.
When the tables are expanded - for example via profiler emit operations
- concurrent reads can end up with invalid pointers resulting in a
segfault or a MissingMethodException.

DataDog has been hitting it multiple times a week in CI and has gotten
customer reports as well.

## Regression

- [ ] Yes
- [x] No

## Testing

@andrewlock validated against CI that was hitting this issue without
this fix:
#127958 (comment)
@kevingosse validated against local changes that were reproing the issue
before his fix:
#127958 (comment)

## Risk

Low.

---------

Co-authored-by: Kevin Gosse <krix33@gmail.com>
Co-authored-by: Noah Falk <noahfalk@microsoft.com>

Author: 3 people

src/coreclr/md/enc/mdinternalrw.cpp

@@ -1566,10 +1566,13 @@ MDInternalRW::GetCustomAttributeProps( // S_OK or error.
     mdToken     *pTkType)               // Put attribute type here.
 {
     HRESULT hr;
-    // Getting the custom value prop with a token, no need to lock!
 
     _ASSERTE(TypeFromToken(at) == mdtCustomAttribute);
 
+    *pTkType = mdTokenNil;
+
+    LOCKREADIFFAILRET();
+
     // Do a linear search on compressed version as we do not want to
     // depend on ICR.
     //
@@ -1591,10 +1594,14 @@ MDInternalRW::GetCustomAttributeAsBlob(
     void const  **ppBlob,               // [OUT] return the pointer to internal blob
     ULONG       *pcbSize)               // [OUT] return the size of the blob
 {
-    // Getting the custom value prop with a token, no need to lock!
     HRESULT hr;
     _ASSERTE(ppBlob && pcbSize && TypeFromToken(cv) == mdtCustomAttribute);
 
+    *ppBlob = NULL;
+    *pcbSize = 0;
+
+    LOCKREADIFFAILRET();
+
     CustomAttributeRec *pCustomAttributeRec;
 
     IfFailRet(m_pStgdb->m_MiniMd.GetCustomAttributeRecord(RidFromToken(cv), &pCustomAttributeRec));
@@ -1805,7 +1812,6 @@ MDInternalRW::GetNameOfTypeDef(     // return hresult
     LPCSTR*     pszname,            // pointer to an internal UTF8 string
     LPCSTR*     psznamespace)       // pointer to the namespace.
 {
-    // No need to lock this method.
     HRESULT hr;
 
     if (pszname != NULL)
@@ -1819,6 +1825,8 @@ MDInternalRW::GetNameOfTypeDef(     // return hresult
 
     if (TypeFromToken(classdef) == mdtTypeDef)
     {
+        LOCKREADIFFAILRET();
+
         TypeDefRec *pTypeDefRec;
         IfFailRet(m_pStgdb->m_MiniMd.GetTypeDefRecord(RidFromToken(classdef), &pTypeDefRec));
 
@@ -1898,10 +1906,12 @@ MDInternalRW::GetNameOfMethodDef(
     mdMethodDef md,
     LPCSTR     *pszMethodName)
 {
-    // name of method will not change. So no need to lock
     HRESULT      hr;
-    MethodRec *pMethodRec;
     *pszMethodName = NULL;
+
+    LOCKREADIFFAILRET();
+
+    MethodRec *pMethodRec;
     IfFailRet(m_pStgdb->m_MiniMd.GetMethodRecord(RidFromToken(md), &pMethodRec));
     IfFailRet(m_pStgdb->m_MiniMd.getNameOfMethod(pMethodRec, pszMethodName));
     return S_OK;
@@ -1920,20 +1930,23 @@ MDInternalRW::GetNameAndSigOfMethodDef(
     LPCSTR          *pszMethodName)
 {
     HRESULT hr;
-    // we don't need lock here because name and signature will not change
 
     // Output parameter should not be NULL
     _ASSERTE(ppvSigBlob && pcbSigBlob);
     _ASSERTE(TypeFromToken(methoddef) == mdtMethodDef);
 
-    MethodRec *pMethodRec;
     *pszMethodName = NULL;
     *ppvSigBlob = NULL;
-    *ppvSigBlob = NULL;
+    *pcbSigBlob = 0;
+
+    LOCKREADIFFAILRET();
+
+    MethodRec *pMethodRec;
     IfFailRet(m_pStgdb->m_MiniMd.GetMethodRecord(RidFromToken(methoddef), &pMethodRec));
     IfFailRet(m_pStgdb->m_MiniMd.getSignatureOfMethod(pMethodRec, ppvSigBlob, pcbSigBlob));
+    IfFailRet(m_pStgdb->m_MiniMd.getNameOfMethod(pMethodRec, pszMethodName));
 
-    return GetNameOfMethodDef(methoddef, pszMethodName);
+    return S_OK;
 } // MDInternalRW::GetNameAndSigOfMethodDef
 
 
@@ -1946,11 +1959,12 @@ MDInternalRW::GetNameOfFieldDef(    // return hresult
     mdFieldDef fd,                  // given field
     LPCSTR    *pszFieldName)
 {
-    // we don't need lock here because name of field will not change
     HRESULT hr;
+    *pszFieldName = NULL;
+
+    LOCKREADIFFAILRET();
 
     FieldRec *pFieldRec;
-    *pszFieldName = NULL;
     IfFailRet(m_pStgdb->m_MiniMd.GetFieldRecord(RidFromToken(fd), &pFieldRec));
     IfFailRet(m_pStgdb->m_MiniMd.getNameOfField(pFieldRec, pszFieldName));
     return S_OK;
@@ -1973,7 +1987,7 @@ MDInternalRW::GetNameOfTypeRef(     // return TypeDef's name
     *psznamespace = NULL;
     *pszname = NULL;
 
-    // we don't need lock here because name of a typeref will not change
+    LOCKREADIFFAILRET();
 
     TypeRefRec *pTypeRefRec;
     IfFailRet(m_pStgdb->m_MiniMd.GetTypeRefRecord(RidFromToken(classref), &pTypeRefRec));
@@ -2196,6 +2210,8 @@ MDInternalRW::GetCountNestedClasses(  // return count of Nested classes.
 
     *pcNestedClassesCount = 0;
 
+    LOCKREADIFFAILRET();
+
     ulCount = m_pStgdb->m_MiniMd.getCountNestedClasss();
 
     for (ULONG i = 1; i <= ulCount; i++)
@@ -2229,6 +2245,8 @@ MDInternalRW::GetNestedClasses(   // Return actual count.
 
     *pcNestedClasses = 0;
 
+    LOCKREADIFFAILRET();
+
     ulCount = m_pStgdb->m_MiniMd.getCountNestedClasss();
 
     for (ULONG i = 1; i <= ulCount; i++)
@@ -2285,11 +2303,12 @@ MDInternalRW::GetSigOfMethodDef(
     _ASSERTE(TypeFromToken(methoddef) == mdtMethodDef);
 
     HRESULT hr;
-    // We don't change MethodDef signature. No need to lock.
-
-    MethodRec *pMethodRec;
     *ppSig = NULL;
     *pcbSigBlob = 0;
+
+    LOCKREADIFFAILRET();
+
+    MethodRec *pMethodRec;
     IfFailRet(m_pStgdb->m_MiniMd.GetMethodRecord(RidFromToken(methoddef), &pMethodRec));
     IfFailRet(m_pStgdb->m_MiniMd.getSignatureOfMethod(pMethodRec, ppSig, pcbSigBlob));
     return S_OK;
@@ -2310,11 +2329,12 @@ MDInternalRW::GetSigOfFieldDef(
     _ASSERTE(TypeFromToken(fielddef) == mdtFieldDef);
 
     HRESULT hr;
-    // We don't change Field's signature. No need to lock.
-
-    FieldRec *pFieldRec;
     *ppSig = NULL;
     *pcbSigBlob = 0;
+
+    LOCKREADIFFAILRET();
+
+    FieldRec *pFieldRec;
     IfFailRet(m_pStgdb->m_MiniMd.GetFieldRecord(RidFromToken(fielddef), &pFieldRec));
     IfFailRet(m_pStgdb->m_MiniMd.getSignatureOfField(pFieldRec, ppSig, pcbSigBlob));
     return S_OK;
@@ -2332,34 +2352,35 @@ MDInternalRW::GetSigFromToken(
     PCCOR_SIGNATURE * ppSig)
 {
     HRESULT hr;
-    // We don't change token's signature. Thus no need to lock.
 
     *ppSig = NULL;
     *pcbSig = 0;
     switch (TypeFromToken(tk))
     {
     case mdtSignature:
         {
+            LOCKREADIFFAILRET();
             StandAloneSigRec *pRec;
-            IfFailGo(m_pStgdb->m_MiniMd.GetStandAloneSigRecord(RidFromToken(tk), &pRec));
-            IfFailGo(m_pStgdb->m_MiniMd.getSignatureOfStandAloneSig(pRec, ppSig, pcbSig));
+            IfFailRet(m_pStgdb->m_MiniMd.GetStandAloneSigRecord(RidFromToken(tk), &pRec));
+            IfFailRet(m_pStgdb->m_MiniMd.getSignatureOfStandAloneSig(pRec, ppSig, pcbSig));
             return S_OK;
         }
     case mdtTypeSpec:
         {
+            LOCKREADIFFAILRET();
             TypeSpecRec *pRec;
-            IfFailGo(m_pStgdb->m_MiniMd.GetTypeSpecRecord(RidFromToken(tk), &pRec));
-            IfFailGo(m_pStgdb->m_MiniMd.getSignatureOfTypeSpec(pRec, ppSig, pcbSig));
+            IfFailRet(m_pStgdb->m_MiniMd.GetTypeSpecRecord(RidFromToken(tk), &pRec));
+            IfFailRet(m_pStgdb->m_MiniMd.getSignatureOfTypeSpec(pRec, ppSig, pcbSig));
             return S_OK;
         }
     case mdtMethodDef:
         {
-            IfFailGo(GetSigOfMethodDef(tk, pcbSig, ppSig));
+            IfFailRet(GetSigOfMethodDef(tk, pcbSig, ppSig));
             return S_OK;
         }
     case mdtFieldDef:
         {
-            IfFailGo(GetSigOfFieldDef(tk, pcbSig, ppSig));
+            IfFailRet(GetSigOfFieldDef(tk, pcbSig, ppSig));
             return S_OK;
         }
     }
@@ -2370,10 +2391,7 @@ MDInternalRW::GetSigFromToken(
             _ASSERTE(!"Unexpected token type");
 #endif
     *pcbSig = 0;
-    hr = META_E_INVALID_TOKEN_TYPE;
-
-ErrExit:
-    return hr;
+    return META_E_INVALID_TOKEN_TYPE;
 } // MDInternalRW::GetSigFromToken
 
 
@@ -2584,12 +2602,13 @@ MDInternalRW::GetTypeOfInterfaceImpl( // return hresult
     mdToken        *ptkType)
 {
     HRESULT hr;
-    // no need to lock this function.
 
     _ASSERTE(TypeFromToken(iiImpl) == mdtInterfaceImpl);
 
     *ptkType = mdTypeDefNil;
 
+    LOCKREADIFFAILRET();
+
     InterfaceImplRec *pIIRec;
     IfFailRet(m_pStgdb->m_MiniMd.GetInterfaceImplRecord(RidFromToken(iiImpl), &pIIRec));
     *ptkType = m_pStgdb->m_MiniMd.getInterfaceOfInterfaceImpl(pIIRec);
@@ -2611,6 +2630,15 @@ HRESULT MDInternalRW::GetMethodSpecProps(         // S_OK or error.
 
     _ASSERTE(TypeFromToken(mi) == mdtMethodSpec);
 
+    if (tkParent)
+        *tkParent = mdTokenNil;
+    if (ppvSigBlob)
+        *ppvSigBlob = NULL;
+    if (pcbSigBlob)
+        *pcbSigBlob = 0;
+
+    LOCKREADIFFAILRET();
+
     IfFailGo(m_pStgdb->m_MiniMd.GetMethodSpecRecord(RidFromToken(mi), &pMethodSpecRec));
 
     if (tkParent)
@@ -2670,24 +2698,26 @@ MDInternalRW::GetNameAndSigOfMemberRef( // meberref's name
 {
     HRESULT hr;
 
-    // MemberRef's name and sig won't change. Don't need to lock this.
-
     _ASSERTE(TypeFromToken(memberref) == mdtMemberRef);
 
-    MemberRefRec *pMemberRefRec;
     *pszMemberRefName = NULL;
     if (ppvSigBlob != NULL)
     {
         _ASSERTE(pcbSigBlob != NULL);
         *ppvSigBlob = NULL;
         *pcbSigBlob = 0;
     }
+
+    LOCKREADIFFAILRET();
+
+    MemberRefRec *pMemberRefRec;
     IfFailRet(m_pStgdb->m_MiniMd.GetMemberRefRecord(RidFromToken(memberref), &pMemberRefRec));
     if (ppvSigBlob != NULL)
     {
         IfFailRet(m_pStgdb->m_MiniMd.getSignatureOfMemberRef(pMemberRefRec, ppvSigBlob, pcbSigBlob));
     }
     IfFailRet(m_pStgdb->m_MiniMd.getNameOfMemberRef(pMemberRefRec, pszMemberRefName));
+
     return S_OK;
 } // MDInternalRW::GetNameAndSigOfMemberRef
 
@@ -3243,6 +3273,19 @@ HRESULT MDInternalRW::GetGenericParamProps(        // S_OK or error.
     HRESULT         hr = NOERROR;
     GenericParamRec  *pGenericParamRec = NULL;
 
+    if (pulSequence)
+        *pulSequence = 0;
+    if (pdwAttr)
+        *pdwAttr = 0;
+    if (ptOwner)
+        *ptOwner = mdTokenNil;
+    if (reserved)
+        *reserved = 0;
+    if (szName != NULL)
+        *szName = NULL;
+
+    LOCKREADIFFAILRET();
+
     // See if this version of the metadata can do Generics
     if (!m_pStgdb->m_MiniMd.SupportsGenerics())
         IfFailGo(CLDB_E_INCOMPATIBLE);
@@ -3282,6 +3325,13 @@ HRESULT MDInternalRW::GetGenericParamConstraintProps(      // S_OK or error.
     GenericParamConstraintRec  *pGPCRec;
     RID             ridRD = RidFromToken(rd);
 
+    if (ptGenericParam)
+        *ptGenericParam = mdGenericParamNil;
+    if (ptkConstraintType)
+        *ptkConstraintType = mdTokenNil;
+
+    LOCKREADIFFAILRET();
+
     // See if this version of the metadata can do Generics
     if (!m_pStgdb->m_MiniMd.SupportsGenerics())
         IfFailGo(CLDB_E_INCOMPATIBLE);
@@ -3775,9 +3825,14 @@ HRESULT MDInternalRW::GetTypeSpecFromToken(   // S_OK or error.
     _ASSERTE(TypeFromToken(typespec) == mdtTypeSpec);
     _ASSERTE(ppvSig && pcbSig);
 
+    *ppvSig = NULL;
+    *pcbSig = 0;
+
     if (!IsValidToken(typespec))
         return E_INVALIDARG;
 
+    LOCKREADIFFAILRET();
+
     TypeSpecRec *pRec;
     IfFailRet(m_pStgdb->m_MiniMd.GetTypeSpecRecord(RidFromToken(typespec), &pRec));
 
@@ -3969,6 +4024,9 @@ HRESULT MDInternalRW::EnumDeltaTokensInit(  // return hresult
     phEnum->m_EnumType = MDSimpleEnum;
 
     HENUMInternal::InitDynamicArrayEnum(phEnum);
+
+    LOCKREADIFFAILRET();
+
     for (index = 1; index <= m_pStgdb->m_MiniMd.m_Schema.m_cRecs[TBL_ENCLog]; ++index)
     {
         // Get the token type; see if it is a real token.