diff --git a/src/Framework/NativeMethods.txt b/src/Framework/NativeMethods.txt index 3508948fd36..b16268823b8 100644 --- a/src/Framework/NativeMethods.txt +++ b/src/Framework/NativeMethods.txt @@ -51,6 +51,7 @@ GetLogicalDrives GetLogicalProcessorInformationEx GetLongPathName GetModuleFileName +GetModuleHandle GetNativeSystemInfo GetOEMCP GetProcAddress diff --git a/src/Framework/Windows/Win32/System/Com/ComClassFactory.cs b/src/Framework/Windows/Win32/System/Com/ComClassFactory.cs index 277c4d0bf43..35f67338cc5 100644 --- a/src/Framework/Windows/Win32/System/Com/ComClassFactory.cs +++ b/src/Framework/Windows/Win32/System/Com/ComClassFactory.cs @@ -6,6 +6,7 @@ using System; using System.Diagnostics.CodeAnalysis; +using System.Runtime.InteropServices; using System.Runtime.Versioning; using Windows.Win32.Foundation; @@ -19,6 +20,36 @@ namespace Windows.Win32.System.Com; [SupportedOSPlatform("windows6.1")] internal sealed unsafe class ComClassFactory : IDisposable { + private const string DllGetClassObjectExportName = "DllGetClassObject"; + + /// + /// Name of the non-standard class-object entry point exported by the .NET + /// Framework CLR (clr.dll). Pass to + /// + /// to activate CLR-hosted CLSIDs without triggering the mscoree.dll shim. + /// + /// + /// + /// CLSID_CorMetaDataDispenser and the other CLR-hosted legacy COM CLSIDs are registered + /// with mscoree.dll as their InprocServer32. A raw CoCreateInstance therefore + /// loads the shim, which then calls LoadLibraryShim to bind a runtime before delegating + /// activation to that runtime's class factory. + /// + /// + /// In hosts where the CLR was loaded through the native hosting APIs + /// (CLRCreateInstance / ICLRRuntimeHost) rather than the standard + /// mscoree entry point that runs when a managed .exe is launched + /// normally, the shim's bound-runtime state is not initialized and + /// LoadLibraryShim fails with CLR_E_SHIM_RUNTIMELOAD (0x80131700). + /// (One concrete example is a native test harness that embeds MSBuild in-process + /// via BuildManager.) Calling DllGetClassObjectInternal on the + /// already-loaded clr.dll bypasses the shim entirely and delegates straight + /// to the CLR's class factory — which is what the CLR's own managed-COM activation + /// does internally for CLSIDs on its hosted-CLSID list. + /// + /// + internal const string ClrDllGetClassObjectInternalExportName = "DllGetClassObjectInternal"; + private IClassFactory* _classFactory; private ComClassFactory(IClassFactory* classFactory) @@ -27,7 +58,9 @@ private ComClassFactory(IClassFactory* classFactory) } /// - /// Attempts to get a class factory for the given COM class ID. + /// Attempts to get a class factory for the given COM class ID via the standard + /// CoGetClassObject path. Goes through the COM registry and may load a + /// server DLL. /// public static bool TryCreate( Guid classId, @@ -53,6 +86,125 @@ public static bool TryCreate( return true; } + /// + /// Attempts to get a class factory by calling the named module's standard + /// DllGetClassObject export directly, bypassing the COM registry and + /// any shim that CoGetClassObject would normally invoke. + /// + /// + /// Module to resolve. May be a bare DLL name. The method first tries + /// GetModuleHandle (no refcount, no DLL-search path) and falls back to + /// LoadLibrary only if the module is not already loaded. + /// + /// CLSID of the COM class to activate. + /// On success, the wrapped class factory. + /// HRESULT from the underlying call. + public static bool TryCreateFromModule( + string moduleName, + Guid classId, + [NotNullWhen(true)] out ComClassFactory? factory, + out HRESULT result) + => TryCreateFromModule(moduleName, classId, DllGetClassObjectExportName, out factory, out result); + + /// + /// Name of the class-object entry point exported by the module. Pass + /// to activate + /// CLR-hosted CLSIDs without triggering the mscoree.dll shim. + /// + /// +#pragma warning disable CS1573 // analyzer doesn't see params merged from + public static bool TryCreateFromModule( + string moduleName, + Guid classId, + string exportName, + [NotNullWhen(true)] out ComClassFactory? factory, + out HRESULT result) +#pragma warning restore CS1573 + { + factory = null; + result = HRESULT.S_OK; + + // Prefer GetModuleHandle: it asserts "module must already be loaded" (true for + // any DLL implementing a CLSID we want to activate in this process), does not + // touch the loader refcount, and sidesteps DLL-search-order concerns. Fall back + // to LoadLibrary only when the module isn't already mapped. + HMODULE module; + bool ownsModuleRef; + fixed (char* pModuleName = moduleName) + { + module = PInvoke.GetModuleHandle(pModuleName); + if (module.IsNull) + { + module = PInvoke.LoadLibrary(pModuleName); + ownsModuleRef = true; + } + else + { + ownsModuleRef = false; + } + } + + if (module.IsNull) + { + result = (HRESULT)Marshal.GetHRForLastWin32Error(); + if (result.Succeeded) + { + result = (HRESULT)unchecked((int)0x80004005); // E_FAIL + } + + return false; + } + + // On success we keep the class factory alive and the module must remain loaded + // for its vtable to be callable. If we acquired the only ref via LoadLibrary, + // intentionally leak it: a single unmatched ref over the lifetime of this + // ComClassFactory is cheaper than tracking the HMODULE through Dispose. + bool keepModuleLoaded = false; + try + { + FARPROC proc = PInvoke.GetProcAddress(module, exportName); + if (proc.IsNull) + { + // GetProcAddress is not required to call SetLastError; the returned + // HRESULT can therefore be S_OK on failure. Force a failing code so + // callers that branch on result.Failed see a deterministic value. + result = (HRESULT)Marshal.GetHRForLastWin32Error(); + if (result.Succeeded) + { + result = (HRESULT)unchecked((int)0x80004005); // E_FAIL + } + + return false; + } + + IClassFactory* classFactory; + Guid iid = typeof(IClassFactory).GUID; + + // DllGetClassObject is STDAPI (__stdcall) in COM headers; on x86 that + // matters, on x64 there's a single AMD64 calling convention. + result = ((delegate* unmanaged[Stdcall])proc.Value)( + &classId, + &iid, + (void**)&classFactory); + + if (result.Failed || classFactory is null) + { + return false; + } + + factory = new ComClassFactory(classFactory); + keepModuleLoaded = true; + return true; + } + finally + { + if (!keepModuleLoaded && ownsModuleRef) + { + PInvoke.FreeLibrary(module); + } + } + } + /// /// Creates an instance of the COM class via the class factory. /// diff --git a/src/Tasks.UnitTests/AssemblyDependency/AssemblyInformation_Tests.cs b/src/Tasks.UnitTests/AssemblyDependency/AssemblyInformation_Tests.cs new file mode 100644 index 00000000000..d513ce21c81 --- /dev/null +++ b/src/Tasks.UnitTests/AssemblyDependency/AssemblyInformation_Tests.cs @@ -0,0 +1,298 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Regression coverage for commit 56826722 (CsWin32 CLR metadata migration) and +// its follow-up fix. Scope: net472 only. The .NET Core RAR path does not use +// IMetaDataDispenser / RegMeta; it reads PE metadata through +// System.Reflection.Metadata.PEReader. This file is gated on +// !FEATURE_ASSEMBLYLOADCONTEXT so it compiles into Microsoft.Build.Tasks.UnitTests +// only when targeting net472. +#if !FEATURE_ASSEMBLYLOADCONTEXT + +using System; +using System.IO; +using System.Reflection; +using Microsoft.Build.Tasks; +using Microsoft.Build.UnitTests; +using Shouldly; +using Xunit; + +namespace Microsoft.Build.Tasks.UnitTests +{ + /// + /// Regression coverage for after #13853 migrated + /// the CLR metadata path from RCW + Marshal.ReleaseComObject to struct-based COM + /// through AgileComPointer / GIT. Two distinct concerns covered here: + /// + /// + /// File-mapping lifetime — IMetaDataDispenser::OpenScope memory-maps the source + /// PE. After Dispose, both GIT cookies must be revoked, the underlying RegMeta + /// refcount must hit zero, and the mapping must be released so the caller can + /// delete or overwrite the file. This was the original suspicion behind the VC + /// regression; investigation falsified it, but the behavior is still required + /// and these tests guard against a future refcount or GIT-revocation leak. + /// + /// + /// Dispenser activation — IMetaDataDispenser must be activated through a path + /// that works in any host running managed code, including hosts that did not + /// enter the CLR via the mscoree.dll shim's startup path. The actual cause of + /// the VC P2PReferences.08 regression was that the commit switched activation + /// to raw CoCreateInstance, which loads the mscoree shim and fails with + /// CLR_E_SHIM_RUNTIMELOAD (0x80131700) in embedded-BuildManager hosts. The fix + /// calls clr.dll's exported DllGetClassObjectInternal directly via + /// . + /// + /// + /// + public sealed class AssemblyInformation_Tests + { + private readonly ITestOutputHelper _output; + + public AssemblyInformation_Tests(ITestOutputHelper output) + { + _output = output; + } + + /// + /// Copies a known-good managed PE into a TestEnvironment-scoped temp file so + /// delete / overwrite attempts don't trip on the test runner's own load of + /// the test assembly, and so cleanup is handled by the test infrastructure. + /// + private static string CopyTestAssemblyInto(TestEnvironment env) + { + string source = typeof(AssemblyInformation_Tests).Assembly.Location; + TransientTestFile destFile = env.GetTempFile(".dll"); + File.Copy(source, destFile.Path, overwrite: true); + return destFile.Path; + } + + [WindowsOnlyFact] + public void Dispose_ReleasesFileSoCallerCanDeleteImmediately() + { + // The RegMeta object opened by IMetaDataDispenser::OpenScope memory-maps the + // PE. After Dispose, both AgileComPointer GIT cookies should be revoked, the + // RegMeta refcount should hit zero, and the mapping should be released. We + // verify by attempting a synchronous File.Delete on the same thread. + using TestEnvironment env = TestEnvironment.Create(_output); + string path = CopyTestAssemblyInto(env); + + using (var info = new AssemblyInformation(path)) + { + // Touch Dependencies to force the metadata enum APIs to run, which is + // what RAR does. Mere construction does not exercise every code path. + _ = info.Dependencies; + _ = info.FrameworkNameAttribute; + } + + // If the mapping is still alive, Win32 returns ERROR_USER_MAPPED_FILE + // (1224) which surfaces as IOException "The requested operation cannot + // be performed on a file with a user-mapped section open." Plain + // sharing locks surface as IOException with ERROR_SHARING_VIOLATION. + Should.NotThrow(() => File.Delete(path)); + } + + [WindowsOnlyFact] + public void Dispose_ReleasesFileSoCallerCanOverwriteImmediately() + { + // Mirror of the P2PReferences scenario: build emits a fresh copy of the + // dependency DLL into a downstream output dir. If RAR (which constructed + // AssemblyInformation over this DLL on an upstream pass) left the source + // mapped, the Copy task on the downstream pass would fail. + using TestEnvironment env = TestEnvironment.Create(_output); + string path = CopyTestAssemblyInto(env); + + using (var info = new AssemblyInformation(path)) + { + _ = info.Dependencies; + } + + // Overwrite the file in place. ERROR_USER_MAPPED_FILE here is the + // signature failure for the leaked-mapping hypothesis. + Should.NotThrow(() => + File.WriteAllBytes(path, new byte[] { 0x4D, 0x5A })); + } + + [WindowsOnlyFact] + public void RepeatedConstructAndDispose_DoesNotAccumulateLocks() + { + // VC's CLR+CLR+CLR scenario walks A -> B -> C and re-opens each binary on + // multiple RAR invocations. Make sure the GIT cookies issued on iteration N + // are gone by iteration N+1 — a leak per iteration would push us past the + // GIT entry cap eventually and lock the file the whole time. + using TestEnvironment env = TestEnvironment.Create(_output); + string path = CopyTestAssemblyInto(env); + + for (int i = 0; i < 25; i++) + { + using var info = new AssemblyInformation(path); + _ = info.Dependencies; + _ = info.FrameworkNameAttribute; + } + + Should.NotThrow(() => File.Delete(path)); + } + + /// + /// Looks for an in-the-box .NET Framework assembly we can use as a non-test + /// assembly target. CustomMarshalers.dll ships under + /// %WINDIR%\Microsoft.NET\Framework[64]\v4.0.30319 on any machine with .NET + /// Framework 4.x. Returns null if absent (clean source-build machine), in + /// which case the using test is skipped. + /// + private static string? TryGetCandidateFrameworkDll() // null = absent + { + string windir = Environment.GetEnvironmentVariable("SystemRoot") ?? @"C:\Windows"; + string archDir = IntPtr.Size == 8 ? "Framework64" : "Framework"; + string path = Path.Combine(windir, "Microsoft.NET", archDir, "v4.0.30319", "CustomMarshalers.dll"); + return File.Exists(path) ? path : null; + } + + [WindowsOnlyFact] + public void FrameworkDll_ReadAndDisposeReleasesFile() + { + // Run the full AssemblyInformation read path over a different file than + // the test assembly itself (which is what every other lifetime test uses), + // then attempt to delete it. ERROR_USER_MAPPED_FILE here would indicate + // a leaked metadata mapping. + string? source = TryGetCandidateFrameworkDll(); + if (source is null) + { + Assert.Skip("No candidate framework DLL available on this machine."); + } + + using TestEnvironment env = TestEnvironment.Create(_output); + TransientTestFile destFile = env.GetTempFile(".dll"); + File.Copy(source!, destFile.Path, overwrite: true); + + using (var info = new AssemblyInformation(destFile.Path)) + { + _ = info.Dependencies; + _ = info.FrameworkNameAttribute; + _ = info.Files; + } + + Should.NotThrow(() => File.Delete(destFile.Path)); + } + + [WindowsOnlyFact] + public void FrameworkDll_OpenReadOpenAgain_DoesNotAccumulateState() + { + // Re-reading the same DLL across multiple RAR-shaped passes is the hot + // path in the failing VC test. If any iteration leaves a mapping alive, + // the N+1th attempt to overwrite the file would fail with + // ERROR_USER_MAPPED_FILE (1224). Uses a non-test-assembly file so the + // delete/overwrite attempt is not confounded by the test runner's own + // load of the assembly. + string? source = TryGetCandidateFrameworkDll(); + if (source is null) + { + Assert.Skip("No candidate framework DLL available on this machine."); + } + + using TestEnvironment env = TestEnvironment.Create(_output); + TransientTestFile destFile = env.GetTempFile(".dll"); + File.Copy(source!, destFile.Path, overwrite: true); + + for (int i = 0; i < 10; i++) + { + using var info = new AssemblyInformation(destFile.Path); + _ = info.Dependencies; + _ = info.FrameworkNameAttribute; + } + + // Overwriting in place is the operation the Copy task performs when + // an incremental build refreshes the dependency in the output dir. + Should.NotThrow(() => + File.WriteAllBytes(destFile.Path, new byte[] { 0x4D, 0x5A })); + } + + [WindowsOnlyFact] + public void TwoLiveInstancesOnSameFile_BothReleaseAfterDispose() + { + // The new design wraps each AssemblyInformation's RegMeta with two + // AgileComPointer entries (IMetaDataImport2 + IMetaDataAssemblyImport). + // Two live instances against the same path therefore put four entries + // through the GIT, backing two independent RegMeta objects over the same + // file. Verify both can be disposed and the file can then be removed. + using TestEnvironment env = TestEnvironment.Create(_output); + string path = CopyTestAssemblyInto(env); + + var info1 = new AssemblyInformation(path); + var info2 = new AssemblyInformation(path); + _ = info1.Dependencies; + _ = info2.Dependencies; + + info1.Dispose(); + info2.Dispose(); + + Should.NotThrow(() => File.Delete(path)); + } + + /// + /// Regression for dotnet/msbuild #13853 fallout / VC P2PReferences.08: + /// IMetaDataDispenser must be activated through a path that works in any + /// host running managed code. The fix calls clr.dll's exported + /// DllGetClassObjectInternal directly via + /// , + /// bypassing the mscoree.dll shim that raw CoCreateInstance on + /// CLSID_CorMetaDataDispenser would otherwise load. The shim fails with + /// CLR_E_SHIM_RUNTIMELOAD (0x80131700) in hosts whose bound-runtime state + /// was never set up (e.g., the VC cppxplatdev test harness embedding MSBuild + /// in-process via BuildManager). + /// + /// This test sanity-checks the symptom: + /// must construct, read full metadata, and report transitive dependencies + /// for a normal managed assembly. The xunit test process itself enters the + /// CLR via the standard mscoree startup path, so a pre-fix CoCreateInstance + /// activation would still succeed here — this test cannot fully reproduce + /// the failing host condition. The defense-in-depth is the comment block in + /// AssemblyInformation.cs / MetadataReader.cs explaining why activation must + /// not change back to CoCreateInstance. + /// + [WindowsOnlyFact] + public void Construction_ReportsExpectedDependencies() + { + string path = typeof(AssemblyInformation_Tests).Assembly.Location; + + using AssemblyInformation info = new(path); + Microsoft.Build.Shared.AssemblyNameExtension[] deps = info.Dependencies; + + deps.ShouldNotBeNull(); + deps.Length.ShouldBeGreaterThan(0); + + // Every managed assembly transitively references mscorlib. If the dispenser + // failed to activate, the metadata read path would have thrown a COMException + // (the VC failure mode) and we never reach this assertion. + bool seesMscorlib = false; + foreach (Microsoft.Build.Shared.AssemblyNameExtension d in deps) + { + if (string.Equals(d.Name, "mscorlib", System.StringComparison.OrdinalIgnoreCase)) + { + seesMscorlib = true; + break; + } + } + seesMscorlib.ShouldBeTrue("AssemblyInformation should resolve mscorlib as a dependency of any managed assembly."); + } + + /// + /// Companion regression for #13853: must + /// remain functional across rapid construct-read-dispose cycles. Exercises + /// the module-based activation path on every iteration and would catch a + /// per-iteration leak in GIT cookies or in clr.dll's refcount on the + /// IClassFactory. + /// + [WindowsOnlyFact] + public void RepeatedActivation_DoesNotDegrade() + { + string path = typeof(AssemblyInformation_Tests).Assembly.Location; + for (int i = 0; i < 50; i++) + { + using AssemblyInformation info = new(path); + _ = info.Dependencies; + } + } + } +} + +#endif diff --git a/src/Tasks/AssemblyDependency/AssemblyInformation.cs b/src/Tasks/AssemblyDependency/AssemblyInformation.cs index 2bbaded3439..a968a7dec50 100644 --- a/src/Tasks/AssemblyDependency/AssemblyInformation.cs +++ b/src/Tasks/AssemblyDependency/AssemblyInformation.cs @@ -17,7 +17,6 @@ using Microsoft.Build.Shared; using Microsoft.Build.Shared.FileSystem; #if !FEATURE_ASSEMBLYLOADCONTEXT -using Windows.Win32; using Windows.Win32.Foundation; using Windows.Win32.System.Com; #endif @@ -43,7 +42,9 @@ internal unsafe class AssemblyInformation : DisposableBase private AssemblyNameExtension[] _assemblyDependencies; private string[] _assemblyFiles; #if !FEATURE_ASSEMBLYLOADCONTEXT - // COM pointers stored thread-agile via the GIT. Disposed in DisposeManagedResources. + // COM pointers stored thread-agile via the GIT. Disposed in DisposeUnmanagedResources + // so the finalizer path also revokes the GIT cookies and releases the underlying + // RegMeta — matching the lifetime of the legacy Marshal.ReleaseComObject pattern. // The CLR metadata object returned by IMetaDataDispenser::OpenScope implements // all three of IMetaDataImport, IMetaDataImport2, and IMetaDataAssemblyImport; // we QueryInterface for the two we actually call. The dispenser itself is only @@ -85,30 +86,67 @@ internal AssemblyInformation(string sourceFile) #if !FEATURE_ASSEMBLYLOADCONTEXT // net472-only = inherently Windows. CsWin32 types used directly. - // Activate the dispenser and ask OpenScope directly for IMetaDataImport2 — the - // underlying CLR RegMeta coclass implements every IMetaData* interface, so we save a - // QueryInterface round-trip vs. asking for the base IMetaDataImport. We still need a - // single QI for IMetaDataAssemblyImport since OpenScope only returns one pointer. - // Each ComScope releases at end of method; AgileComPointer (takeOwnership: false) - // AddRefs through GIT registration so the field retains the only persistent reference. - Guid clsid = CorMetadata.CLSID_CorMetaDataDispenser; - Guid dispenserIid = IID.Get(); - using ComScope dispenser = new(); - PInvoke.CoCreateInstance(&clsid, null, CLSCTX.CLSCTX_INPROC_SERVER, &dispenserIid, dispenser) - .ThrowOnFailure(); - - Guid import2Iid = IMetaDataImport2.IID_IMetaDataImport2; - using ComScope import2 = new(); - fixed (char* pPath = sourceFile) + // Activate the dispenser by calling clr.dll's exported DllGetClassObject + // directly via ComClassFactory.TryCreateFromModule. Once we have + // IMetaDataDispenser we stay on the struct-based COM path (OpenScope, QI + // for IMetaDataAssemblyImport, AgileComPointer storage). + // + // IMPORTANT — do NOT call PInvoke.CoCreateInstance(CLSID_CorMetaDataDispenser, ...) + // here, and do NOT use Activator.CreateInstance(Type.GetTypeFromCLSID(...)) + // (which would also block AOT on .NET 10+). + // + // CLSID_CorMetaDataDispenser is registered with mscoree.dll (the .NET + // Framework shim) as its InprocServer32. Raw CoCreateInstance loads the + // shim, which then has to bind a CLR via LoadLibraryShim before delegating + // to MetaDataDllGetClassObject. In hosts that did not enter the CLR via + // mscoree (e.g., the VC cppxplatdev test harness invoking MSBuild + // in-process via BuildManager), the shim's bound-runtime state is not set + // up and LoadLibraryShim returns CLR_E_SHIM_RUNTIMELOAD = 0x80131700 + // ("Failed to load the runtime"). RAR catches that as a + // DependencyResolutionException and silently drops every transitive + // dependency for the failing reference. Regression introduced by + // dotnet/msbuild #13853; observed as VC.Tests.MsBuild.VC.MsBuild.P2PReferences.08. + // + // The right approach is what the CLR's own managed-COM activation path + // does internally: bypass the shim entirely for CLSIDs coming from the runtime + // and call DllGetClassObject directly on the currently-loaded CLR module. + + if (!ComClassFactory.TryCreateFromModule( + "clr.dll", + CorMetadata.CLSID_CorMetaDataDispenser, + ComClassFactory.ClrDllGetClassObjectInternalExportName, + out ComClassFactory factory, + out HRESULT activationHr)) { - dispenser.Pointer->OpenScope(pPath, CorOpenFlags.ofRead, &import2Iid, import2).ThrowOnFailure(); + activationHr.ThrowOnFailure(); } - _import2 = new AgileComPointer(import2.Pointer, takeOwnership: false); - Guid asmIid = IMetaDataAssemblyImport.IID_IMetaDataAssemblyImport; - using ComScope asmImport = new(); - import2.Pointer->QueryInterface(&asmIid, asmImport).ThrowOnFailure(); - _assemblyImport = new AgileComPointer(asmImport.Pointer, takeOwnership: false); + using (factory) + { + using ComScope dispenser = factory.TryCreateInstance(out HRESULT createHr); + createHr.ThrowOnFailure(); + + // OpenScope is asked directly for IMetaDataImport2 — the underlying CLR RegMeta + // coclass implements every IMetaData* interface, so this saves a QueryInterface + // round-trip vs. asking for the base IMetaDataImport. We still need one QI for + // IMetaDataAssemblyImport since OpenScope only returns one pointer. Each + // AgileComPointer (takeOwnership: false) AddRefs through GIT registration so + // the field retains the only persistent reference; ComScope releases the + // transient pointer on scope exit. + Guid import2Iid = IMetaDataImport2.IID_IMetaDataImport2; + using ComScope import2 = new(); + fixed (char* pPath = sourceFile) + { + dispenser.Pointer->OpenScope(pPath, CorOpenFlags.ofRead, &import2Iid, import2).ThrowOnFailure(); + } + + _import2 = new AgileComPointer(import2.Pointer, takeOwnership: false); + + Guid asmIid = IMetaDataAssemblyImport.IID_IMetaDataAssemblyImport; + using ComScope asmImport = new(); + import2.Pointer->QueryInterface(&asmIid, asmImport).ThrowOnFailure(); + _assemblyImport = new AgileComPointer(asmImport.Pointer, takeOwnership: false); + } #endif } @@ -304,10 +342,15 @@ internal AssemblyAttributes GetAssemblyMetadata() using ComScope import2 = _import2.GetInterface(); MdAssembly assemblyScope; - asmImport.Pointer->GetAssemblyFromScope(&assemblyScope).ThrowOnFailure(); + // Tolerate failure here: a scope with no mdAssembly token (a netmodule or other + // non-assembly PE) returns CLDB_E_RECORD_NOTFOUND. The legacy [PreserveSig] RCW + // code silently ignored that HR and fell through to the IsNil check, returning + // null. Preserve that contract; the GetAssembliesMetadata task has no try/catch + // around this call and an exception here would fail the whole task. + HRESULT hr = asmImport.Pointer->GetAssemblyFromScope(&assemblyScope); // get the assembly, if there is no assembly, it is a module reference - if (assemblyScope.IsNil) + if (hr.Failed || assemblyScope.IsNil) { return null; } @@ -382,9 +425,13 @@ internal AssemblyAttributes GetAssemblyMetadata() assemblyAttributes.RuntimeVersion = GetRuntimeVersion(_sourceFile); - uint peKind; - uint machine; - import2.Pointer->GetPEKind(&peKind, &machine).ThrowOnFailure(); + uint peKind = 0; + uint machine; // out-param required by GetPEKind; value not consumed by RAR + // Tolerate failure: GetPEKind can return CLDB_E_FILE_BADREAD or other variant HRs + // on unusual PEs. The legacy [PreserveSig] code ignored the HR and left peKind at + // whatever value (typically 0) the call had written. Match that behavior so the + // GetAssembliesMetadata task does not fail end-to-end on a borderline binary. + _ = import2.Pointer->GetPEKind(&peKind, &machine); assemblyAttributes.PeKind = peKind; return assemblyAttributes; @@ -647,9 +694,6 @@ private static List GetFixedStringArguments(MetadataReader reader, Custo #endif #if !FEATURE_ASSEMBLYLOADCONTEXT - /// - /// Release interface pointers on Dispose(). - /// protected override void DisposeManagedResources() { _import2?.Dispose(); diff --git a/src/Tasks/ManifestUtil/MetadataReader.cs b/src/Tasks/ManifestUtil/MetadataReader.cs index 420be57345c..78f8007ad38 100644 --- a/src/Tasks/ManifestUtil/MetadataReader.cs +++ b/src/Tasks/ManifestUtil/MetadataReader.cs @@ -12,7 +12,6 @@ #else using System.Runtime.InteropServices; using Microsoft.Build.Tasks.Metadata; -using Windows.Win32; using Windows.Win32.Foundation; using Windows.Win32.System.Com; #endif @@ -290,42 +289,56 @@ internal unsafe class MetadataReader : IDisposable private MetadataReader(string path) { _path = path; - // Receive transient pointers directly into ComScope (implicit void** conversion) - // so no try/finally is needed for cleanup. Each AgileComPointer is constructed with - // takeOwnership: false; the GIT registration AddRefs and the ComScope Releases on - // scope exit. + // OpenScope failure is per-file (the path isn't a valid PE / assembly); we leave + // _assemblyImport null so that Create(...) returns null, matching the .NET Core + // branch's catch-all. // - // CoCreateInstance failure is an environment-level problem (CLR metadata host - // missing/unregistered) and is thrown, matching the behavior of the old - // `new CorMetaDataDispenser()` built-in-interop activation. OpenScope failure is - // per-file (the path isn't a valid PE / assembly); we leave _assemblyImport null - // so that Create(...) returns null, matching the .NET Core branch's catch-all. + // IMPORTANT — do NOT call PInvoke.CoCreateInstance(CLSID_CorMetaDataDispenser, ...) + // here, and do NOT use Activator.CreateInstance(Type.GetTypeFromCLSID(...)) if + // we want this code to AOT-compile. See the matching comment in + // AssemblyDependency/AssemblyInformation.cs for the full mechanism (regression + // dotnet/msbuild #13853 / VC P2PReferences.08, HRESULT 0x80131700 + // CLR_E_SHIM_RUNTIMELOAD when raw CoCreateInstance hits the mscoree shim in a + // host where the shim's bound-runtime state is not set up). + // + // ComClassFactory.TryCreateFromModule calls clr.dll's exported DllGetClassObject + // directly, which routes to MetaDataDllGetClassObject and bypasses the shim. // // OpenScope is asked directly for IMetaDataImport2 — the underlying CLR RegMeta // coclass implements every IMetaData* interface, so this saves a QueryInterface // round-trip vs. asking for the base IMetaDataImport. - Guid clsid = CorMetadata.CLSID_CorMetaDataDispenser; - Guid dispenserIid = IID.Get(); - using ComScope dispenser = new(); - PInvoke.CoCreateInstance(&clsid, null, CLSCTX.CLSCTX_INPROC_SERVER, &dispenserIid, dispenser).ThrowOnFailure(); - - Guid import2Iid = IMetaDataImport2.IID_IMetaDataImport2; - using ComScope import2 = new(); - HRESULT hr; - fixed (char* pPath = path) + if (!ComClassFactory.TryCreateFromModule( + "clr.dll", + CorMetadata.CLSID_CorMetaDataDispenser, + ComClassFactory.ClrDllGetClassObjectInternalExportName, + out ComClassFactory factory, + out HRESULT activationHr)) { - hr = dispenser.Pointer->OpenScope(pPath, CorOpenFlags.ofRead, &import2Iid, import2); + activationHr.ThrowOnFailure(); } - if (hr.Failed || import2.IsNull) + using (factory) { - return; - } - _import2 = new AgileComPointer(import2.Pointer, takeOwnership: false); + using ComScope dispenser = factory.TryCreateInstance(out HRESULT createHr); + createHr.ThrowOnFailure(); + + Guid import2Iid = IMetaDataImport2.IID_IMetaDataImport2; + using ComScope import2 = new(); + HRESULT hr; + fixed (char* pPath = path) + { + hr = dispenser.Pointer->OpenScope(pPath, CorOpenFlags.ofRead, &import2Iid, import2); + } + if (hr.Failed || import2.IsNull) + { + return; + } + _import2 = new AgileComPointer(import2.Pointer, takeOwnership: false); - Guid asmIid = IMetaDataAssemblyImport.IID_IMetaDataAssemblyImport; - using ComScope asmImport = new(); - import2.Pointer->QueryInterface(&asmIid, asmImport).ThrowOnFailure(); - _assemblyImport = new AgileComPointer(asmImport.Pointer, takeOwnership: false); + Guid asmIid = IMetaDataAssemblyImport.IID_IMetaDataAssemblyImport; + using ComScope asmImport = new(); + import2.Pointer->QueryInterface(&asmIid, asmImport).ThrowOnFailure(); + _assemblyImport = new AgileComPointer(asmImport.Pointer, takeOwnership: false); + } } public static MetadataReader Create(string path) @@ -339,7 +352,13 @@ public bool HasAssemblyAttribute(string name) using ComScope asmImport = _assemblyImport.GetInterface(); using ComScope import2 = _import2.GetInterface(); MdAssembly assemblyScope; - asmImport.Pointer->GetAssemblyFromScope(&assemblyScope).ThrowOnFailure(); + // Tolerate failure: a scope with no mdAssembly token returns CLDB_E_RECORD_NOTFOUND. + // The legacy [PreserveSig] RCW code ignored the HR; preserve that contract by + // treating any non-S_OK as "attribute not present". + if (asmImport.Pointer->GetAssemblyFromScope(&assemblyScope).Failed || assemblyScope.IsNil) + { + return false; + } // The CLR returns S_OK with pcbData=size when the attribute is present, S_FALSE with // pcbData=0 when it is absent, and an error HRESULT otherwise. Treat anything that is