Reapply "Refactor external_assembly_probe to be separate from single-file bundle probing" (#113776)

Undo the revert in #113738

Fix on top of the original: Make a copy of the host contract information on initialization instead of just operating on a pointer to the struct. This should address intermittent failures on shutdown where a thread is still running after coreclr_shutdown has completed and the actual contract struct provided by a host may be cleaned up. The assumption is that any function pointers provided on the host contract at initialization should remain valid for the process lifetime.

Author: elinor-fung

src/coreclr/binder/assemblybindercommon.cpp

@@ -37,7 +37,7 @@ extern HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadCon
 
 STDAPI BinderAcquirePEImage(LPCTSTR            szAssemblyPath,
     PEImage** ppPEImage,
-    BundleFileLocation bundleFileLocation);
+    ProbeExtensionResult probeExtensionResult);
 
 namespace BINDER_SPACE
 {
@@ -271,8 +271,8 @@ namespace BINDER_SPACE
         StackSString sCoreLibName(CoreLibName_IL_W);
         StackSString sCoreLib;
         BinderTracing::PathSource pathSource = BinderTracing::PathSource::Bundle;
-        BundleFileLocation bundleFileLocation = Bundle::ProbeAppBundle(sCoreLibName, /*pathIsBundleRelative */ true);
-        if (!bundleFileLocation.IsValid())
+        ProbeExtensionResult probeExtensionResult = AssemblyProbeExtension::Probe(sCoreLibName, /*pathIsBundleRelative */ true);
+        if (!probeExtensionResult.IsValid())
         {
             pathSource = BinderTracing::PathSource::ApplicationAssemblies;
         }
@@ -282,7 +282,7 @@ namespace BINDER_SPACE
         hr = AssemblyBinderCommon::GetAssembly(sCoreLib,
                                          TRUE /* fIsInTPA */,
                                          &pSystemAssembly,
-                                         bundleFileLocation);
+                                         probeExtensionResult);
 
         BinderTracing::PathProbed(sCoreLib, pathSource, hr);
 
@@ -322,7 +322,7 @@ namespace BINDER_SPACE
             hr = AssemblyBinderCommon::GetAssembly(sCoreLib,
                 TRUE /* fIsInTPA */,
                 &pSystemAssembly,
-                bundleFileLocation);
+                probeExtensionResult);
 
             BinderTracing::PathProbed(sCoreLib, BinderTracing::PathSource::ApplicationAssemblies, hr);
         }
@@ -367,8 +367,8 @@ namespace BINDER_SPACE
         StackSString sCoreLibSatellite;
 
         BinderTracing::PathSource pathSource = BinderTracing::PathSource::Bundle;
-        BundleFileLocation bundleFileLocation = Bundle::ProbeAppBundle(relativePath, /*pathIsBundleRelative */ true);
-        if (!bundleFileLocation.IsValid())
+        ProbeExtensionResult probeExtensionResult = AssemblyProbeExtension::Probe(relativePath, /*pathIsBundleRelative */ true);
+        if (!probeExtensionResult.IsValid())
         {
             sCoreLibSatellite.Set(systemDirectory);
             pathSource = BinderTracing::PathSource::ApplicationAssemblies;
@@ -379,7 +379,7 @@ namespace BINDER_SPACE
         IF_FAIL_GO(AssemblyBinderCommon::GetAssembly(sCoreLibSatellite,
                                                TRUE /* fIsInTPA */,
                                                &pSystemAssembly,
-                                               bundleFileLocation));
+                                               probeExtensionResult));
         BinderTracing::PathProbed(sCoreLibSatellite, pathSource, hr);
 
         *ppSystemAssembly = pSystemAssembly.Extract();
@@ -590,15 +590,15 @@ namespace BINDER_SPACE
 
     namespace
     {
-        HRESULT BindSatelliteResourceFromBundle(
+        HRESULT BindSatelliteResourceByProbeExtension(
             AssemblyName*          pRequestedAssemblyName,
             SString               &relativePath,
             BindResult*            pBindResult)
         {
             HRESULT hr = S_OK;
 
-            BundleFileLocation bundleFileLocation = Bundle::ProbeAppBundle(relativePath, /* pathIsBundleRelative */ true);
-            if (!bundleFileLocation.IsValid())
+            ProbeExtensionResult probeExtensionResult = AssemblyProbeExtension::Probe(relativePath, /* pathIsBundleRelative */ true);
+            if (!probeExtensionResult.IsValid())
             {
                 return hr;
             }
@@ -607,7 +607,7 @@ namespace BINDER_SPACE
             hr = AssemblyBinderCommon::GetAssembly(relativePath,
                                                    FALSE /* fIsInTPA */,
                                                    &pAssembly,
-                                                   bundleFileLocation);
+                                                   probeExtensionResult);
 
             BinderTracing::PathProbed(relativePath, BinderTracing::PathSource::Bundle, hr);
 
@@ -692,7 +692,7 @@ namespace BINDER_SPACE
             BindResult* pBindResult)
         {
             // Satellite resource probing strategy is to look:
-            //   * First within the single-file bundle
+            //   * First via probe extensions (single-file bundle, external data)
             //   * Then under each of the Platform Resource Roots
             //   * Then under each of the App Paths.
             //
@@ -712,7 +712,7 @@ namespace BINDER_SPACE
             CombinePath(fileName, simpleNameRef, fileName);
             fileName.Append(W(".dll"));
 
-            hr = BindSatelliteResourceFromBundle(pRequestedAssemblyName, fileName, pBindResult);
+            hr = BindSatelliteResourceByProbeExtension(pRequestedAssemblyName, fileName, pBindResult);
 
             if (pBindResult->HaveResult() || FAILED(hr))
             {
@@ -841,12 +841,9 @@ namespace BINDER_SPACE
             ReleaseHolder<Assembly> pTPAAssembly;
             const SString& simpleName = pRequestedAssemblyName->GetSimpleName();
 
-            // Is assembly in the bundle?
-            // Single-file bundle contents take precedence over TPA.
-            // The list of bundled assemblies is contained in the bundle manifest, and NOT in the TPA.
-            // Therefore the bundle is first probed using the assembly's simple name.
-            // If found, the assembly is loaded from the bundle.
-            if (Bundle::AppIsBundle())
+            // Probing extensions (single-file, external probe) take precedence over TPA.
+            // For single-file, bundled assemblies should only be in the bundle manifest, not in the TPA.
+            if (AssemblyProbeExtension::IsEnabled())
             {
                 // Search Assembly.ni.dll, then Assembly.dll
                 // The Assembly.ni.dll paths are rare, and intended for supporting managed C++ R2R assemblies.
@@ -858,16 +855,16 @@ namespace BINDER_SPACE
                     SString assemblyFileName(simpleName);
                     assemblyFileName.Append(candidates[i]);
 
-                    SString assemblyFilePath(Bundle::AppBundle->BasePath());
-                    assemblyFilePath.Append(assemblyFileName);
-
-                    BundleFileLocation bundleFileLocation = Bundle::ProbeAppBundle(assemblyFileName, /* pathIsBundleRelative */ true);
-                    if (bundleFileLocation.IsValid())
+                    ProbeExtensionResult probeExtensionResult = AssemblyProbeExtension::Probe(assemblyFileName, /* pathIsBundleRelative */ true);
+                    if (probeExtensionResult.IsValid())
                     {
+                        SString assemblyFilePath(Bundle::AppIsBundle() ? Bundle::AppBundle->BasePath() : SString::Empty());
+                        assemblyFilePath.Append(assemblyFileName);
+
                         hr = GetAssembly(assemblyFilePath,
                                          TRUE,  // fIsInTPA
                                          &pTPAAssembly,
-                                         bundleFileLocation);
+                                         probeExtensionResult);
 
                         BinderTracing::PathProbed(assemblyFilePath, BinderTracing::PathSource::Bundle, hr);
 
@@ -996,7 +993,7 @@ namespace BINDER_SPACE
     HRESULT AssemblyBinderCommon::GetAssembly(SString            &assemblyPath,
                                               BOOL               fIsInTPA,
                                               Assembly           **ppAssembly,
-                                              BundleFileLocation bundleFileLocation)
+                                              ProbeExtensionResult probeExtensionResult)
     {
         HRESULT hr = S_OK;
 
@@ -1012,7 +1009,7 @@ namespace BINDER_SPACE
         {
             LPCTSTR szAssemblyPath = const_cast<LPCTSTR>(assemblyPath.GetUnicode());
 
-            hr = BinderAcquirePEImage(szAssemblyPath, &pPEImage, bundleFileLocation);
+            hr = BinderAcquirePEImage(szAssemblyPath, &pPEImage, probeExtensionResult);
             IF_FAIL_GO(hr);
         }
 

src/coreclr/binder/inc/assembly.hpp

@@ -25,7 +25,6 @@
 #include "customassemblybinder.h"
 #endif // !defined(DACCESS_COMPILE)
 
-#include "bundle.h"
 #include <assemblybinderutil.h>
 
 namespace BINDER_SPACE

src/coreclr/binder/inc/assemblybindercommon.hpp

@@ -16,7 +16,7 @@
 
 #include "bindertypes.hpp"
 #include "bindresult.hpp"
-#include "bundle.h"
+#include <assemblyprobeextension.h>
 
 class AssemblyBinder;
 class DefaultAssemblyBinder;
@@ -28,7 +28,7 @@ namespace BINDER_SPACE
     class AssemblyBinderCommon
     {
     public:
-        static HRESULT BindAssembly(/* in */  AssemblyBinder      *pBinder, 
+        static HRESULT BindAssembly(/* in */  AssemblyBinder      *pBinder,
                                     /* in */  AssemblyName        *pAssemblyName,
                                     /* in */  bool                 excludeAppPaths,
                                     /* out */ Assembly           **ppAssembly);
@@ -44,7 +44,7 @@ namespace BINDER_SPACE
         static HRESULT GetAssembly(/* in */  SString     &assemblyPath,
                                    /* in */  BOOL         fIsInTPA,
                                    /* out */ Assembly   **ppAssembly,
-                                   /* in */  BundleFileLocation bundleFileLocation = BundleFileLocation::Invalid());
+                                   /* in */  ProbeExtensionResult probeExtensionResult = ProbeExtensionResult::Invalid());
 
 #if !defined(DACCESS_COMPILE)
         static HRESULT BindUsingHostAssemblyResolver (/* in */ INT_PTR pManagedAssemblyLoadContextToBindWithin,

src/coreclr/dlls/mscoree/exports.cpp

@@ -288,12 +288,9 @@ int coreclr_initialize(
     hr = CorHost2::CreateObject(IID_ICLRRuntimeHost4, (void**)&host);
     IfFailRet(hr);
 
-    ConstWStringHolder appDomainFriendlyNameW = StringToUnicode(appDomainFriendlyName);
-
-    ExternalAssemblyProbeFn* externalAssemblyProbe = hostContract != nullptr ? hostContract->external_assembly_probe : nullptr;
-    if (bundleProbe != nullptr || externalAssemblyProbe != nullptr)
+    if (bundleProbe != nullptr)
     {
-        static Bundle bundle(exePath, bundleProbe, externalAssemblyProbe);
+        static Bundle bundle(exePath, bundleProbe);
         Bundle::AppBundle = &bundle;
     }
 
@@ -309,6 +306,7 @@ int coreclr_initialize(
     hr = host->Start();
     IfFailRet(hr);
 
+    ConstWStringHolder appDomainFriendlyNameW = StringToUnicode(appDomainFriendlyName);
     hr = host->CreateAppDomainWithManager(
         appDomainFriendlyNameW,
         0,

src/coreclr/inc/assemblyprobeextension.h

@@ -0,0 +1,68 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+#ifndef HAVE_ASSEMBLY_PROBE_EXTENSIONS_H
+#define HAVE_ASSEMBLY_PROBE_EXTENSIONS_H
+
+#include <sstring.h>
+#include "bundle.h"
+
+class ProbeExtensionResult
+{
+public:
+    enum class Type
+    {
+        Invalid,
+        Bundle,
+        External,
+    };
+
+    Type Type;
+    union
+    {
+        BundleFileLocation BundleLocation;
+        struct
+        {
+            void* Data;
+            int64_t Size;
+        } ExternalData;
+    };
+
+    ProbeExtensionResult()
+        : Type{Type::Invalid}
+    { }
+
+    static ProbeExtensionResult Bundle(BundleFileLocation location)
+    {
+        return ProbeExtensionResult(location);
+    }
+
+    static ProbeExtensionResult External(void* data, int64_t size)
+    {
+        return ProbeExtensionResult(data, size);
+    }
+
+    static ProbeExtensionResult Invalid() { LIMITED_METHOD_CONTRACT; return ProbeExtensionResult(); }
+
+    bool IsValid() const { return Type != Type::Invalid; }
+
+private:
+    ProbeExtensionResult(BundleFileLocation location)
+        : Type{Type::Bundle}
+        , BundleLocation{location}
+    { }
+
+    ProbeExtensionResult(void* data, int64_t size)
+        : Type{Type::External}
+        , ExternalData{data, size}
+    { }
+};
+
+class AssemblyProbeExtension
+{
+public:
+    static bool IsEnabled();
+    static ProbeExtensionResult Probe(const SString& path, bool pathIsBundleRelative = false);
+};
+
+#endif // HAVE_ASSEMBLY_PROBE_EXTENSIONS_H

src/coreclr/inc/bundle.h

@@ -18,31 +18,29 @@ class Bundle;
 struct BundleFileLocation
 {
     INT64 Size;
-    void* DataStart;
     INT64 Offset;
-    INT64 UncompresedSize;
+    INT64 UncompressedSize;
 
     BundleFileLocation()
     {
         LIMITED_METHOD_CONTRACT;
 
         Size = 0;
-        DataStart = nullptr;
         Offset = 0;
-        UncompresedSize = 0;
+        UncompressedSize = 0;
     }
 
     static BundleFileLocation Invalid() { LIMITED_METHOD_CONTRACT; return BundleFileLocation(); }
 
     const SString &Path() const;
 
-    bool IsValid() const { LIMITED_METHOD_CONTRACT; return DataStart != nullptr || Offset != 0; }
+    bool IsValid() const { LIMITED_METHOD_CONTRACT; return Offset != 0; }
 };
 
 class Bundle
 {
 public:
-    Bundle(LPCSTR bundlePath, BundleProbeFn *probe, ExternalAssemblyProbeFn* externalAssemblyProbe = nullptr);
+    Bundle(LPCSTR bundlePath, BundleProbeFn *probe);
     BundleFileLocation Probe(const SString& path, bool pathIsBundleRelative = false) const;
 
     const SString &Path() const { LIMITED_METHOD_CONTRACT; return m_path; }
@@ -53,9 +51,8 @@ class Bundle
     static BundleFileLocation ProbeAppBundle(const SString& path, bool pathIsBundleRelative = false);
 
 private:
-    SString m_path; // The path to single-file executable or package name/id on Android
+    SString m_path; // The path to single-file executable
     BundleProbeFn *m_probe;
-    ExternalAssemblyProbeFn *m_externalAssemblyProbe;
 
     SString m_basePath; // The prefix to denote a path within the bundle
     COUNT_T m_basePathLength;

src/coreclr/inc/hostinformation.h

@@ -11,6 +11,9 @@ class HostInformation
 public:
     static void SetContract(_In_ host_runtime_contract* hostContract);
     static bool GetProperty(_In_z_ const char* name, SString& value);
+
+    static bool HasExternalProbe();
+    static bool ExternalAssemblyProbe(_In_ const SString& path, _Out_ void** data, _Out_ int64_t* size);
 };
 
 #endif // _HOSTINFORMATION_H_

src/coreclr/vm/CMakeLists.txt

@@ -293,6 +293,7 @@ set(VM_SOURCES_WKS
     ${VM_SOURCES_DAC_AND_WKS_COMMON}
     appdomainnative.cpp
     assemblynative.cpp
+    assemblyprobeextension.cpp
     assemblyspec.cpp
     baseassemblyspec.cpp
     ${RUNTIME_DIR}/CachedInterfaceDispatch.cpp
@@ -390,6 +391,7 @@ set(VM_HEADERS_WKS
     ../inc/jithelpers.h
     appdomainnative.hpp
     assemblynative.hpp
+    ../inc/assemblyprobeextension.h
     assemblyspec.hpp
     assemblyspecbase.h
     baseassemblyspec.h

src/coreclr/vm/assemblynative.cpp

@@ -191,9 +191,7 @@ extern "C" void QCALLTYPE AssemblyNative_LoadFromPath(INT_PTR ptrNativeAssemblyB
 
     if (pwzILPath != NULL)
     {
-        pILImage = PEImage::OpenImage(pwzILPath,
-                                      MDInternalImport_Default,
-                                      BundleFileLocation::Invalid());
+        pILImage = PEImage::OpenImage(pwzILPath);
 
         // Need to verify that this is a valid CLR assembly.
         if (!pILImage->CheckILFormat())