[release/8.0] PEImage should not permit m_path field mutation (#9…

…1085)

* Remove cases where PEImage::m_path was mutated. Create m_pathHash field and remove function. Remove FEATURE_CASE_SENSITIVE_FILESYSTEM.

* Address contract violations

* Remove try/catch and replace with assert

---------

Co-authored-by: Aaron R Robinson <arobins@microsoft.com>

src/coreclr/debug/daccess/request.cpp

@@ -2648,8 +2648,7 @@ ClrDataAccess::GetAssemblyLocation(CLRDATA_ADDRESS assembly, int count, _Inout_u
     // Turn from bytes to wide characters
     if (!pAssembly->GetPEAssembly()->GetPath().IsEmpty())
     {
-        if (!pAssembly->GetPEAssembly()->GetPath().
-            DacGetUnicode(count, location, pNeeded))
+        if (!pAssembly->GetPEAssembly()->GetPath().DacGetUnicode(count, location, pNeeded))
         {
             hr = E_FAIL;
         }

src/coreclr/inc/sstring.h

@@ -683,7 +683,9 @@ class EMPTY_BASES_DECL SString : private SBuffer
     BOOL IsASCIIScanned() const;
     void SetASCIIScanned() const;
     void SetNormalized() const;
+public:
     BOOL IsNormalized() const;
+private:
     void ClearNormalized() const;
 
     void EnsureWritable() const;

src/coreclr/md/compiler/mdutil.cpp

@@ -265,11 +265,7 @@ HRESULT LOADEDMODULES::FindCachedReadOnlyEntry(
             {
                 // If the name matches...
                 LPCWSTR pszName = pRegMeta->GetNameOfDBFile();
-    #ifdef FEATURE_CASE_SENSITIVE_FILESYSTEM
-                if (u16_strcmp(szName, pszName) == 0)
-    #else
                 if (SString::_wcsicmp(szName, pszName) == 0)
-    #endif
                 {
                     ULONG cRefs;
 
@@ -299,11 +295,7 @@ HRESULT LOADEDMODULES::FindCachedReadOnlyEntry(
             {
                 // If the name matches...
                 LPCWSTR pszName = pRegMeta->GetNameOfDBFile();
-    #ifdef FEATURE_CASE_SENSITIVE_FILESYSTEM
-                if (u16_strcmp(szName, pszName) == 0)
-    #else
                 if (SString::_wcsicmp(szName, pszName) == 0)
-    #endif
                 {
                     ULONG cRefs;
 

src/coreclr/vm/dwbucketmanager.hpp

@@ -960,11 +960,9 @@ bool BaseBucketParamsManager::GetFileVersionInfoForModule(Module* pModule, USHOR
         // if we failed to get the version info from the native image then fall back to the IL image.
         if (!succeeded)
         {
-            LPCWSTR modulePath = pPEAssembly->GetPath().GetUnicode();
-            if (modulePath != NULL && modulePath != SString::Empty() && SUCCEEDED(DwGetFileVersionInfo(modulePath, major, minor, build, revision)))
-            {
-                succeeded = true;
-            }
+            const SString& modulePath = pPEAssembly->GetPath();
+            _ASSERTE(modulePath.IsNormalized());
+            succeeded = !modulePath.IsEmpty() && SUCCEEDED(DwGetFileVersionInfo(modulePath.GetUnicode(), major, minor, build, revision));
         }
     }
 

src/coreclr/vm/excep.cpp

@@ -11616,7 +11616,8 @@ VOID GetAssemblyDetailInfo(SString    &sType,
 
     SString sAlcName;
     pPEAssembly->GetAssemblyBinder()->GetNameForDiagnostics(sAlcName);
-    if (pPEAssembly->GetPath().IsEmpty())
+    SString assemblyPath{ pPEAssembly->GetPath() };
+    if (assemblyPath.IsEmpty())
     {
         detailsUtf8.Printf("Type %s originates from '%s' in the context '%s' in a byte array",
                                    sType.GetUTF8(),
@@ -11629,7 +11630,7 @@ VOID GetAssemblyDetailInfo(SString    &sType,
                                    sType.GetUTF8(),
                                    sAssemblyDisplayName.GetUTF8(),
                                    sAlcName.GetUTF8(),
-                                   pPEAssembly->GetPath().GetUTF8());
+                                   assemblyPath.GetUTF8());
     }
 
     sAssemblyDetailInfo.Append(detailsUtf8.GetUnicode());

src/coreclr/vm/peimage.cpp

@@ -141,11 +141,11 @@ ULONG PEImage::Release()
         result=InterlockedDecrement(&m_refCount);
         if (result == 0 )
         {
-            LOG((LF_LOADER, LL_INFO100, "PEImage: Closing Image %s\n", m_path.GetUTF8()));
+            LOG((LF_LOADER, LL_INFO100, "PEImage: Closing %p\n", this));
             if(m_bInHashMap)
             {
                 PEImageLocator locator(this);
-                PEImage* deleted = (PEImage *)s_Images->DeleteValue(GetPathHash(), &locator);
+                PEImage* deleted = (PEImage *)s_Images->DeleteValue(m_pathHash, &locator);
                 _ASSERTE(deleted == this);
             }
         }
@@ -249,12 +249,7 @@ BOOL PEImage::CompareImage(UPTR u1, UPTR u2)
     EX_TRY
     {
         SString path(SString::Literal, pLocator->m_pPath);
-
-#ifdef FEATURE_CASE_SENSITIVE_FILESYSTEM
-        if (pImage->GetPath().Equals(path))
-#else
         if (pImage->GetPath().EqualsCaseInsensitive(path))
-#endif
         {
             ret = TRUE;
         }
@@ -623,6 +618,7 @@ void PEImage::EnumMemoryRegions(CLRDataEnumMemoryFlags flags)
 
 PEImage::PEImage():
     m_path(),
+    m_pathHash(0),
     m_refCount(1),
     m_bInHashMap(FALSE),
     m_bundleFileLocation(),

src/coreclr/vm/peimage.h

@@ -132,8 +132,6 @@ class PEImage final
     PTR_PEImageLayout GetLoadedLayout();
     PTR_PEImageLayout GetFlatLayout();
 
-    BOOL  HasPath();
-    ULONG GetPathHash();
     const SString& GetPath();
     const SString& GetPathToLoad();
     LPCWSTR GetPathForErrorMessages() { return GetPath(); }
@@ -288,6 +286,7 @@ class PEImage final
     // ------------------------------------------------------------
 
     SString   m_path;
+    ULONG     m_pathHash;
     LONG      m_refCount;
 
     // means this is a unique (deduped) instance.

src/coreclr/vm/peimage.inl

@@ -288,6 +288,7 @@ inline void  PEImage::Init(LPCWSTR pPath, BundleFileLocation bundleFileLocation)
 
     m_path = pPath;
     m_path.Normalize();
+    m_pathHash = m_path.HashCaseInsensitive();
     m_bundleFileLocation = bundleFileLocation;
     SetModuleFileNameHintForDAC();
 }
@@ -310,11 +311,7 @@ inline PTR_PEImage PEImage::FindByPath(LPCWSTR pPath, BOOL isInBundle /* = TRUE
     int CaseHashHelper(const WCHAR *buffer, COUNT_T count);
 
     PEImageLocator locator(pPath, isInBundle);
-#ifdef FEATURE_CASE_SENSITIVE_FILESYSTEM
-    DWORD dwHash=path.Hash();
-#else
     DWORD dwHash = CaseHashHelper(pPath, (COUNT_T) u16_strlen(pPath));
-#endif
     return (PEImage *) s_Images->LookupValue(dwHash, &locator);
 }
 
@@ -366,7 +363,7 @@ inline void PEImage::AddToHashMap()
     CONTRACTL_END;
 
     _ASSERTE(s_hashLock.OwnedByCurrentThread());
-    s_Images->InsertValue(GetPathHash(),this);
+    s_Images->InsertValue(m_pathHash,this);
     m_bInHashMap=TRUE;
 }
 
@@ -378,31 +375,6 @@ inline BOOL PEImage::Has32BitNTHeaders()
     return GetOrCreateLayout(PEImageLayout::LAYOUT_ANY)->Has32BitNTHeaders();
 }
 
-inline BOOL PEImage::HasPath()
-{
-    LIMITED_METHOD_CONTRACT;
-
-    return !GetPath().IsEmpty();
-}
-
-inline ULONG PEImage::GetPathHash()
-{
-    CONTRACT(ULONG)
-    {
-        PRECONDITION(HasPath());
-        MODE_ANY;
-        GC_NOTRIGGER;
-        THROWS;
-    }
-    CONTRACT_END;
-
-#ifdef FEATURE_CASE_SENSITIVE_FILESYSTEM
-    RETURN m_path.Hash();
-#else
-    RETURN m_path.HashCaseInsensitive();
-#endif
-}
-
 inline void  PEImage::GetPEKindAndMachine(DWORD* pdwKind, DWORD* pdwMachine)
 {
     CONTRACTL

src/coreclr/vm/peimagelayout.cpp

@@ -534,7 +534,10 @@ LoadedImageLayout::LoadedImageLayout(PEImage* pOwner, HRESULT* loadFailure)
 
     IfFailThrow(Init(m_Module, true));
 
-    LOG((LF_LOADER, LL_INFO1000, "PEImage: Opened HMODULE %s\n", pOwner->GetPath().GetUTF8()));
+#ifdef LOGGING
+    SString ownerPath{ pOwner->GetPath() };
+    LOG((LF_LOADER, LL_INFO1000, "PEImage: Opened HMODULE %s\n", ownerPath.GetUTF8()));
+#endif // LOGGING
 
 #else
     HANDLE hFile = pOwner->GetFileHandle();
@@ -548,8 +551,11 @@ LoadedImageLayout::LoadedImageLayout(PEImage* pOwner, HRESULT* loadFailure)
         return;
     }
 
+#ifdef LOGGING
+    SString ownerPath{ pOwner->GetPath() };
     LOG((LF_LOADER, LL_INFO1000, "PEImage: image %s (hFile %p) mapped @ %p\n",
-        pOwner->GetPath().GetUTF8(), hFile, (void*)m_LoadedFile));
+        ownerPath.GetUTF8(), hFile, (void*)m_LoadedFile));
+#endif // LOGGING
 
     IfFailThrow(Init((void*)m_LoadedFile));
 
@@ -616,7 +622,10 @@ FlatImageLayout::FlatImageLayout(PEImage* pOwner)
     INT64 offset = pOwner->GetOffset();
     INT64 size = pOwner->GetSize();
 
-    LOG((LF_LOADER, LL_INFO100, "PEImage: Opening flat %s\n", pOwner->GetPath().GetUTF8()));
+#ifdef LOGGING
+    SString ownerPath{ pOwner->GetPath() };
+    LOG((LF_LOADER, LL_INFO100, "PEImage: Opening flat %s\n", ownerPath.GetUTF8()));
+#endif // LOGGING
 
     // If a size is not specified, load the whole file
     if (size == 0)

src/coreclr/vm/readytoruninfo.cpp

@@ -429,7 +429,8 @@ static void LogR2r(const char *msg, PEAssembly *pPEAssembly)
     if (r2rLogFile == NULL)
         return;
 
-    fprintf(r2rLogFile, "%s: \"%s\".\n", msg, pPEAssembly->GetPath().GetUTF8());
+    SString assemblyPath{ pPEAssembly->GetPath() };
+    fprintf(r2rLogFile, "%s: \"%s\".\n", msg, assemblyPath.GetUTF8());
     fflush(r2rLogFile);
 }
 
@@ -1904,7 +1905,7 @@ uint32_t ReadyToRun_TypeGenericInfoMap::GetGenericArgumentCount(mdTypeDef input,
     uint32_t count = ((uint8_t)typeGenericInfo & (uint8_t)ReadyToRunTypeGenericInfo::GenericCountMask);
     if (count > 2)
         foundResult = false;
-    
+
     if (!foundResult)
     {
         HENUMInternalHolder hEnumTyPars(pImport);
@@ -1922,7 +1923,7 @@ HRESULT ReadyToRun_TypeGenericInfoMap::GetGenericArgumentCountNoThrow(mdTypeDef
     uint32_t count = ((uint8_t)typeGenericInfo & (uint8_t)ReadyToRunTypeGenericInfo::GenericCountMask);
     if (count > 2)
         foundResult = false;
-    
+
     if (!foundResult)
     {
         HENUMInternalHolder hEnumTyPars(pImport);