Return actual LoadLibrary failures for IJW scenarios. (dotnet#45997)

* Return actual LoadLibrary failures for IJW scenarios.
nattress · Jan 5, 2021 · 0622fa0 · 0622fa0
1 parent 7ed93fa
commit 0622fa0
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 22 deletions.
diff --git a/src/coreclr/vm/peimage.cpp b/src/coreclr/vm/peimage.cpp
@@ -1026,11 +1026,13 @@ PTR_PEImageLayout PEImage::CreateLayoutMapped()
 
     PEImageLayout * pLoadLayout = NULL;
 
+    HRESULT loadFailure = S_OK;
     if (m_bIsTrustedNativeImage || IsFile())
     {
-        // For CoreCLR, try to load all files via LoadLibrary first. If LoadLibrary did not work, retry using
-        // regular mapping - but not for native images.
-        pLoadLayout = PEImageLayout::Load(this, FALSE /* bNTSafeLoad */, m_bIsTrustedNativeImage /* bThrowOnError */);
+        // Try to load all files via LoadLibrary first. If LoadLibrary did not work,
+        // retry using regular mapping.
+        HRESULT* returnDontThrow = m_bIsTrustedNativeImage ? NULL : &loadFailure;
+        pLoadLayout = PEImageLayout::Load(this, FALSE /* bNTSafeLoad */, returnDontThrow);
     }
 
     if (pLoadLayout != NULL)
@@ -1045,21 +1047,25 @@ PTR_PEImageLayout PEImage::CreateLayoutMapped()
         PEImageLayoutHolder pLayout(PEImageLayout::Map(this));
 
         bool fMarkAnyCpuImageAsLoaded = false;
+
         // Avoid mapping another image if we can. We can only do this for IL-ONLY images
-        // since LoadLibrary is needed if we are to actually load code
+        // since LoadLibrary is needed if we are to actually load code (e.g. IJW).
         if (pLayout->HasCorHeader())
         {
-            if (pLayout->IsILOnly())
+            // IJW images must be successfully loaded by the OS to handle
+            // native dependencies, therefore they cannot be mapped.
+            if (!pLayout->IsILOnly())
             {
-                // For CoreCLR, IL only images will always be mapped. We also dont bother doing the conversion of PE header on 64bit,
-                // as done below for the desktop case, as there is no appcompat burden for CoreCLR on 64bit to have that conversion done.
-                fMarkAnyCpuImageAsLoaded = true;
-            }
-            else
-            {
-                // IJW images must be loaded, not mapped
-                ThrowHR(COR_E_BADIMAGEFORMAT);
+                // For compat with older CoreCLR versions we will fallback to the
+                // COR_E_BADIMAGEFORMAT error code if a failure wasn't indicated.
+                loadFailure = FAILED(loadFailure) ? loadFailure : COR_E_BADIMAGEFORMAT;
+                EEFileLoadException::Throw(GetPath(), loadFailure);
             }
+
+            // IL only images will always be mapped. We don't bother doing a conversion
+            // of PE header on 64bit, as done for .NET Framework, since there is no
+            // appcompat burden for CoreCLR on 64bit.
+            fMarkAnyCpuImageAsLoaded = true;
         }
 
         pLayout.SuppressRelease();

diff --git a/src/coreclr/vm/peimagelayout.cpp b/src/coreclr/vm/peimagelayout.cpp
@@ -50,7 +50,7 @@ PEImageLayout* PEImageLayout::LoadConverted(PEImage* pOwner, BOOL isInBundle)
     return new ConvertedImageLayout(pFlat, isInBundle);
 }
 
-PEImageLayout* PEImageLayout::Load(PEImage* pOwner, BOOL bNTSafeLoad, BOOL bThrowOnError)
+PEImageLayout* PEImageLayout::Load(PEImage* pOwner, BOOL bNTSafeLoad, HRESULT* returnDontThrow)
 {
     STANDARD_VM_CONTRACT;
 
@@ -62,7 +62,7 @@ PEImageLayout* PEImageLayout::Load(PEImage* pOwner, BOOL bNTSafeLoad, BOOL bThro
         return PEImageLayout::LoadConverted(pOwner, true);
     }
 
-    PEImageLayoutHolder pAlloc(new LoadedImageLayout(pOwner,bNTSafeLoad,bThrowOnError));
+    PEImageLayoutHolder pAlloc(new LoadedImageLayout(pOwner,bNTSafeLoad,returnDontThrow));
     if (pAlloc->GetBase()==NULL)
         return NULL;
     return pAlloc.Extract();
@@ -644,7 +644,7 @@ MappedImageLayout::MappedImageLayout(PEImage* pOwner)
 }
 
 #if !defined(CROSSGEN_COMPILE) && !defined(TARGET_UNIX)
-LoadedImageLayout::LoadedImageLayout(PEImage* pOwner, BOOL bNTSafeLoad, BOOL bThrowOnError)
+LoadedImageLayout::LoadedImageLayout(PEImage* pOwner, BOOL bNTSafeLoad, HRESULT* returnDontThrow)
 {
     CONTRACTL
     {
@@ -664,12 +664,15 @@ LoadedImageLayout::LoadedImageLayout(PEImage* pOwner, BOOL bNTSafeLoad, BOOL bTh
     m_Module = CLRLoadLibraryEx(pOwner->GetPath(), NULL, dwFlags);
     if (m_Module == NULL)
     {
-        if (!bThrowOnError)
-            return;
-
         // Fetch the HRESULT upfront before anybody gets a chance to corrupt it
         HRESULT hr = HRESULT_FROM_GetLastError();
-        EEFileLoadException::Throw(pOwner->GetPath(), hr, NULL);
+        if (returnDontThrow != NULL)
+        {
+            *returnDontThrow = hr;
+            return;
+        }
+
+        EEFileLoadException::Throw(pOwner->GetPath(), hr);
     }
     IfFailThrow(Init(m_Module,true));
 

diff --git a/src/coreclr/vm/peimagelayout.h b/src/coreclr/vm/peimagelayout.h
@@ -51,7 +51,7 @@ class PEImageLayout : public PEDecoder
     static PEImageLayout* CreateFlat(const void *flat, COUNT_T size,PEImage* pOwner);
     static PEImageLayout* CreateFromHMODULE(HMODULE mappedbase,PEImage* pOwner, BOOL bTakeOwnership);
     static PEImageLayout* LoadFromFlat(PEImageLayout* pflatimage);
-    static PEImageLayout* Load(PEImage* pOwner, BOOL bNTSafeLoad, BOOL bThrowOnError = TRUE);
+    static PEImageLayout* Load(PEImage* pOwner, BOOL bNTSafeLoad, HRESULT* returnDontThrow = NULL);
     static PEImageLayout* LoadFlat(PEImage* pOwner);
     static PEImageLayout* LoadConverted(PEImage* pOwner, BOOL isInBundle = FALSE);
     static PEImageLayout* LoadNative(LPCWSTR fullPath);
@@ -142,7 +142,7 @@ class LoadedImageLayout: public PEImageLayout
     HINSTANCE m_Module;
 public:
 #ifndef DACCESS_COMPILE
-    LoadedImageLayout(PEImage* pOwner, BOOL bNTSafeLoad, BOOL bThrowOnError);
+    LoadedImageLayout(PEImage* pOwner, BOOL bNTSafeLoad, HRESULT* returnDontThrow);
     ~LoadedImageLayout()
     {
         CONTRACTL