From 8085d18449da10428122a369c4e5d27541183cd3 Mon Sep 17 00:00:00 2001 From: Jeremy Kuhne Date: Tue, 26 May 2026 17:10:22 -0700 Subject: [PATCH 1/4] CsWin32 follow-up: WindowsNative + VS Setup Configuration + remaining hand-rolled interop Continues the CsWin32 struct-based interop migration from #13746 and #13853. Removes the legacy Microsoft.VisualStudio.Setup.Configuration.Interop RCW package outright and migrates the directory-enumeration / FileTracker / VS locator / test-fixture call sites to PInvoke + ComScope. New manual COM struct definitions src/Framework/Shared/VisualStudio/ SetupConfiguration.cs CLSID + GetSetupConfiguration fallback DllImport ISetupConfiguration.cs v1 (IUnknown surface only, for QI) ISetupConfiguration2.cs EnumAllInstances (vtable slot 6) IEnumSetupInstances.cs Next (slot 3) ISetupInstance.cs GetInstallationPath/Version/DisplayName (slots 6/7/8) ISetupInstance2.cs GetState (slot 11) InstanceState.cs [Flags] enum InstanceState : uint, Complete = MAXUINT Rewrites src/Framework/VisualStudioLocationHelper.cs Replaces the RCW-based GetInstances() with PInvoke.CoCreateInstance + REGDB_E_CLASSNOTREG fallback to GetSetupConfiguration P/Invoke + QI. Pointer lifetimes managed entirely via ComScope; BSTR out-params via `using BSTR x = default;`. Helper returns `default` ComScope on COM failure rather than throwing a COMException the caller would discard. src/Framework/FileSystem/WindowsNative.cs src/Framework/FileSystem/SafeFileHandle.cs FindFirstFileW / FindNextFileW / FindClose / PathMatchSpecExW migrated to PInvoke.FindFirstFile / FindNextFile / FindClose / PathMatchSpecEx (added to NativeMethods.txt). The legacy hand-shaped Win32FindData (with [MarshalAs(UnmanagedType.ByValTStr)] string fields) is preserved as a caller-facing adapter built from the blittable WIN32_FIND_DATAW. SafeFindFileHandle now wraps a manually-constructed HANDLE and its ReleaseHandle calls PInvoke.FindClose. src/Shared/InprocTrackingNativeMethods.cs FileTracker.dll loader rewritten to use PInvoke.LoadLibrary + PInvoke.GetProcAddress with typed delegate* unmanaged[Stdcall]<...> function pointers matching the actual export signatures from FileTracker.h / FileTracker.def (StartTrackingContext @2 ... SetThreadCount @10). The SafeLibraryHandle subclass and all [UnmanagedFunctionPointer] managed delegate types are deleted; the public API surface gets full XML doc comments. The HMODULE is intentionally never freed (FileTracker detours ExitProcess; unloading mid-shutdown would corrupt the CLR). ANSI export-name encoding uses Encoding.Default + a pooled byte[] (correct for DBCS code pages). Test fixtures migrated to CsWin32 src/UnitTests.Shared/DriveMapping.cs DefineDosDevice / QueryDosDevice -> PInvoke.* with DEFINE_DOS_DEVICE_FLAGS. src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs LoadLibrary / FindResource / LoadResource / LockResource / SizeofResource -> PInvoke.LoadLibraryEx / FindResource / LoadResource / LockResource / SizeofResource consuming HMODULE / HRSRC / HGLOBAL. src/Build.UnitTests/BackEnd/TargetUpToDateChecker_Tests.cs Hand-rolled CreateSymbolicLink / CreateFileForSymlink / SetFileTime replaced with wrappers over PInvoke.* (CreateSymbolicLink already in NativeMethods.txt; SetFileTime added). Deletion src/Build/BackEnd/Node/NativeMethods.cs (~230 lines) The CreateProcess + STARTUP_INFO + PROCESS_INFORMATION + SECURITY_ATTRIBUTES wrapper was consumed by exactly one place -- FileTrackerTests' [Fact(Skip=...)] helper methods. That helper now defines a small private BackEndNativeMethods static class inline, forwarding to PInvoke.CreateProcess and bridging the test-facing struct shapes to Win32 STARTUPINFOW / PROCESS_INFORMATION / SECURITY_ATTRIBUTES at the call. Package reference removed (now unused everywhere) - src/Framework/Microsoft.Build.Framework.csproj - src/Build/Microsoft.Build.csproj - src/Utilities/Microsoft.Build.Utilities.csproj - src/Tasks/Microsoft.Build.Tasks.csproj - src/Framework.UnitTests/Microsoft.Build.Framework.UnitTests.csproj PR #13853 review feedback (carried in) src/Tasks/ManifestUtil/MetadataReader.cs src/Tasks/ManifestUtil/ApplicationManifest.cs Adds MetadataReader.HasAssemblyAttributes(string[], bool[]) batch overload on both .NETCore and net472 paths; the net472 path acquires IMetaDataAssemblyImport / IMetaDataImport2 from the GIT once and reuses the same GetAssemblyFromScope across all probes. AssemblyAttributeFlags in ApplicationManifest.cs now does one GIT round-trip for all four attribute checks instead of eight (Jan's review comment). src/Tasks.UnitTests/AssemblyInformation_Tests.cs (new) Two real-file tests exercising the full CLR-metadata COM call chain end-to-end on net472 (CoCreateInstance -> OpenScope -> QI -> GetAssemblyProps -> GetCustomAttributeByName x4 -> GetPEKind), with parity assertions against AssemblyName.GetAssemblyName / TargetFrameworkAttribute (Viktor's review comment). Other new tests src/Tasks.UnitTests/MetadataReader_Tests.cs HasAssemblyAttributes_Batch_AgreesWithSingular: cross-validates the new batch overload against the singular overload for both known-true and known-false attributes. src/Framework.UnitTests/FileSystem/WindowsNative_Tests.cs (new) Six tests directly covering the FindFirstFile / FindNextFile / PathMatchSpecEx migration + the Win32FindData adapter + SafeFindFileHandle idempotent-dispose, with WindowsOnlyFact gating. src/Framework.UnitTests/VisualStudioLocationHelper_Tests.cs (new) Six baseline tests asserting the contract that GetInstances() never throws, never returns null, that each returned VisualStudioInstance has valid Name/Path/Version (Major >= 15) shape, paths are unique, and existing paths are absolute directories. Establishes the regression net before the RCW -> struct-based COM migration. NativeMethods.txt additions DefineDosDevice, QueryDosDevice, FindClose, FindFirstFile, FindNextFile, FindResource, LoadResource, LockResource, SizeofResource, LoadLibraryEx, LOAD_LIBRARY_FLAGS, PathMatchSpecEx, SetFileTime, PCSTR. Skill updates .github/skills/cswin32-com/SKILL.md Compressed from ~5,400 -> ~3,520 estimated tokens (-35%). Adds: - "Lifetime: using ComScope" promoted to workflow step 2; raw try/finally Release pattern explicitly banned. - Helper "ownership-transfer" pattern (return ComScope from a helper) with the AcquireSetupConfiguration2 worked example. - BSTR scoping bullet -- `using BSTR x = default;` replaces manual try/finally + SysFreeString. - "Don't throw when the caller swallows it" subsection under Error-Handling Parity, citing AcquireSetupConfiguration2. Verified - Full build: dotnet build MSBuild.Dev.slnf -> 0 warnings, 0 errors. - Tests: Framework.UnitTests 750/750 net472, 721/721 net10.0 Tasks.UnitTests new MetadataReader/AssemblyInformation + AddToWin32Manifest tests pass on both TFMs. - Source build (DotNetBuildSourceOnly=true) unchanged: every new interop file is gated #if FEATURE_WINDOWSINTEROP (and && FEATURE_VISUALSTUDIOSETUP for the VS Setup interfaces). --- .github/skills/cswin32-com/SKILL.md | 303 ++++++------- .../BackEnd/TargetUpToDateChecker_Tests.cs | 61 +-- .../Microsoft.Build.Engine.UnitTests.csproj | 1 + src/Build/BackEnd/Node/NativeMethods.cs | 229 ---------- src/Build/Microsoft.Build.csproj | 1 - .../FileSystem/WindowsNative_Tests.cs | 153 +++++++ ...Microsoft.Build.Framework.UnitTests.csproj | 1 - .../VisualStudioLocationHelper_Tests.cs | 127 ++++++ src/Framework/FileSystem/SafeFileHandle.cs | 18 +- src/Framework/FileSystem/WindowsNative.cs | 64 ++- .../Microsoft.Build.Framework.csproj | 1 - src/Framework/NativeMethods.txt | 16 +- .../VisualStudio/IEnumSetupInstances.cs | 85 ++++ .../VisualStudio/ISetupConfiguration.cs | 66 +++ .../VisualStudio/ISetupConfiguration2.cs | 87 ++++ .../Shared/VisualStudio/ISetupInstance.cs | 107 +++++ .../Shared/VisualStudio/ISetupInstance2.cs | 90 ++++ .../Shared/VisualStudio/InstanceState.cs | 45 ++ .../Shared/VisualStudio/SetupConfiguration.cs | 47 ++ src/Framework/VisualStudioLocationHelper.cs | 195 ++++++--- src/Shared/InprocTrackingNativeMethods.cs | 406 +++++++++--------- .../AddToWin32Manifest_Tests.cs | 49 +-- src/Tasks.UnitTests/MetadataReader_Tests.cs | 32 ++ .../Microsoft.Build.Tasks.UnitTests.csproj | 1 + src/Tasks/ManifestUtil/ApplicationManifest.cs | 21 +- src/Tasks/ManifestUtil/MetadataReader.cs | 55 ++- src/Tasks/Microsoft.Build.Tasks.csproj | 1 - src/UnitTests.Shared/DriveMapping.cs | 44 +- ...Microsoft.Build.Utilities.UnitTests.csproj | 4 +- .../TrackedDependencies/FileTrackerTests.cs | 144 ++++++- .../Microsoft.Build.Utilities.csproj | 1 - 31 files changed, 1662 insertions(+), 793 deletions(-) delete mode 100644 src/Build/BackEnd/Node/NativeMethods.cs create mode 100644 src/Framework.UnitTests/FileSystem/WindowsNative_Tests.cs create mode 100644 src/Framework.UnitTests/VisualStudioLocationHelper_Tests.cs create mode 100644 src/Framework/Shared/VisualStudio/IEnumSetupInstances.cs create mode 100644 src/Framework/Shared/VisualStudio/ISetupConfiguration.cs create mode 100644 src/Framework/Shared/VisualStudio/ISetupConfiguration2.cs create mode 100644 src/Framework/Shared/VisualStudio/ISetupInstance.cs create mode 100644 src/Framework/Shared/VisualStudio/ISetupInstance2.cs create mode 100644 src/Framework/Shared/VisualStudio/InstanceState.cs create mode 100644 src/Framework/Shared/VisualStudio/SetupConfiguration.cs diff --git a/.github/skills/cswin32-com/SKILL.md b/.github/skills/cswin32-com/SKILL.md index 72b7fd71c7c..e183a25fe7c 100644 --- a/.github/skills/cswin32-com/SKILL.md +++ b/.github/skills/cswin32-com/SKILL.md @@ -8,257 +8,194 @@ argument-hint: 'Describe the COM interface or activation pattern you are working Struct-based COM interop using CsWin32 patterns — AOT-compatible, no `[ComImport]` or built-in marshalling. -**Paired skill:** [cswin32-interop](../cswin32-interop/SKILL.md) covers general P/Invoke (`[DllImport]` migration, `FEATURE_WINDOWSINTEROP` gating, blittable signature rules that apply to both `[DllImport]` and COM vtables, `BufferScope`, source-build verification). This file covers only the COM-specific layer on top. +**Paired skill:** [cswin32-interop](../cswin32-interop/SKILL.md) covers general P/Invoke and the blittable signature rules that apply to both `[DllImport]` and COM vtables. This file covers only the COM-specific layer. ## Workflow -1. **Determine if the interface is in Win32 metadata.** If yes, add the name to `src/Framework/NativeMethods.txt` — CsWin32 generates it. If no (e.g. WMI), define a manual struct (see below). -2. **Create a `ComScope`** for lifetime management: `using ComScope scope = new();` -3. **Activate the COM object** via `ComClassFactory.TryCreate(CLSID, ...)` or `PInvoke.CoCreateInstance` with `IID.Get()`. -4. **Call methods** via `scope.Pointer->Method(...)`. Pass `ComScope` directly as `T**` output parameters. -5. **Guard with `#if FEATURE_WINDOWSINTEROP`** — add `&& NET` only when the struct uses - the static-abstract `IComIID` form *exclusively*. The dual-target pattern below works - on net472 without `&& NET`. - -## COM Interfaces in Win32 Metadata - -Add the interface name to `src/Framework/NativeMethods.txt` → CsWin32 generates it → use `ComScope`: - -```csharp -#if FEATURE_WINDOWSINTEROP -using ComScope rot = new(); -HRESULT hr = PInvoke.GetRunningObjectTable(0, rot); -if (hr.Failed) return; -rot.Pointer->SomeMethod(...); -#endif -``` +1. **Interface in Win32 metadata?** Add the name to `src/Framework/NativeMethods.txt` → CsWin32 generates it. Not in metadata (WMI, Fusion, Setup Configuration) → define a manual struct under its own folder, excluded from source builds via ``. +2. **Lifetime: `using ComScope scope = new();`** for every transient COM pointer (`CoCreateInstance`, `QueryInterface`, `IEnumXxx::Next`, factory output, app-local `STDAPI Get*`, etc.). Never write `T* local; try { ... } finally { local->Release(); }` — that's the pre-`ComScope` shape that leaks on every early return. Same for `BSTR` out-params: `using BSTR x = default;`. +3. **Activate** via `ComClassFactory.TryCreate(CLSID, ...)` (AOT-compatible) or `PInvoke.CoCreateInstance` with `IID.Get()` — not `&localGuid`. +4. **Call** via `scope.Pointer->Method(...)`. Pass `ComScope` directly where the API expects `T**` / `void**` — the implicit operator does the address-of. +5. **Match the caller's error contract.** If the top-level consumer swallows COM failure ("no result" == "absent"), helpers return `default` / `false` instead of throwing a `COMException` that's immediately discarded. `ThrowOnFailure` only when the exception will actually propagate or assert a should-never-happen. +6. **Guard with `#if FEATURE_WINDOWSINTEROP`** — add `&& NET` only when the struct uses the static-abstract `IComIID` form exclusively (no net472 dual-target). ## Manual COM Structs (Not in Metadata) -For interfaces not in Win32 metadata (e.g. WMI, Fusion), define struct-based implementations in their own files, excluded via `` in source builds. Guard with `#if FEATURE_WINDOWSINTEROP && NET` when the struct uses **only** the static-abstract `IComIID` member (only emitted on .NET 7+). If the struct needs to compile on net472, provide **both** the static-abstract member (gated `#if NET`) and an instance member (gated `#else`) so the right one binds against the per-target `IComIID` shape — see Fusion structs in `src/Tasks/AssemblyDependency/Fusion/` for this pattern. +Define each interface in its own file under e.g. `src/Tasks/AssemblyDependency/Fusion/`, `src/Framework/Shared/VisualStudio/`, `src/Framework/Utilities/Wmi/`. Exclude from source builds. Pattern (dual-target — works on net472 + .NET): ```csharp -[SupportedOSPlatform("windows6.1")] -internal unsafe struct IWbemLocator : IComIID +internal unsafe struct IAssemblyCache : IComIID { - public static Guid Guid { get; } = new(0xDC12A687, ...); + public static readonly Guid IID_IAssemblyCache = new(0xE707DCDE, ...); - // .NET 7+ static abstract IComIID implementation +#if NET static ref readonly Guid IComIID.Guid { [MethodImpl(MethodImplOptions.AggressiveInlining)] - get - { - ReadOnlySpan data = [ /* 16 GUID bytes */ ]; - return ref Unsafe.As(ref MemoryMarshal.GetReference(data)); - } + get => ref Unsafe.AsRef(in IID_IAssemblyCache); } +#else + readonly Guid IComIID.Guid => IID_IAssemblyCache; +#endif private readonly void** _lpVtbl; - // IUnknown (vtable 0-2) + interface methods at correct indices - public HRESULT ConnectServer(char* strNetworkResource, ...) { - fixed (IWbemLocator* pThis = &this) - return ((delegate* unmanaged[Stdcall])_lpVtbl[3])(pThis, ...); + // IUnknown at indices 0-2, then interface methods at 3+ + public HRESULT QueryInterface(Guid* riid, void** ppvObject) + { + fixed (IAssemblyCache* pThis = &this) + return ((delegate* unmanaged[Stdcall])_lpVtbl[0])(pThis, riid, ppvObject); } - - public static Guid CLSID { get; } = new(0x4590F811, ...); + // AddRef @1, Release @2, then interface methods at correct slot indices. } ``` -**Requirements:** -- `delegate* unmanaged[Stdcall]` for the function-pointer cast -- Static-abstract `IComIID` on .NET 7+ (gate manual structs with `#if NET`); the net472 polyfill is instance-based and is **not** attached to CsWin32-generated structs, so `ComScope` over generated COM types is .NET-only -- Use the CsWin32-generated `PCWSTR` / `PWSTR` for wide string parameters (add the type to `NativeMethods.txt`); raw `char*` only when no typed equivalent exists -- CS0592 prevents `[SupportedOSPlatform]` on structs — put on individual methods instead +**Rules:** +- `delegate* unmanaged[Stdcall]` for the function-pointer cast — IDL `STDMETHODCALLTYPE` ⇒ `__stdcall` on Win32. Picking the wrong convention silently corrupts the stack. +- Static-abstract `IComIID` is .NET 7+ only. For .NET-only structs (WMI), drop the `#else` branch and gate the whole file `#if NET`. For dual-target structs (Fusion), keep both as shown — see `src/Tasks/AssemblyDependency/Fusion/` for the canonical example. +- Use CsWin32-generated `PCWSTR` / `PWSTR` for wide strings (add to `NativeMethods.txt`); raw `char*` only when no typed equivalent exists. +- Vtable slots are exact: index 0 = `QueryInterface`, 1 = `AddRef`, 2 = `Release`, 3+ = interface methods in IDL order. When inheriting, **add the parent interface's method count** before counting your own (e.g. `ISetupConfiguration2.EnumAllInstances` is at slot 6 = 3 IUnknown + 3 v1 + 0). +- CS0592 prevents `[SupportedOSPlatform]` on structs — put it on individual methods. -### Dual-target manual structs (net472 + .NET) +## Lifetime & Access -When a manual COM struct must compile on both net472 and .NET, declare both forms of the `IComIID` member in the same struct so the right one binds against the per-target `IComIID` shape (CsWin32-generated static-abstract on .NET, polyfill instance member on net472): +**A raw `T*` (COM struct) is forbidden as a field on a non-`ref` type.** Allowed locations: locals in `unsafe` methods, parameters, fields of a `ref struct`. Anywhere else (instance fields of `class` / non-`ref` `struct`) → use `AgileComPointer` — a raw pointer field is an apartment-agility hazard and leaks on finalize-without-Dispose. + +### `ComScope` — transient pointers + +`ref struct`, `using`'d, calls `Release` on dispose. **The default for everything that doesn't survive the current method.** Implicitly converts to `T**` and `void**`, so out-params write into the scope directly: ```csharp -internal unsafe struct IAssemblyCache : IComIID +using ComScope scope = new(); +Guid clsid = SomeStruct.CLSID; +Guid iid = IID.Get(); +PInvoke.CoCreateInstance(&clsid, null, CLSCTX.CLSCTX_INPROC_SERVER, &iid, scope).ThrowOnFailure(); +scope.Pointer->DoThing(...); + +using ComScope other = new(); +Guid otherIid = IOther.IID_IOther; +scope.Pointer->QueryInterface(&otherIid, other).ThrowOnFailure(); +``` + +- Access methods via `scope.Pointer->Method(...)`. Null-check with `scope.IsNull`. +- Applies to *every* COM out-param: `CoCreateInstance`, `QueryInterface`, `IEnumXxx::Next`, factory methods, app-local `STDAPI Get*` (declare the `[DllImport]` with `T** pp`, not `out T*`, so the implicit operator binds). + +**Ownership transfer out of a helper.** A helper that acquires a COM pointer can return `ComScope` directly; intermediate pointers stay in their own `using` scopes and Release on the helper's `return`. `default` `ComScope` is null and its `Dispose` is a no-op, so callers don't need an extra null guard: + +```csharp +private static ComScope AcquireSetupConfig() { - public static readonly Guid IID_IAssemblyCache = new(0xE707DCDE, ...); + Guid clsid = SetupConfiguration.CLSID_SetupConfiguration; + Guid iid = ISetupConfiguration2.IID_ISetupConfiguration2; -#if NET - static ref readonly Guid IComIID.Guid - { - [MethodImpl(MethodImplOptions.AggressiveInlining)] - get => ref Unsafe.AsRef(in IID_IAssemblyCache); - } -#else - readonly Guid IComIID.Guid => IID_IAssemblyCache; -#endif + ComScope config = new(); + HRESULT hr = PInvoke.CoCreateInstance(&clsid, null, CLSCTX.CLSCTX_INPROC_SERVER, &iid, config); + if (hr.Succeeded) return config; // ownership flows to caller + if (hr != HRESULT.REGDB_E_CLASSNOTREG) return default; - private readonly void** _lpVtbl; - // ... IUnknown + interface methods + using ComScope v1 = new(); // intermediate, Released on return + if (SetupConfiguration.GetSetupConfiguration(v1, IntPtr.Zero) < 0 || v1.IsNull) return default; + return v1.Pointer->QueryInterface(&iid, config).Succeeded ? config : default; } + +// caller: +using ComScope config = AcquireSetupConfig(); +if (!config.IsNull) { /* use config.Pointer */ } ``` -See `src/Tasks/AssemblyDependency/Fusion/` for the full pattern. - -### Blittable vtable signatures - -COM vtable methods must be blittable for the same reason `[DllImport]` signatures must be — -the runtime does no marshalling work. **Follow the general blittable rules in -[cswin32-interop](../cswin32-interop/SKILL.md#blittable-signatures)** (return `HRESULT`, -`.ThrowOnFailure()`, `T**` not `out T*`, `void*` for opaque, `PCWSTR` / `PWSTR` for wide -strings, typed flag enums, no managed reference types). COM-vtable-specific additions: - -- **Function-pointer cast must match the native calling convention** — for COM vtables that - is almost always `delegate* unmanaged[Stdcall]` (the IDL `STDMETHODCALLTYPE` macro expands to - `__stdcall` on Win32). The general form is `delegate* unmanaged[Cc]` where `Cc` matches the - native side: `Cdecl` for varargs / printf-style APIs, `Thiscall` for C++ instance method - pointers, `Fastcall` for rare classic 32-bit APIs. Picking the wrong convention silently - corrupts the stack on the call. The compiler emits an IL `calli` instruction either way and - works on net472. -- **`[ComImport]` `PreserveSig` defaults the opposite way from `[DllImport]`** — `false` vs - `true`. When migrating a `[ComImport]` interface, every method that lacked an explicit - `[PreserveSig]` was previously throwing on failure HRESULTs; the new struct-based method - must call `.ThrowOnFailure()` at every call site to preserve that contract. See - "Error-Handling Parity When Migrating" below. -- **Vtable indices are exact** — unused slots may be omitted as long as the indices of the - ones you expose are correct relative to the native interface layout (count from 3 after - `QueryInterface` / `AddRef` / `Release`, and add the parent interface's method count when - inheriting). - -### Strongly-typed handle / token wrappers - -When the native side `typedef`s a primitive into a family of "same shape, different meaning" -aliases — e.g. `corhdr.h`'s `typedef ULONG32 mdToken; typedef mdToken mdAssembly; typedef mdToken mdAssemblyRef;` — -mirror that hierarchy with distinct `readonly struct` wrappers (same pattern CsWin32 uses for -`HANDLE` / `HWND` / `HMODULE`). Each wrapper holds a single field of the underlying primitive -so it stays blittable and ABI-compatible with the native type; the `delegate*` cast and any -array (`MdAssemblyRef[]`) marshal at zero cost. - -Conversion follows the typedef hierarchy: - -- **Implicit** widening from a specific type to the generic base (`MdAssembly` → `MdToken`). - Always safe — every `mdAssembly` is an `mdToken` at the C level — and lets specific tokens - flow naturally into APIs that accept the generic base (e.g. `GetCustomAttributeByName`). -- **Explicit** narrowing from the base to a specific type (`(MdAssembly)token`). The C side - can't enforce that the value really is the claimed kind — the runtime validates on use, not - at the cast site — so the cast must be opt-in. - -**Check the native header for the canonical validation primitives** before defining `IsNil` / -`IsValid` etc. on the wrapper. Encoding details often surface only in macros that don't show -up on a casual grep. For `mdToken` the encoding is `(TableType << 24) | Rid` with these -helpers in `corhdr.h`: - -| C macro / constant | What it really means | -|---|---| -| `TypeFromToken(tk) = tk & 0xff000000` | Table-type tag (high byte) → `CorTokenType` enum | -| `RidFromToken(tk) = tk & 0x00ffffff` | Row id (low 24 bits) | -| `IsNilToken(tk) = RidFromToken(tk) == 0` | Nil check — **row id half, not the whole value** | -| `mdAssemblyNil = mdtAssembly = 0x20000000` | Per-type nil is the table-type tag, **not 0** | +### `BSTR` — COM strings -A naive `IsNil => Value == 0` would silently misclassify a valid "no assembly in this scope" -return from `GetAssemblyFromScope` (which writes `0x20000000`) as a non-nil token. Mirror the -macro: `IsNil => Rid == 0`. +CsWin32-generated; extended in [`Windows/Win32/Foundation/BSTR.cs`](../../../src/Framework/Windows/Win32/Foundation/BSTR.cs) with `IDisposable`. `Dispose` calls `SysFreeString` and zeroes the field in place. **Scope every COM `BSTR` out-param with `using BSTR x = default;`** — no manual `try/finally + SysFreeString`: -Example: [`src/Tasks/AssemblyDependency/Metadata/Tokens.cs`](../../../src/Tasks/AssemblyDependency/Metadata/Tokens.cs) -defines `MdToken`, `MdAssembly`, `MdAssemblyRef`, `MdFile` with the implicit/explicit -conversions and exposes `Kind` (`CorTokenType`), `Rid`, `IsNil`, and `IsValid` on each. -The `CorTokenType` enum itself lives in -[`CorTokenType.cs`](../../../src/Tasks/AssemblyDependency/Metadata/CorTokenType.cs). +```csharp +using BSTR versionBstr = default; +if (instance->GetInstallationVersion(&versionBstr).Failed) return false; +string version = versionBstr.ToString(); +// SysFreeString runs at scope exit. +``` + +In-place-only: `BSTR.Dispose` writes through `Unsafe.AsRef(in this)`, so `using` must be on the storage location, not on a method-returned copy. Same rule as `ComScope`. + +### `AgileComPointer` — fields outliving a method + +Finalizable managed class. Use whenever a COM pointer is stored in a class field (the only legal way — raw `T*` fields are forbidden, see top of section). + +- Registers in the Global Interface Table (thread-agile); finalizer releases if `Dispose` is missed. +- Access via `using ComScope scope = agile.GetInterface();` then `scope.Pointer->Method(...)`. `GetInterface()` round-trips through the GIT — hoist a single scope to the top of a method when several calls share it. +- Constructor `takeOwnership`: pass `false` when the raw pointer is also held by a `ComScope` that will Release (GIT AddRefs independently); pass `true` only when no other code path will Release. +- Dispose via the owner's `Dispose` / `DisposeManagedResources` (`AgileComPointer` is managed, not unmanaged — its finalizer is the safety net, not the primary cleanup). ## Activation ```csharp -// Via ComClassFactory (AOT-compatible) +// AOT-compatible if (ComClassFactory.TryCreate(IWbemLocator.CLSID, out var factory, out HRESULT hr)) using ComScope instance = factory.TryCreateInstance(out hr); -// Via CoCreateInstance — use IID.Get() for the IID +// CoCreateInstance — IID.Get(), not &localGuid Guid clsid = IWbemLocator.CLSID; using ComScope locator = new(); hr = PInvoke.CoCreateInstance(&clsid, null, CLSCTX.CLSCTX_INPROC_SERVER, IID.Get(), locator); ``` -**Key points:** -- Use `IID.Get()` — do not take `&localGuid` -- Initialize `ComScope` with `new()`. It implicitly converts to `T**` / `void**` output parameters - ## Error-Handling Parity When Migrating -Struct-based COM returns raw `HRESULT`; `[ComImport]` and built-in activation threw automatically. Preserving the old throw-vs-return behavior is part of the migration. +Struct-based COM returns raw `HRESULT`; `[ComImport]` and built-in activation threw automatically. Preserve the old throw-vs-return at each call site: -| Old shape | Threw on failure via | Migrated shape | +| Old shape | Threw via | Migrated shape | |---|---|---| -| `new SomeCoClass()` | built-in interop activation | `PInvoke.CoCreateInstance(...).ThrowOnFailure()` | -| `(IFoo)rcw` cast | `InvalidCastException` on QI failure | `QueryInterface(&iid, scope).ThrowOnFailure()` | -| `[ComImport]` method (default `PreserveSig=false`) | marshaller throws on `FAILED(hr)` | `.ThrowOnFailure()` at the call site | +| `new SomeCoClass()` | built-in activation | `PInvoke.CoCreateInstance(...).ThrowOnFailure()` | +| `(IFoo)rcw` cast | `InvalidCastException` on QI | `QueryInterface(&iid, scope).ThrowOnFailure()` | +| `[ComImport]` method, default `PreserveSig=false` | marshaller throws on `FAILED(hr)` | `.ThrowOnFailure()` at the call site | | `[ComImport]` method with `[PreserveSig]` | caller inspects return | mirror the existing `hr` branch — don't start throwing | -Factory-method exception: when the old code's contract was "return null for invalid input" (e.g. `Create(path)`), keep null-return only for the operation that legitimately rejects input; environment-level failures (`CoCreateInstance`, QI for guaranteed-implemented interfaces) still throw. Example: [`MetadataReader.cs`](../../../src/Tasks/ManifestUtil/MetadataReader.cs) throws on activation and QI, returns null on `OpenScope` failure. +**Factory-method exception:** when the old contract was "return null for invalid input" (e.g. `Create(path)`), keep null-return only for the legitimate-rejection path; activation / QI failures still throw. See [`MetadataReader.cs`](../../../src/Tasks/ManifestUtil/MetadataReader.cs) (throws on activation/QI, returns null on `OpenScope`). -## IComIID Polyfill for net472 +**Don't throw when the caller swallows it.** If the top-level consumer wraps the whole chain in `catch (COMException) { }`, inner helpers must return `default` / `false` / empty instead of constructing a `COMException` only to have it discarded. Throwing here allocates the exception, walks the stack, and hides the HRESULT for no benefit. See [`VisualStudioLocationHelper.AcquireSetupConfiguration2`](../../../src/Framework/VisualStudioLocationHelper.cs). -CsWin32 emits `IComIID` (with static-abstract `Guid`) and attaches it to every generated COM struct **only on .NET 7+**. On older targets (net472, netstandard2.0): +## Strongly-typed handle / token wrappers -- The `IComIID` interface itself is missing — provide an instance-based version at `src/Framework/Polyfills/IComIID.cs`. -- Generated COM structs do not have `IComIID` in their base list — provide a partial struct that adds it. +When the native side `typedef`s a primitive into a family of "same shape, different meaning" aliases (`corhdr.h`'s `typedef ULONG32 mdToken; typedef mdToken mdAssembly; ...`), mirror that hierarchy with distinct `readonly struct` wrappers holding the single underlying primitive — same pattern as CsWin32's `HANDLE` / `HWND` / `HMODULE`. Stays blittable; `delegate*` casts and arrays marshal at zero cost. -This is a **known case that always requires polyfilling** for .NET Framework support. Pattern in [`src/Framework/Polyfills/IComIIDPolyfills.cs`](../../../src/Framework/Polyfills/IComIIDPolyfills.cs): +Conversions follow the typedef hierarchy: **implicit** widening to the base (`MdAssembly` → `MdToken`, always safe), **explicit** narrowing (`(MdAssembly)token`, opt-in because the C side can't enforce the kind at the cast site). -```csharp -namespace Windows.Win32.System.Com; - -internal partial struct IRunningObjectTable : IComIID -{ - readonly Guid IComIID.Guid => IID_Guid; // CsWin32-emitted field, always present -} -``` - -When a new CsWin32-generated COM type is used through `ComScope`, add a partial entry to that file. Modeled after [winforms `IDataObject.cs`](https://github.com/dotnet/winforms/blob/main/src/System.Private.Windows.Core/src/Framework/Windows/Win32/System/Com/IDataObject.cs). - -Both polyfill files are gated with `#if !NET` so they compile on net472/netstandard2.0 and become empty on .NET (where CsWin32 emits its own static-abstract `IComIID` and attaches it to generated structs). - -For manual COM structs (WMI, Setup Configuration, etc.) that already use the static-abstract `IComIID` form, the polyfill approach won't compile on net472 — those structs stay .NET-only via ``. +**Check the native header for the canonical validation primitives** before defining `IsNil` / `IsValid`. Encoding details often hide in macros that don't grep cleanly. For `mdToken` the encoding is `(TableType << 24) | Rid`: -## Lifetime & Access - -**A raw `T*` (where `T` is a COM struct) must never appear as a field of a non-`ref` type.** Allowed locations for a raw `T*`: +| C macro | Meaning | +|---|---| +| `TypeFromToken(tk) = tk & 0xff000000` | Table-type tag (high byte) → `CorTokenType` | +| `RidFromToken(tk) = tk & 0x00ffffff` | Row id (low 24 bits) | +| `IsNilToken(tk) = RidFromToken(tk) == 0` | Nil check — **rid half, not the whole value** | +| `mdAssemblyNil = mdtAssembly = 0x20000000` | Per-type nil is the table-type tag, **not 0** | -- locals inside an `unsafe` method, -- parameters, -- fields of a `ref struct` (whose lifetime is statically bounded to a stack frame). +A naive `IsNil => Value == 0` would misclassify the `0x20000000` "no assembly in this scope" return from `GetAssemblyFromScope`. Mirror the macro: `IsNil => Rid == 0`. Reference: [`Tokens.cs`](../../../src/Tasks/AssemblyDependency/Metadata/Tokens.cs), [`CorTokenType.cs`](../../../src/Tasks/AssemblyDependency/Metadata/CorTokenType.cs). -Anywhere else — instance fields of a `class` or non-`ref` `struct`, including `internal`/`private` ones — use `AgileComPointer`. A raw pointer field in a managed object is an apartment-agility hazard (the field can be observed from any thread, but the underlying interface may be apartment-bound) and leaks the ref count whenever the owner is finalized without `Dispose`. +## IComIID Polyfill for net472 -- `ComScope` — `ref struct`, use with `using`. Releases on dispose. **The preferred way to scope any COM pointer that doesn't survive the current method**, including transient pointers received from `CoCreateInstance`, `QueryInterface`, `OpenScope`, factory methods, etc. - - **Receive output parameters directly into the `ComScope`.** `ComScope` implicitly converts to `T**` and `void**`, so pass the scope itself where the API expects a `T** ppvObject` / `void** ppv`. The call writes into the scope and the `using` Releases on scope exit. No `T* local; ...->Method(&local); try {...} finally { Release(local); }` patterns. +CsWin32 emits `IComIID` (static-abstract `Guid`) and attaches it to generated COM structs **only on .NET 7+**. On net472 / netstandard2.0: - ```csharp - Guid clsid = SomeStruct.CLSID; - Guid iid = IID.Get(); - using ComScope scope = new(); - PInvoke.CoCreateInstance(&clsid, null, CLSCTX.CLSCTX_INPROC_SERVER, &iid, scope).ThrowOnFailure(); - scope.Pointer->DoThing(...); +- The interface is provided instance-based at [`src/Framework/Polyfills/IComIID.cs`](../../../src/Framework/Polyfills/IComIID.cs). +- Generated structs do **not** carry `IComIID` in their base list — add a partial in [`src/Framework/Polyfills/IComIIDPolyfills.cs`](../../../src/Framework/Polyfills/IComIIDPolyfills.cs): - using ComScope other = new(); - Guid otherIid = IOther.IID_IOther; - scope.Pointer->QueryInterface(&otherIid, other).ThrowOnFailure(); - ``` - - Access methods via `scope.Pointer->Method(...)`. Check for null with `scope.IsNull`. + ```csharp + namespace Windows.Win32.System.Com; + internal partial struct IRunningObjectTable : IComIID + { + readonly Guid IComIID.Guid => IID_Guid; // CsWin32-emitted field + } + ``` -- `AgileComPointer` — finalizable managed class. Use for **every COM pointer that outlives a single method call**, i.e. anything stored in a class field. - - Registers in the Global Interface Table (thread-agile) and releases via the finalizer if `Dispose` is missed. - - Access via `using ComScope scope = agile.GetInterface();` then `scope.Pointer->Method(...)`. Each `GetInterface()` round-trips through the GIT, so hoist a single scope to the top of a method when several calls share it. - - **Constructor `takeOwnership`**: pass `false` when the raw pointer was just received into a `ComScope` that will Release on dispose — the GIT registration AddRefs independently, so two owners is correct. Pass `true` only when no other code path will Release the raw pointer (e.g. handing off a pointer that has no `ComScope` wrapper). - - Dispose via the owner's `Dispose` / `DisposeManagedResources` (`AgileComPointer` is a managed object, not an unmanaged resource — it has its own finalizer). +Both files gated `#if !NET`. When you start using a new CsWin32-generated COM type through `ComScope`, add a partial. Manual structs (WMI, Setup Configuration) that already use the static-abstract form stay .NET-only via `` — the polyfill won't compile against them. ## File Organization | Location | Contents | -|----------|----------| -| `src/Framework/Windows/Win32/System/Com/` | `ComScope.cs`, `ComClassFactory.cs`, `AgileComPointer.cs`, `GlobalInterfaceTable.cs` | +|---|---| +| `src/Framework/Windows/Win32/System/Com/` | `ComScope`, `ComClassFactory`, `AgileComPointer`, `GlobalInterfaceTable` | | `src/Framework/Windows/Win32/IID.cs` | Generic IID lookup | -| `src/Framework/Utilities/Wmi/` | Manual WMI structs (.NET-only — use static-abstract `IComIID`) | -| `src/Framework/Polyfills/IComIID.cs` | net472/netstandard2.0 instance-based `IComIID` polyfill (`#if !NET`) | -| `src/Framework/Polyfills/IComIIDPolyfills.cs` | Per-struct partials attaching `IComIID` to CsWin32-generated COM types on net472/netstandard2.0 | +| `src/Framework/Polyfills/IComIID*.cs` | net472 / netstandard2.0 polyfills (`#if !NET`) | +| `src/Framework/Utilities/Wmi/`, `src/Framework/Shared/VisualStudio/`, `src/Tasks/AssemblyDependency/Fusion/`, `src/Tasks/AssemblyDependency/Metadata/`, `src/Tasks/TypeLib/` | Manual COM struct interfaces — own folder per API surface | ## CS3016 CLS Compliance -CsWin32 COM structs trigger CS3016 under `[assembly: CLSCompliant(true)]`. Handled via `[CLSCompliant(false)]` partial declarations in `GeneratedInteropClsCompliance.cs`. CS3019 warnings suppressed in `.editorconfig` for `{**/Windows/**/*.cs}` — do not add per-file suppressions. See https://github.com/dotnet/roslyn/issues/68526. \ No newline at end of file +CsWin32 COM structs trigger CS3016 under `[assembly: CLSCompliant(true)]`. Handled via `[CLSCompliant(false)]` partial declarations in `GeneratedInteropClsCompliance.cs`; CS3019 suppressed in `.editorconfig` for `{**/Windows/**/*.cs}`. Don't add per-file suppressions. See https://github.com/dotnet/roslyn/issues/68526. diff --git a/src/Build.UnitTests/BackEnd/TargetUpToDateChecker_Tests.cs b/src/Build.UnitTests/BackEnd/TargetUpToDateChecker_Tests.cs index 8fee963e09b..6cb71ec69a0 100644 --- a/src/Build.UnitTests/BackEnd/TargetUpToDateChecker_Tests.cs +++ b/src/Build.UnitTests/BackEnd/TargetUpToDateChecker_Tests.cs @@ -16,6 +16,8 @@ using Microsoft.Win32.SafeHandles; using Xunit; #if FEATURE_WINDOWSINTEROP +using Windows.Win32; +using Windows.Win32.Foundation; using Windows.Win32.Storage.FileSystem; #endif @@ -1005,26 +1007,36 @@ public void NewSymlinkNewDestinationIsNotUpToDate() expectedOutOfDate: true); } - [DllImport("kernel32.dll")] - [return: MarshalAs(UnmanagedType.Bool)] [SupportedOSPlatform("windows6.1")] - private static extern bool CreateSymbolicLink(string lpSymlinkFileName, string lpTargetFileName, UInt32 dwFlags); + private static unsafe bool CreateSymbolicLink(string lpSymlinkFileName, string lpTargetFileName, uint dwFlags) + { + fixed (char* pSym = lpSymlinkFileName) + { + fixed (char* pTarget = lpTargetFileName) + { + return PInvoke.CreateSymbolicLink(new PCWSTR(pSym), new PCWSTR(pTarget), (SYMBOLIC_LINK_FLAGS)dwFlags) != 0; + } + } + } - [DllImport("kernel32.dll", SetLastError = true)] [SupportedOSPlatform("windows6.1")] - private static extern bool SetFileTime(SafeFileHandle hFile, ref long creationTime, - ref long lastAccessTime, ref long lastWriteTime); + private static unsafe SafeFileHandle CreateFileForSymlink(string path) + { + HANDLE h; + fixed (char* pPath = path) + { + h = PInvoke.CreateFile( + new PCWSTR(pPath), + (uint)FILE_ACCESS_RIGHTS.FILE_GENERIC_READ | 0x100 /* FILE_WRITE_ATTRIBUTES */, + FILE_SHARE_MODE.FILE_SHARE_READ, + lpSecurityAttributes: null, + FILE_CREATION_DISPOSITION.OPEN_EXISTING, + FILE_FLAGS_AND_ATTRIBUTES.FILE_ATTRIBUTE_NORMAL | FILE_FLAGS_AND_ATTRIBUTES.FILE_FLAG_OPEN_REPARSE_POINT, + hTemplateFile: default); + } - [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true, EntryPoint = "CreateFileW")] - [SupportedOSPlatform("windows6.1")] - private static extern SafeFileHandle CreateFileForSymlink( - string lpFileName, - uint dwDesiredAccess, - uint dwShareMode, - IntPtr lpSecurityAttributes, - uint dwCreationDisposition, - uint dwFlagsAndAttributes, - IntPtr hTemplateFile); + return new SafeFileHandle((IntPtr)h.Value, ownsHandle: true); + } [SupportedOSPlatform("windows6.1")] private void SimpleSymlinkInputCheck(DateTime symlinkWriteTime, DateTime targetWriteTime, @@ -1052,14 +1064,7 @@ private void SimpleSymlinkInputCheck(DateTime symlinkWriteTime, DateTime targetW // File.SetLastWriteTime on the symlink sets the target write time, // so set the symlink's write time the hard way - using (SafeFileHandle handle = CreateFileForSymlink( - inputSymlink, - (uint)FILE_ACCESS_RIGHTS.FILE_GENERIC_READ | 0x100 /* FILE_WRITE_ATTRIBUTES */, - (uint)FILE_SHARE_MODE.FILE_SHARE_READ, - IntPtr.Zero, - (uint)FILE_CREATION_DISPOSITION.OPEN_EXISTING, - (uint)(FILE_FLAGS_AND_ATTRIBUTES.FILE_ATTRIBUTE_NORMAL | FILE_FLAGS_AND_ATTRIBUTES.FILE_FLAG_OPEN_REPARSE_POINT), - IntPtr.Zero)) + using (SafeFileHandle handle = CreateFileForSymlink(inputSymlink)) { if (handle.IsInvalid) { @@ -1067,9 +1072,13 @@ private void SimpleSymlinkInputCheck(DateTime symlinkWriteTime, DateTime targetW } long symlinkWriteTimeTicks = symlinkWriteTime.ToFileTimeUtc(); + System.Runtime.InteropServices.ComTypes.FILETIME ft = new() + { + dwLowDateTime = (int)(symlinkWriteTimeTicks & 0xFFFFFFFF), + dwHighDateTime = (int)(symlinkWriteTimeTicks >> 32), + }; - if (!SetFileTime(handle, ref symlinkWriteTimeTicks, ref symlinkWriteTimeTicks, - ref symlinkWriteTimeTicks)) + if (!PInvoke.SetFileTime((HANDLE)handle.DangerousGetHandle(), ft, ft, ft)) { Marshal.ThrowExceptionForHR(Marshal.GetHRForLastWin32Error()); } diff --git a/src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj b/src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj index 386ce335f62..48b02992818 100644 --- a/src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj +++ b/src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj @@ -17,6 +17,7 @@ $(NoWarn);MSB3270;CS0436 ..\Shared\UnitTests\App.config + true diff --git a/src/Build/BackEnd/Node/NativeMethods.cs b/src/Build/BackEnd/Node/NativeMethods.cs deleted file mode 100644 index 31054f8cbd2..00000000000 --- a/src/Build/BackEnd/Node/NativeMethods.cs +++ /dev/null @@ -1,229 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System; -using System.Runtime.InteropServices; - -namespace Microsoft.Build.BackEnd -{ - /// - /// Native methods used by the backend. This was copied from the oldOM so we can make it stylecop compliant and allow - /// easier deletion of the native code in the old OM - /// - internal static class NativeMethods - { - /// - /// Null Pointer - /// - internal static readonly IntPtr NullPtr = IntPtr.Zero; - - /// - /// Invalid Handle - /// - internal static readonly IntPtr InvalidHandle = new IntPtr(-1); - - /// - /// Start the process with a normal priority class - /// - internal const uint NORMALPRIORITYCLASS = 0x0020; - - /// - /// Do not create a window - /// - internal const uint CREATENOWINDOW = 0x08000000; - - /// - /// Use the standard handles - /// - internal const Int32 STARTFUSESTDHANDLES = 0x00000100; - - /// - /// Create a new console. - /// - internal const Int32 CREATE_NEW_CONSOLE = 0x00000010; - - /// - /// Indicates that the environment block uses Unicode characters. - /// - internal const uint CREATE_UNICODE_ENVIRONMENT = 0x00000400; - - /// - /// Create a new process - /// - [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)] - [return: MarshalAs(UnmanagedType.Bool)] - internal static extern bool CreateProcess( - string lpApplicationName, - string lpCommandLine, - ref SECURITY_ATTRIBUTES lpProcessAttributes, - ref SECURITY_ATTRIBUTES lpThreadAttributes, - [In, MarshalAs(UnmanagedType.Bool)] - bool bInheritHandles, - uint dwCreationFlags, - IntPtr lpEnvironment, - string lpCurrentDirectory, - [In] ref STARTUP_INFO lpStartupInfo, - out PROCESS_INFORMATION lpProcessInformation); - - /// - /// Structure that contains the startupinfo - /// Represents STARTUP_INFO in win32 - /// - [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] - internal struct STARTUP_INFO - { - /// - /// The size of the structure, in bytes. - /// - internal Int32 cb; - - /// - /// Reserved; must be NULL - /// - internal string lpReserved; - - /// - /// The name of the desktop, or the name of both the desktop and window station for this process. - /// A backslash in the string indicates that the string includes both the desktop and window station names - /// - internal string lpDesktop; - - /// - /// For console processes, this is the title displayed in the title bar if a new console window is created. - /// If NULL, the name of the executable file is used as the window title instead. - /// This parameter must be NULL for GUI or console processes that do not create a new console window - /// - internal string lpTitle; - - /// - /// If dwFlags specifies STARTF_USEPOSITION, this member is the x offset of the upper left corner of a window if a new window is created, in pixels. Otherwise, this member is ignored - /// - internal Int32 dwX; - - /// - /// If dwFlags specifies STARTF_USEPOSITION, this member is the y offset of the upper left corner of a window if a new window is created, in pixels. Otherwise, this member is ignored. - /// - internal Int32 dwY; - - /// - /// If dwFlags specifies STARTF_USESIZE, this member is the width of the window if a new window is created, in pixels. Otherwise, this member is ignored. - /// - internal Int32 dwXSize; - - /// - /// If dwFlags specifies STARTF_USESIZE, this member is the height of the window if a new window is created, in pixels. Otherwise, this member is ignored. - /// - internal Int32 dwYSize; - - /// - /// If dwFlags specifies STARTF_USECOUNTCHARS, if a new console window is created in a console process, this member specifies the screen buffer width, in character columns. Otherwise, this member is ignored. - /// - internal Int32 dwXCountChars; - - /// - /// If dwFlags specifies STARTF_USECOUNTCHARS, if a new console window is created in a console process, this member specifies the screen buffer height, in character rows. Otherwise, this member is ignored.dwFillAttribute - /// - internal Int32 dwYCountChars; - - /// - /// If dwFlags specifies STARTF_USEFILLATTRIBUTE, this member is the initial text and background colors if a new console window is created in a console application. Otherwise, this member is ignored. - /// - internal Int32 dwFillAttribute; - - /// - /// A bit field that determines whether certain STARTUPINFO members are used when the process creates a window - /// - internal Int32 dwFlags; - - /// - /// If dwFlags specifies STARTF_USESHOWWINDOW, this member can be any of the SW_ constants defined in Winuser.h. Otherwise, this member is ignored. - /// - internal Int16 wShowWindow; - - /// - /// Reserved for use by the C Run-time; must be zero. - /// - internal Int16 cbReserved2; - - /// - /// Reserved for use by the C Run-time; must be NULL. - /// - internal IntPtr lpReserved2; - - /// - /// If dwFlags specifies STARTF_USESTDHANDLES, this member is the standard input handle for the process. Otherwise, this member is ignored and the default for standard input is the keyboard buffer. - /// - internal IntPtr hStdInput; - - /// - /// If dwFlags specifies STARTF_USESTDHANDLES, this member is the standard output handle for the process. Otherwise, this member is ignored and the default for standard output is the console window's buffer. - /// - internal IntPtr hStdOutput; - - /// - /// If dwFlags specifies STARTF_USESTDHANDLES, this member is the standard error handle for the process. Otherwise, this member is ignored and the default for standard error is the console window's buffer. - /// - internal IntPtr hStdError; - } - - /// - /// Structure to contain security attributes from the create process call represents - /// SECURITY_ATTRIBUTE in win32 - /// - [StructLayout(LayoutKind.Sequential)] - internal struct SECURITY_ATTRIBUTES - { - /// - /// The size, in bytes, of this structure. Set this value to the size of the SECURITY_ATTRIBUTES structure - /// - public int nLength; - - /// - /// A pointer to a security descriptor for the object that controls the sharing of it. - /// If NULL is specified for this member, the object is assigned the default security descriptor of the calling process. - /// This is not the same as granting access to everyone by assigning a NULL discretionary access control list (DACL). - /// The default security descriptor is based on the default DACL of the access token belonging to the calling process. - /// By default, the default DACL in the access token of a process allows access only to the user represented by the access token. - /// If other users must access the object, you can either create a security descriptor with the appropriate access, - /// or add ACEs to the DACL that grants access to a group of users. - /// - public IntPtr lpSecurityDescriptor; - - /// - /// A Boolean value that specifies whether the returned handle is inherited when a new process is created. - /// If this member is TRUE, the new process inherits the handle. - /// - public int bInheritHandle; - } - - /// - /// Process information from the create process call - /// Represents PROCESS_INFORMATION in win32 - /// - [StructLayout(LayoutKind.Sequential)] - internal struct PROCESS_INFORMATION - { - /// - /// A handle to the newly created process. The handle is used to specify the process in all functions that perform operations on the process object. - /// - public IntPtr hProcess; - - /// - /// A handle to the primary thread of the newly created process. The handle is used to specify the thread in all functions that perform operations on the thread object - /// - public IntPtr hThread; - - /// - /// A value that can be used to identify a process. - /// The value is valid from the time the process is created until all handles to the process are closed and - /// the process object is freed; at this point, the identifier may be reused. - /// - public int dwProcessId; - - /// - /// A value that can be used to identify a thread. The value is valid from the time the thread is created until all handles to the thread are closed and the thread object is freed; at this point, the identifier may be reused. - /// - public int dwThreadId; - } - } -} diff --git a/src/Build/Microsoft.Build.csproj b/src/Build/Microsoft.Build.csproj index d851eee77b8..af914f03cdd 100644 --- a/src/Build/Microsoft.Build.csproj +++ b/src/Build/Microsoft.Build.csproj @@ -65,7 +65,6 @@ - diff --git a/src/Framework.UnitTests/FileSystem/WindowsNative_Tests.cs b/src/Framework.UnitTests/FileSystem/WindowsNative_Tests.cs new file mode 100644 index 00000000000..3d3045671ab --- /dev/null +++ b/src/Framework.UnitTests/FileSystem/WindowsNative_Tests.cs @@ -0,0 +1,153 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.IO; +using System.Linq; +using System.Runtime.Versioning; +using Microsoft.Build.Shared.FileSystem; +using Microsoft.Build.UnitTests.Shared; +using Shouldly; +using Xunit; + +#nullable disable + +namespace Microsoft.Build.UnitTests +{ + /// + /// Covers 's migration from `[DllImport]`-based + /// FindFirstFileW / FindNextFileW / FindClose / PathMatchSpecExW + /// to CsWin32 (PInvoke.FindFirstFile / FindNextFile / FindClose / + /// PathMatchSpecEx), plus the Win32FindData adapter built from the + /// blittable WIN32_FIND_DATAW. + /// + [SupportedOSPlatform("windows6.0.6000")] + public sealed class WindowsNative_Tests : IDisposable + { + private readonly string _tempDir; + + public WindowsNative_Tests() + { + _tempDir = Path.Combine(Path.GetTempPath(), "msb-WindowsNative_Tests-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(_tempDir); + } + + public void Dispose() + { + try + { + if (Directory.Exists(_tempDir)) + { + Directory.Delete(_tempDir, recursive: true); + } + } + catch + { + // Best-effort: leave it to the temp folder cleanup if something locks a file. + } + } + + [WindowsOnlyFact] + public void FindFirstFile_ReturnsExpectedFileName() + { + // Single concrete file looked up by its exact name. CFileName must come back + // through the WIN32_FIND_DATAW.cFileName fixed buffer -> managed string adapter. + string filePath = Path.Combine(_tempDir, "alpha.txt"); + File.WriteAllText(filePath, "test"); + + using SafeFindFileHandle handle = WindowsNative.FindFirstFileW(filePath, out WindowsNative.Win32FindData data); + handle.IsInvalid.ShouldBeFalse(); + data.CFileName.ShouldBe("alpha.txt"); + } + + [WindowsOnlyFact] + public void FindNextFile_EnumeratesAllEntries() + { + // FindFirst + repeated FindNext should walk every file in the directory exactly once. + // "." and ".." come back too — exclude them, then compare to the canonical + // Directory.GetFiles enumeration of the same directory. + string[] names = ["a.txt", "b.txt", "c.txt"]; + foreach (string n in names) + { + File.WriteAllText(Path.Combine(_tempDir, n), n); + } + + using SafeFindFileHandle handle = WindowsNative.FindFirstFileW(Path.Combine(_tempDir, "*"), out WindowsNative.Win32FindData data); + handle.IsInvalid.ShouldBeFalse(); + + var found = new System.Collections.Generic.List(); + do + { + if (data.CFileName is not "." and not "..") + { + found.Add(data.CFileName); + } + } + while (WindowsNative.FindNextFileW(handle, out data)); + + found.OrderBy(s => s).ShouldBe(names.OrderBy(s => s)); + } + + [WindowsOnlyFact] + public void Win32FindData_AttributesAndTimes_MatchManagedAPIs() + { + // The adapter must preserve every field the callers depend on. Round-trip the + // attributes and last-write time against the standard managed file-system APIs. + string filePath = Path.Combine(_tempDir, "props.txt"); + File.WriteAllText(filePath, "x"); + File.SetAttributes(filePath, FileAttributes.ReadOnly | FileAttributes.Normal); + + using SafeFindFileHandle handle = WindowsNative.FindFirstFileW(filePath, out WindowsNative.Win32FindData data); + handle.IsInvalid.ShouldBeFalse(); + + File.GetAttributes(filePath).ShouldBe(data.DwFileAttributes); + + long fileTimeFromFindData = + ((long)(uint)data.FtLastWriteTime.dwHighDateTime << 32) + | (uint)data.FtLastWriteTime.dwLowDateTime; + DateTime.FromFileTimeUtc(fileTimeFromFindData) + .ShouldBe(File.GetLastWriteTimeUtc(filePath), TimeSpan.FromMilliseconds(10)); + + // nFileSizeHigh:nFileSizeLow is the 64-bit size. + ((long)data.NFileSizeHigh << 32 | data.NFileSizeLow).ShouldBe(1L); + } + + [WindowsOnlyFact] + public void FindFirstFile_NonExistentDirectory_ReturnsInvalidHandle() + { + // Bad path must surface as a SafeHandle whose IsInvalid is true (FindFirstFile + // returns INVALID_HANDLE_VALUE in this case). The SafeHandle must not throw on + // Dispose for an invalid handle either. + string bogus = Path.Combine(_tempDir, "does-not-exist", "*"); + using SafeFindFileHandle handle = WindowsNative.FindFirstFileW(bogus, out _); + handle.IsInvalid.ShouldBeTrue(); + } + + [WindowsOnlyFact] + public void PathMatchSpecExW_MatchAndNonMatch() + { + // Win32 PathMatchSpecEx returns S_OK (ErrorSuccess) on a match and S_FALSE + // on a non-match. The wrapper preserves that as the raw int. + WindowsNative.PathMatchSpecExW("foo.txt", "*.txt", WindowsNative.DwFlags.PmsfNormal) + .ShouldBe(WindowsNative.ErrorSuccess); + WindowsNative.PathMatchSpecExW("foo.bin", "*.txt", WindowsNative.DwFlags.PmsfNormal) + .ShouldNotBe(WindowsNative.ErrorSuccess); + } + + [WindowsOnlyFact] + public void SafeFindFileHandle_Dispose_IsIdempotent() + { + // SafeFindFileHandle's ReleaseHandle calls PInvoke.FindClose. Double-dispose and + // implicit-dispose at scope exit must both be no-ops. + string filePath = Path.Combine(_tempDir, "z.txt"); + File.WriteAllText(filePath, "z"); + + SafeFindFileHandle handle = WindowsNative.FindFirstFileW(filePath, out _); + handle.IsInvalid.ShouldBeFalse(); + + handle.Dispose(); + Should.NotThrow(() => handle.Dispose()); + handle.IsClosed.ShouldBeTrue(); + } + } +} diff --git a/src/Framework.UnitTests/Microsoft.Build.Framework.UnitTests.csproj b/src/Framework.UnitTests/Microsoft.Build.Framework.UnitTests.csproj index b2196587455..5d140de8c2b 100644 --- a/src/Framework.UnitTests/Microsoft.Build.Framework.UnitTests.csproj +++ b/src/Framework.UnitTests/Microsoft.Build.Framework.UnitTests.csproj @@ -9,7 +9,6 @@ - diff --git a/src/Framework.UnitTests/VisualStudioLocationHelper_Tests.cs b/src/Framework.UnitTests/VisualStudioLocationHelper_Tests.cs new file mode 100644 index 00000000000..7b7e7aa48d4 --- /dev/null +++ b/src/Framework.UnitTests/VisualStudioLocationHelper_Tests.cs @@ -0,0 +1,127 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.IO; +using System.Linq; +using System.Runtime.Versioning; +using Microsoft.Build.Shared; +using Microsoft.Build.UnitTests.Shared; +using Shouldly; +using Xunit; + +#nullable disable + +namespace Microsoft.Build.UnitTests +{ + /// + /// Baseline behavioural contract for — the helper + /// that queries the Visual Studio Setup Configuration COM API + /// (Microsoft.VisualStudio.Setup.Configuration.Native.dll) for installed VS + /// instances on Windows .NET Framework. + /// + /// + /// These tests intentionally make no assumption about whether VS is installed on the + /// machine they run on — CI agents may have zero, one, or many VS instances. The contract + /// asserted here is: + /// + /// The call itself never throws (any underlying COMException / + /// DllNotFoundException is swallowed by design). + /// The returned list is non-null. + /// Every returned has a non-empty Name, + /// a non-empty Path, and a Version at major 15 or higher (the + /// ISetupConfiguration API does not surface VS < 15 instances). + /// Repeated calls return an equivalent set of instances (the helper is a pure + /// read of installer state; there is no per-call side effect). + /// + /// Tests run only on .NET Framework with VS Setup Configuration enabled — the + /// FEATURE_VISUALSTUDIOSETUP gate inside the helper means the non-net472 build + /// returns an empty list unconditionally and there is no native interop to exercise. + /// + public sealed class VisualStudioLocationHelper_Tests + { + [WindowsOnlyFact] + public void GetInstances_ReturnsNonNullList() + { + // The helper's contract is that it never throws and never returns null. An empty + // list is the legitimate "no VS installed" outcome — that's still a valid result, + // not an error. + var instances = VisualStudioLocationHelper.GetInstances(); + instances.ShouldNotBeNull(); + } + + [WindowsOnlyFact] + public void GetInstances_DoesNotThrowOnRepeatedCalls() + { + // The underlying ISetupConfiguration COM API is reentrant. The helper must be + // safe to invoke multiple times in the same process and should return the same + // set of installation paths each time. We compare paths because the same physical + // install produces a fresh COM enumeration object on each call. + var first = VisualStudioLocationHelper.GetInstances(); + var second = VisualStudioLocationHelper.GetInstances(); + + first.Select(i => i.Path).OrderBy(p => p) + .ShouldBe(second.Select(i => i.Path).OrderBy(p => p)); + } + + [WindowsOnlyFact] + public void GetInstances_AllResults_HaveValidShape() + { + // For every instance the helper surfaces, the three carried fields (Name, Path, + // Version) must be populated. Path doesn't have to exist on disk in the tests's + // sandbox (the user might have moved the install), but it must be a non-empty + // string. Version must be >= 15.0 because ISetupConfiguration was introduced + // for "VS 15" (Visual Studio 2017) and never reports earlier products. + var instances = VisualStudioLocationHelper.GetInstances(); + + foreach (var instance in instances) + { + instance.Name.ShouldNotBeNullOrEmpty(); + instance.Path.ShouldNotBeNullOrEmpty(); + instance.Version.ShouldNotBeNull(); + instance.Version.Major.ShouldBeGreaterThanOrEqualTo(15); + } + } + + [WindowsOnlyFact] + public void GetInstances_PathsAreUnique() + { + // Each install lives at its own physical location. The helper must not duplicate + // an instance — if the same path shows up twice that would indicate the + // enumeration didn't terminate or the same ISetupInstance was added twice. + var paths = VisualStudioLocationHelper.GetInstances().Select(i => i.Path).ToList(); + paths.ShouldBeUnique(); + } + + [WindowsOnlyFact] + public void GetInstances_ExistingPaths_AreAbsoluteDirectories() + { + // Real installations report rooted directories. If the path actually exists on + // disk it must be a directory (not a file). Paths that don't exist are still + // allowed (the user may have manually deleted an install without uninstalling). + foreach (var instance in VisualStudioLocationHelper.GetInstances()) + { + Path.IsPathRooted(instance.Path).ShouldBeTrue($"VS install path '{instance.Path}' is not rooted"); + if (Directory.Exists(instance.Path)) + { + // Sanity: when present, it's a directory, not a file masquerading as one. + File.Exists(instance.Path).ShouldBeFalse(); + } + } + } + + [Fact] + public void VisualStudioInstance_Constructor_AssignsAllProperties() + { + // Trivial data-shape check on the value-type wrapper. The constructor is the only + // surface code on this class (no validation, no normalization) — verifying it + // here guards against an accidental change to property order or copy-paste error. + var v = new Version(17, 8, 0, 0); + var instance = new VisualStudioInstance("VS 2022 Enterprise", @"C:\Program Files\Microsoft Visual Studio\2022\Enterprise", v); + + instance.Name.ShouldBe("VS 2022 Enterprise"); + instance.Path.ShouldBe(@"C:\Program Files\Microsoft Visual Studio\2022\Enterprise"); + instance.Version.ShouldBe(v); + } + } +} diff --git a/src/Framework/FileSystem/SafeFileHandle.cs b/src/Framework/FileSystem/SafeFileHandle.cs index 1ba5c2dc3e9..313adfd37ba 100644 --- a/src/Framework/FileSystem/SafeFileHandle.cs +++ b/src/Framework/FileSystem/SafeFileHandle.cs @@ -1,28 +1,32 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System; using Microsoft.Win32.SafeHandles; - +using Windows.Win32; +using Windows.Win32.Foundation; namespace Microsoft.Build.Shared.FileSystem { /// - /// Handle for a volume iteration as returned by WindowsNative.FindFirstVolumeW /> + /// Handle for a FindFirstFileW enumeration. Owns the FindClose call on Dispose. /// internal sealed class SafeFindFileHandle : SafeHandleZeroOrMinusOneIsInvalid { - /// - /// Private constructor for the PInvoke marshaller. - /// - private SafeFindFileHandle() + // Default ctor is used internally by SetHandle below; we don't want the P/Invoke + // marshaller path here anymore since SafeFindFileHandle is constructed manually + // from the HANDLE returned by PInvoke.FindFirstFile. + internal SafeFindFileHandle(IntPtr handle) : base(ownsHandle: true) { + SetHandle(handle); } /// + [System.Runtime.Versioning.SupportedOSPlatform("windows5.1.2600")] protected override bool ReleaseHandle() { - return WindowsNative.FindClose(handle); + return PInvoke.FindClose(new HANDLE(handle)); } } } diff --git a/src/Framework/FileSystem/WindowsNative.cs b/src/Framework/FileSystem/WindowsNative.cs index d50e4d56870..e1dc5207251 100644 --- a/src/Framework/FileSystem/WindowsNative.cs +++ b/src/Framework/FileSystem/WindowsNative.cs @@ -8,13 +8,12 @@ using System.IO; using System.Runtime.InteropServices; - namespace Microsoft.Build.Shared.FileSystem { /// /// Native implementation of file system operations /// - internal static class WindowsNative + internal static unsafe class WindowsNative { /// /// Maximum path length. @@ -254,26 +253,57 @@ public struct Win32FindData } /// - [DllImport("kernel32.dll", SetLastError = true, CharSet = CharSet.Unicode)] - [SuppressMessage("Microsoft.Interoperability", "CA1401:PInvokesShouldNotBeVisible", Justification = "Needed for custom enumeration.")] - public static extern SafeFindFileHandle FindFirstFileW( - string lpFileName, - out Win32FindData lpFindFileData); + [System.Runtime.Versioning.SupportedOSPlatform("windows5.1.2600")] + public static SafeFindFileHandle FindFirstFileW(string lpFileName, out Win32FindData lpFindFileData) + { + Windows.Win32.Foundation.HANDLE h = Windows.Win32.PInvoke.FindFirstFile(lpFileName, out Windows.Win32.Storage.FileSystem.WIN32_FIND_DATAW raw); + lpFindFileData = ConvertFindData(in raw); + return new SafeFindFileHandle((nint)h.Value); + } /// - [DllImport("kernel32.dll", SetLastError = true, CharSet = CharSet.Unicode)] - [return: MarshalAs(UnmanagedType.Bool)] - [SuppressMessage("Microsoft.Interoperability", "CA1401:PInvokesShouldNotBeVisible", Justification = "Needed for custom enumeration.")] - public static extern bool FindNextFileW(SafeHandle hFindFile, out Win32FindData lpFindFileData); + [System.Runtime.Versioning.SupportedOSPlatform("windows5.1.2600")] + public static bool FindNextFileW(SafeHandle hFindFile, out Win32FindData lpFindFileData) + { + bool ok = Windows.Win32.PInvoke.FindNextFile(new Windows.Win32.Foundation.HANDLE((void*)hFindFile.DangerousGetHandle()), out var raw); + lpFindFileData = ConvertFindData(in raw); + return ok; + } /// - [DllImport("shlwapi.dll", CharSet = CharSet.Unicode)] - [SuppressMessage("Microsoft.Interoperability", "CA1401:PInvokesShouldNotBeVisible", Justification = "Needed for creating symlinks.")] - public static extern int PathMatchSpecExW([In] string pszFileParam, [In] string pszSpec, [In] int flags); + [System.Runtime.Versioning.SupportedOSPlatform("windows6.0.6000")] + public static int PathMatchSpecExW(string pszFileParam, string pszSpec, int flags) + { + return Windows.Win32.PInvoke.PathMatchSpecEx(pszFileParam, pszSpec, (uint)flags); + } /// - [DllImport("kernel32.dll", SetLastError = true)] - [return: MarshalAs(UnmanagedType.Bool)] - internal static extern bool FindClose(IntPtr findFileHandle); + [System.Runtime.Versioning.SupportedOSPlatform("windows5.1.2600")] + internal static bool FindClose(IntPtr findFileHandle) + { + return Windows.Win32.PInvoke.FindClose(new Windows.Win32.Foundation.HANDLE((void*)findFileHandle)); + } + + // Adapter from CsWin32's WIN32_FIND_DATAW (fixed-size char buffers, no string marshalling) + // to the legacy Win32FindData shape used by callers that read CFileName as a managed string. + private static Win32FindData ConvertFindData(in Windows.Win32.Storage.FileSystem.WIN32_FIND_DATAW raw) + { + fixed (Windows.Win32.Storage.FileSystem.WIN32_FIND_DATAW* p = &raw) + { + return new Win32FindData + { + DwFileAttributes = (FileAttributes)p->dwFileAttributes, + FtCreationTime = p->ftCreationTime, + FtLastAccessTime = p->ftLastAccessTime, + FtLastWriteTime = p->ftLastWriteTime, + NFileSizeHigh = p->nFileSizeHigh, + NFileSizeLow = p->nFileSizeLow, + DwReserved0 = p->dwReserved0, + DwReserved1 = p->dwReserved1, + CFileName = new string((char*)&p->cFileName), + CAlternate = new string((char*)&p->cAlternateFileName), + }; + } + } } } diff --git a/src/Framework/Microsoft.Build.Framework.csproj b/src/Framework/Microsoft.Build.Framework.csproj index 92d39aa2e0b..fcac65ccc6f 100644 --- a/src/Framework/Microsoft.Build.Framework.csproj +++ b/src/Framework/Microsoft.Build.Framework.csproj @@ -43,7 +43,6 @@ - diff --git a/src/Framework/NativeMethods.txt b/src/Framework/NativeMethods.txt index b16268823b8..cf7494f6039 100644 --- a/src/Framework/NativeMethods.txt +++ b/src/Framework/NativeMethods.txt @@ -16,12 +16,13 @@ CreateItemMoniker CreatePipe CreateProcess CreateSymbolicLink +DebugCreate DEBUG_PROC_DESC_NO_MTS_PACKAGES DEBUG_PROC_DESC_NO_PATHS DEBUG_PROC_DESC_NO_SERVICES DEBUG_PROC_DESC_NO_SESSION_ID DEBUG_PROC_DESC_NO_USER_NAME -DebugCreate +DefineDosDevice EndUpdateResource EOLE_AUTHENTICATION_CAPABILITIES FACILITY_CODE @@ -31,6 +32,10 @@ FILE_FLAGS_AND_ATTRIBUTES FILE_MAP FILE_SHARE_MODE FILETIME +FindClose +FindFirstFile +FindNextFile +FindResource FreeEnvironmentStrings FreeEnvironmentStringsW FreeLibrary @@ -71,7 +76,11 @@ INVALID_FILE_ATTRIBUTES INVALID_HANDLE_VALUE IRunningObjectTable KNOWN_FOLDER_FLAG +LOAD_LIBRARY_FLAGS LoadLibrary +LoadLibraryEx +LoadResource +LockResource MapViewOfFile MAX_PATH MEMORYSTATUSEX @@ -80,6 +89,7 @@ MoveFileEx NtQueryInformationProcess OpenProcess PAGE_PROTECTION_FLAGS +PathMatchSpecEx PROCESS_ACCESS_RIGHTS PROCESS_BASIC_INFORMATION PROCESS_CREATION_FLAGS @@ -87,6 +97,8 @@ PROCESS_INFORMATION PropVariantClear PWSTR PCWSTR +PCSTR +QueryDosDevice ReadFile REGDB_E_CLASSNOTREG RM_APP_TYPE @@ -108,8 +120,10 @@ SECURITY_ATTRIBUTES SetConsoleMode SetCurrentDirectory SetEnvironmentVariable +SetFileTime SetThreadErrorMode SHGetKnownFolderPath +SizeofResource STARTUPINFOW STARTUPINFOW_FLAGS SYMBOLIC_LINK_FLAGS diff --git a/src/Framework/Shared/VisualStudio/IEnumSetupInstances.cs b/src/Framework/Shared/VisualStudio/IEnumSetupInstances.cs new file mode 100644 index 00000000000..bb60c1d162f --- /dev/null +++ b/src/Framework/Shared/VisualStudio/IEnumSetupInstances.cs @@ -0,0 +1,85 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#if FEATURE_WINDOWSINTEROP && FEATURE_VISUALSTUDIOSETUP + +using System; +#if NET +using System.Runtime.CompilerServices; +#endif +using Windows.Win32; +using Windows.Win32.Foundation; + +namespace Microsoft.Build.Shared.VisualStudio; + +/// +/// Standard COM enumerator over pointers. +/// +/// +/// Declared in Setup.Configuration.idl. IID = {6380BCFF-41D3-4B2E-8B2E-BF8A6810C848}. +/// +internal unsafe struct IEnumSetupInstances : IComIID +{ + public static readonly Guid IID_IEnumSetupInstances = new(0x6380BCFF, 0x41D3, 0x4B2E, 0x8B, 0x2E, 0xBF, 0x8A, 0x68, 0x10, 0xC8, 0x48); + +#if NET + static ref readonly Guid IComIID.Guid + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get => ref Unsafe.AsRef(in IID_IEnumSetupInstances); + } +#else + readonly Guid IComIID.Guid => IID_IEnumSetupInstances; +#endif + + private readonly void** _lpVtbl; + + // IUnknown methods (vtable indices 0-2) + + public HRESULT QueryInterface(Guid* riid, void** ppvObject) + { + fixed (IEnumSetupInstances* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[0])(pThis, riid, ppvObject); + } + } + + public uint AddRef() + { + fixed (IEnumSetupInstances* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[1])(pThis); + } + } + + public uint Release() + { + fixed (IEnumSetupInstances* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[2])(pThis); + } + } + + // IEnumSetupInstances vtable layout (indices 3-6): + // 3 = Next (ULONG celt, ISetupInstance** rgelt, ULONG* pceltFetched) <-- Used + // 4 = Skip (ULONG celt) + // 5 = Reset () + // 6 = Clone (out IEnumSetupInstances**) + + /// + /// Retrieve up to instances from the enumerator. Returns + /// when exactly items were + /// produced and when fewer (including zero) were + /// available. always reflects the actual count. + /// + public HRESULT Next(uint celt, ISetupInstance** rgelt, uint* pceltFetched) + { + fixed (IEnumSetupInstances* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[3])( + pThis, celt, rgelt, pceltFetched); + } + } +} + +#endif diff --git a/src/Framework/Shared/VisualStudio/ISetupConfiguration.cs b/src/Framework/Shared/VisualStudio/ISetupConfiguration.cs new file mode 100644 index 00000000000..3edc16b1721 --- /dev/null +++ b/src/Framework/Shared/VisualStudio/ISetupConfiguration.cs @@ -0,0 +1,66 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#if FEATURE_WINDOWSINTEROP && FEATURE_VISUALSTUDIOSETUP + +using System; +#if NET +using System.Runtime.CompilerServices; +#endif +using Windows.Win32; +using Windows.Win32.Foundation; + +namespace Microsoft.Build.Shared.VisualStudio; + +/// +/// Top-level Visual Studio Setup Configuration query interface (v1). MSBuild only needs +/// the IUnknown surface here so it can QI to ; the +/// v1-specific vtable slots (3–5) are intentionally not exposed. +/// +/// +/// Declared in Setup.Configuration.idl. IID = {42843719-DB4C-46C2-8E7C-64F1816EFD5B}. +/// +internal unsafe struct ISetupConfiguration : IComIID +{ + public static readonly Guid IID_ISetupConfiguration = new(0x42843719, 0xDB4C, 0x46C2, 0x8E, 0x7C, 0x64, 0xF1, 0x81, 0x6E, 0xFD, 0x5B); + +#if NET + static ref readonly Guid IComIID.Guid + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get => ref Unsafe.AsRef(in IID_ISetupConfiguration); + } +#else + readonly Guid IComIID.Guid => IID_ISetupConfiguration; +#endif + + private readonly void** _lpVtbl; + + // IUnknown methods (vtable indices 0-2) + + public HRESULT QueryInterface(Guid* riid, void** ppvObject) + { + fixed (ISetupConfiguration* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[0])(pThis, riid, ppvObject); + } + } + + public uint AddRef() + { + fixed (ISetupConfiguration* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[1])(pThis); + } + } + + public uint Release() + { + fixed (ISetupConfiguration* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[2])(pThis); + } + } +} + +#endif diff --git a/src/Framework/Shared/VisualStudio/ISetupConfiguration2.cs b/src/Framework/Shared/VisualStudio/ISetupConfiguration2.cs new file mode 100644 index 00000000000..495e528e4a5 --- /dev/null +++ b/src/Framework/Shared/VisualStudio/ISetupConfiguration2.cs @@ -0,0 +1,87 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#if FEATURE_WINDOWSINTEROP && FEATURE_VISUALSTUDIOSETUP + +using System; +#if NET +using System.Runtime.CompilerServices; +#endif +using Windows.Win32; +using Windows.Win32.Foundation; + +namespace Microsoft.Build.Shared.VisualStudio; + +/// +/// v2 of the Visual Studio Setup Configuration top-level query interface. Derives from +/// ; adds for enumerating +/// every installed instance (including incomplete / partial installations). +/// +/// +/// Declared in Setup.Configuration.idl. IID = {26AAB78C-4A60-49D6-AF3B-3C35BC93365D}. +/// +internal unsafe struct ISetupConfiguration2 : IComIID +{ + public static readonly Guid IID_ISetupConfiguration2 = new(0x26AAB78C, 0x4A60, 0x49D6, 0xAF, 0x3B, 0x3C, 0x35, 0xBC, 0x93, 0x36, 0x5D); + +#if NET + static ref readonly Guid IComIID.Guid + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get => ref Unsafe.AsRef(in IID_ISetupConfiguration2); + } +#else + readonly Guid IComIID.Guid => IID_ISetupConfiguration2; +#endif + + private readonly void** _lpVtbl; + + // IUnknown methods (vtable indices 0-2) + + public HRESULT QueryInterface(Guid* riid, void** ppvObject) + { + fixed (ISetupConfiguration2* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[0])(pThis, riid, ppvObject); + } + } + + public uint AddRef() + { + fixed (ISetupConfiguration2* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[1])(pThis); + } + } + + public uint Release() + { + fixed (ISetupConfiguration2* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[2])(pThis); + } + } + + // ISetupConfiguration vtable layout (indices 3-5): + // 3 = EnumInstances (out IEnumSetupInstances**) + // 4 = GetInstanceForCurrentProcess(out ISetupInstance**) + // 5 = GetInstanceForPath (LPCWSTR, out ISetupInstance**) + + // ISetupConfiguration2 vtable layout (added at index 6): + // 6 = EnumAllInstances (out IEnumSetupInstances**) <-- Used + + /// + /// Enumerate every installed Visual Studio instance the installer knows about, + /// including ones whose InstanceState is incomplete. Use this in preference + /// to EnumInstances when the caller wants to surface partial installs. + /// + public HRESULT EnumAllInstances(IEnumSetupInstances** ppEnumInstances) + { + fixed (ISetupConfiguration2* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[6])(pThis, ppEnumInstances); + } + } +} + +#endif diff --git a/src/Framework/Shared/VisualStudio/ISetupInstance.cs b/src/Framework/Shared/VisualStudio/ISetupInstance.cs new file mode 100644 index 00000000000..b8b3fe22098 --- /dev/null +++ b/src/Framework/Shared/VisualStudio/ISetupInstance.cs @@ -0,0 +1,107 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#if FEATURE_WINDOWSINTEROP && FEATURE_VISUALSTUDIOSETUP + +using System; +#if NET +using System.Runtime.CompilerServices; +#endif +using Windows.Win32; +using Windows.Win32.Foundation; + +namespace Microsoft.Build.Shared.VisualStudio; + +/// +/// A single installed Visual Studio instance. MSBuild only reads three identity fields +/// (installation path, display name, installation version); the rest of the v1 vtable +/// is intentionally not exposed. +/// +/// +/// Declared in Setup.Configuration.idl. IID = {B41463C3-8866-43B5-BC33-2B0676F7F42E}. +/// String returns are ; callers own the BSTR and must release it via +/// SysFreeString. +/// +internal unsafe struct ISetupInstance : IComIID +{ + public static readonly Guid IID_ISetupInstance = new(0xB41463C3, 0x8866, 0x43B5, 0xBC, 0x33, 0x2B, 0x06, 0x76, 0xF7, 0xF4, 0x2E); + +#if NET + static ref readonly Guid IComIID.Guid + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get => ref Unsafe.AsRef(in IID_ISetupInstance); + } +#else + readonly Guid IComIID.Guid => IID_ISetupInstance; +#endif + + private readonly void** _lpVtbl; + + // IUnknown methods (vtable indices 0-2) + + public HRESULT QueryInterface(Guid* riid, void** ppvObject) + { + fixed (ISetupInstance* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[0])(pThis, riid, ppvObject); + } + } + + public uint AddRef() + { + fixed (ISetupInstance* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[1])(pThis); + } + } + + public uint Release() + { + fixed (ISetupInstance* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[2])(pThis); + } + } + + // ISetupInstance vtable layout (indices 3-10): + // 3 = GetInstanceId (out BSTR) + // 4 = GetInstallDate (out FILETIME) + // 5 = GetInstallationName (out BSTR) + // 6 = GetInstallationPath (out BSTR) <-- Used + // 7 = GetInstallationVersion (out BSTR) <-- Used + // 8 = GetDisplayName (LCID, out BSTR) <-- Used + // 9 = GetDescription (LCID, out BSTR) + // 10 = ResolvePath (LPCWSTR, out BSTR) + + /// Get the absolute installation path of this instance (typically ...\Microsoft Visual Studio\YYYY\Edition). + public HRESULT GetInstallationPath(BSTR* pbstrInstallationPath) + { + fixed (ISetupInstance* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[6])(pThis, pbstrInstallationPath); + } + } + + /// Get the installation version (e.g. "17.8.0.0"); parseable by . + public HRESULT GetInstallationVersion(BSTR* pbstrInstallationVersion) + { + fixed (ISetupInstance* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[7])(pThis, pbstrInstallationVersion); + } + } + + /// Get the localized display name (e.g. "Visual Studio Enterprise 2022"). + /// LCID for localization; 0 for the current user UI culture. + /// Receives the localized display name as a freshly-allocated BSTR; the caller owns it and must release it via SysFreeString. + public HRESULT GetDisplayName(uint lcid, BSTR* pbstrDisplayName) + { + fixed (ISetupInstance* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[8])(pThis, lcid, pbstrDisplayName); + } + } +} + +#endif diff --git a/src/Framework/Shared/VisualStudio/ISetupInstance2.cs b/src/Framework/Shared/VisualStudio/ISetupInstance2.cs new file mode 100644 index 00000000000..9dcfb2c8fa8 --- /dev/null +++ b/src/Framework/Shared/VisualStudio/ISetupInstance2.cs @@ -0,0 +1,90 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#if FEATURE_WINDOWSINTEROP && FEATURE_VISUALSTUDIOSETUP + +using System; +#if NET +using System.Runtime.CompilerServices; +#endif +using Windows.Win32; +using Windows.Win32.Foundation; + +namespace Microsoft.Build.Shared.VisualStudio; + +/// +/// v2 of . Derives from ; adds +/// installer-state queries. MSBuild only needs here to filter out +/// partial installations. +/// +/// +/// Declared in Setup.Configuration.idl. IID = {89143C9A-05AF-49B0-B717-72E218A2185C}. +/// +internal unsafe struct ISetupInstance2 : IComIID +{ + public static readonly Guid IID_ISetupInstance2 = new(0x89143C9A, 0x05AF, 0x49B0, 0xB7, 0x17, 0x72, 0xE2, 0x18, 0xA2, 0x18, 0x5C); + +#if NET + static ref readonly Guid IComIID.Guid + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get => ref Unsafe.AsRef(in IID_ISetupInstance2); + } +#else + readonly Guid IComIID.Guid => IID_ISetupInstance2; +#endif + + private readonly void** _lpVtbl; + + // IUnknown methods (vtable indices 0-2) + + public HRESULT QueryInterface(Guid* riid, void** ppvObject) + { + fixed (ISetupInstance2* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[0])(pThis, riid, ppvObject); + } + } + + public uint AddRef() + { + fixed (ISetupInstance2* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[1])(pThis); + } + } + + public uint Release() + { + fixed (ISetupInstance2* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[2])(pThis); + } + } + + // ISetupInstance vtable layout (indices 3-10): see ISetupInstance.cs. + // ISetupInstance2 vtable layout (added at indices 11+): + // 11 = GetState (out InstanceState) <-- Used + // 12 = GetPackages (out SAFEARRAY of ISetupPackageReference*) + // 13 = GetProduct (out ISetupPackageReference**) + // 14 = GetProductPath (out BSTR) + // 15 = GetErrors (out ISetupErrorState**) + // 16 = IsLaunchable (out VARIANT_BOOL) + // 17 = IsComplete (out VARIANT_BOOL) + // 18 = GetProperties (out ISetupPropertyStore**) + // 19 = GetEnginePath (out BSTR) + + /// + /// Get the installer-state bitfield. Compare against + /// to filter out partial installations that the user should not be invoking against. + /// + public HRESULT GetState(InstanceState* pState) + { + fixed (ISetupInstance2* pThis = &this) + { + return ((delegate* unmanaged[Stdcall])_lpVtbl[11])(pThis, pState); + } + } +} + +#endif diff --git a/src/Framework/Shared/VisualStudio/InstanceState.cs b/src/Framework/Shared/VisualStudio/InstanceState.cs new file mode 100644 index 00000000000..2fd5870e8fc --- /dev/null +++ b/src/Framework/Shared/VisualStudio/InstanceState.cs @@ -0,0 +1,45 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#if FEATURE_WINDOWSINTEROP && FEATURE_VISUALSTUDIOSETUP + +using System; + +namespace Microsoft.Build.Shared.VisualStudio; + +/// +/// Bitfield describing how complete the installer considers a given Visual Studio +/// instance. Only an instance whose state equals (all known +/// state bits set) is safe for tooling to invoke against. +/// +/// +/// Declared in Setup.Configuration.idl as InstanceState : DWORD. +/// +[Flags] +internal enum InstanceState : uint +{ + /// No state set yet; the install record is still being populated. + None = 0, + + /// The instance's files exist on disk under its installation path. + Local = 1, + + /// The instance is registered with the OS (Add/Remove Programs, COM, etc.). + Registered = 2, + + /// The instance does not need a reboot before it can be used. + NoRebootRequired = 4, + + /// The instance has no recorded installer errors. + NoErrors = 8, + + /// + /// Sentinel "everything is set" value (MAXUINT in the IDL). Matches the + /// InstanceState.Complete constant in the managed + /// Microsoft.VisualStudio.Setup.Configuration.Interop RCW wrapper that this + /// enum replaces. + /// + Complete = uint.MaxValue, +} + +#endif diff --git a/src/Framework/Shared/VisualStudio/SetupConfiguration.cs b/src/Framework/Shared/VisualStudio/SetupConfiguration.cs new file mode 100644 index 00000000000..7d2ed2c8f9c --- /dev/null +++ b/src/Framework/Shared/VisualStudio/SetupConfiguration.cs @@ -0,0 +1,47 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Manually-defined Visual Studio Setup Configuration COM structs following CsWin32 +// struct-based COM patterns. The Setup Configuration COM API is not in Win32 metadata +// (it ships with the VS installer redistributable as +// Microsoft.VisualStudio.Setup.Configuration.Native.dll); declarations from the +// Setup.Configuration type library. + +#if FEATURE_WINDOWSINTEROP && FEATURE_VISUALSTUDIOSETUP + +using System; +using System.Runtime.InteropServices; + +namespace Microsoft.Build.Shared.VisualStudio; + +/// +/// Activation entry points and well-known IIDs for the Visual Studio Setup Configuration +/// COM API exposed by Microsoft.VisualStudio.Setup.Configuration.Native.dll. +/// +internal static unsafe class SetupConfiguration +{ + /// + /// CLSID of the SetupConfiguration coclass (the entry-point activatable object). + /// CoCreate this CLSID against / + /// to obtain the top-level query interface. + /// + public static readonly Guid CLSID_SetupConfiguration = new(0x177F0C4A, 0x1CD3, 0x4DE7, 0xA3, 0x2C, 0x71, 0xDB, 0xBB, 0x9F, 0xA3, 0x6D); + + /// + /// Fallback activation entry point that lives next to the coclass when the CLSID is + /// not registered with COM (which happens when the helper DLL has not been registered + /// on the machine — e.g. fresh CI agent without the VS installer). Returns a raw + /// pointer suitable for QI'ing to + /// . + /// + /// + /// Signature from Setup.Configuration.idl: + /// STDAPI GetSetupConfiguration(_Outptr_ ISetupConfiguration **ppConfiguration, _Reserved_ LPVOID lpReserved);. + /// The ISetupConfiguration** parameter accepts ComScope<ISetupConfiguration> + /// directly via its implicit T** operator. + /// + [DllImport("Microsoft.VisualStudio.Setup.Configuration.Native.dll", ExactSpelling = true, PreserveSig = true)] + public static extern int GetSetupConfiguration(ISetupConfiguration** ppConfiguration, IntPtr lpReserved); +} + +#endif diff --git a/src/Framework/VisualStudioLocationHelper.cs b/src/Framework/VisualStudioLocationHelper.cs index 1110cdee745..a2bb197bd39 100644 --- a/src/Framework/VisualStudioLocationHelper.cs +++ b/src/Framework/VisualStudioLocationHelper.cs @@ -3,20 +3,27 @@ using System; using System.Collections.Generic; -#if FEATURE_VISUALSTUDIOSETUP -using System.Runtime.InteropServices; -using Microsoft.VisualStudio.Setup.Configuration; +#if FEATURE_WINDOWSINTEROP && FEATURE_VISUALSTUDIOSETUP +using Microsoft.Build.Shared.VisualStudio; +using Windows.Win32; using Windows.Win32.Foundation; +using Windows.Win32.System.Com; #endif namespace Microsoft.Build.Shared { /// - /// Helper class to wrap the Microsoft.VisualStudio.Setup.Configuration.Interop API to query - /// Visual Studio setup for instances installed on the machine. - /// Code derived from sample: https://code.msdn.microsoft.com/Visual-Studio-Setup-0cedd331 + /// Helper class that queries the Visual Studio Setup Configuration COM API for instances + /// of Visual Studio installed on the machine. Will not include anything before VS "15". /// - internal class VisualStudioLocationHelper + /// + /// Uses manually-defined struct-based COM declarations under Shared/VisualStudio/ + /// (matching the CsWin32 struct-based COM pattern used elsewhere in the repo) so this + /// project does not need a reference to the legacy + /// Microsoft.VisualStudio.Setup.Configuration.Interop RCW package. All COM pointer + /// lifetimes are managed via ComScope<T>. + /// + internal static unsafe class VisualStudioLocationHelper { /// /// Query the Visual Studio setup API to get instances of Visual Studio installed @@ -27,84 +34,146 @@ internal static IList GetInstances() { var validInstances = new List(); -#if FEATURE_VISUALSTUDIOSETUP +#if FEATURE_WINDOWSINTEROP && FEATURE_VISUALSTUDIOSETUP try { - // This code is not obvious. See the sample (link above) for reference. - var query = (ISetupConfiguration2)GetQuery(); - var e = query.EnumAllInstances(); - - int fetched; - var instances = new ISetupInstance[1]; - do + using ComScope config = AcquireSetupConfiguration2(); + if (!config.IsNull) { - // Call e.Next to query for the next instance (single item or nothing returned). - e.Next(1, instances, out fetched); - if (fetched <= 0) - { - continue; - } - - var instance = instances[0]; - var state = ((ISetupInstance2)instance).GetState(); - Version version; - - try - { - version = new Version(instance.GetInstallationVersion()); - } - catch (FormatException) - { - continue; - } - - // If the install was complete and a valid version, consider it. - if (state == InstanceState.Complete) - { - validInstances.Add(new VisualStudioInstance( - instance.GetDisplayName(), - instance.GetInstallationPath(), - version)); - } - } while (fetched > 0); + PopulateInstances(config.Pointer, validInstances); + } } - catch (COMException) - { } catch (DllNotFoundException) { - // This is OK, VS "15" or greater likely not installed. + // The Setup Configuration native helper isn't on disk — VS "15" or newer + // is likely not installed. Fall through with whatever has been collected. } #endif + return validInstances; } -#if FEATURE_VISUALSTUDIOSETUP - private static ISetupConfiguration GetQuery() +#if FEATURE_WINDOWSINTEROP && FEATURE_VISUALSTUDIOSETUP + /// + /// Try to obtain a top-level pointer. First attempts + /// CoCreateInstance on the registered coclass; on + /// falls back to the app-local + /// GetSetupConfiguration entry point exported by + /// Microsoft.VisualStudio.Setup.Configuration.Native.dll (which requires that + /// helper DLL to be loadable but doesn't require COM registration). Any other COM + /// failure is returned as a null scope — the caller treats absent VS Setup as an + /// empty result, not an error. + /// + private static ComScope AcquireSetupConfiguration2() { - try + Guid clsid = SetupConfiguration.CLSID_SetupConfiguration; + Guid iidConfig2 = ISetupConfiguration2.IID_ISetupConfiguration2; + + ComScope config2 = new(); + HRESULT hr = PInvoke.CoCreateInstance(&clsid, pUnkOuter: null, CLSCTX.CLSCTX_INPROC_SERVER, &iidConfig2, config2); + if (hr.Succeeded) { - // Try to CoCreate the class object. - return new SetupConfiguration(); + return config2; } - catch (COMException ex) when (ex.ErrorCode == HRESULT.REGDB_E_CLASSNOTREG) + + if (hr != HRESULT.REGDB_E_CLASSNOTREG) { - // Try to get the class object using app-local call. - ISetupConfiguration query; - var result = GetSetupConfiguration(out query, IntPtr.Zero); + // Some other COM failure. The caller swallows errors, so signal "no result" + // by returning an empty scope rather than constructing a COMException only + // to have it discarded. + return default; + } - if (result < 0) + // App-local fallback: the helper DLL is present but not COM-registered. + using ComScope config1 = new(); + int rawHr = SetupConfiguration.GetSetupConfiguration(config1, IntPtr.Zero); + if (rawHr < 0 || config1.IsNull) + { + return default; + } + + HRESULT qiHr = config1.Pointer->QueryInterface(&iidConfig2, config2); + return qiHr.Succeeded ? config2 : default; + } + + /// + /// Enumerate every instance the configuration exposes and append the complete ones + /// to . Per-instance failures are silent — a partial/broken + /// install simply does not appear in the output. + /// + private static void PopulateInstances(ISetupConfiguration2* config, List results) + { + using ComScope enumInstances = new(); + if (config->EnumAllInstances(enumInstances).Failed || enumInstances.IsNull) + { + return; + } + + while (true) + { + using ComScope instance = new(); + uint fetched = 0; + if (enumInstances.Pointer->Next(1, instance, &fetched).Failed + || fetched == 0 + || instance.IsNull) { - throw new COMException("Failed to get query", result); + break; } - return query; + if (TryReadInstance(instance.Pointer, out VisualStudioInstance vs)) + { + results.Add(vs); + } } } - [DllImport("Microsoft.VisualStudio.Setup.Configuration.Native.dll", ExactSpelling = true, PreserveSig = true)] - private static extern int GetSetupConfiguration( - [MarshalAs(UnmanagedType.Interface), Out] out ISetupConfiguration configuration, - IntPtr reserved); + /// + /// Read one : filter by , + /// parse the version string, and capture the display name and installation path. Returns + /// when the instance should be skipped (incomplete install, + /// missing v2 interface, or unparseable version). + /// + private static bool TryReadInstance(ISetupInstance* instance, out VisualStudioInstance result) + { + result = null!; + + // QI to ISetupInstance2 so we can read the install state. Instances that lack v2 are + // ignored — only VS 15 and newer ship the v2 interface, which matches what the old + // RCW-based code required for `GetState`. + using ComScope instance2 = new(); + Guid iidInstance2 = ISetupInstance2.IID_ISetupInstance2; + if (instance->QueryInterface(&iidInstance2, instance2).Failed || instance2.IsNull) + { + return false; + } + + InstanceState state = default; + if (instance2.Pointer->GetState(&state).Failed || state != InstanceState.Complete) + { + return false; + } + + // BSTR is IDisposable: `using` calls SysFreeString in place at scope exit, so we + // don't need a try/finally around each of the three out-params. + using BSTR versionBstr = default; + using BSTR nameBstr = default; + using BSTR pathBstr = default; + + if (instance->GetInstallationVersion(&versionBstr).Failed + || !Version.TryParse(versionBstr.ToString(), out Version version)) + { + return false; + } + + if (instance->GetDisplayName(0, &nameBstr).Failed + || instance->GetInstallationPath(&pathBstr).Failed) + { + return false; + } + + result = new VisualStudioInstance(nameBstr.ToString(), pathBstr.ToString(), version); + return true; + } #endif } diff --git a/src/Shared/InprocTrackingNativeMethods.cs b/src/Shared/InprocTrackingNativeMethods.cs index c3880e7457a..4ca61fc6a78 100644 --- a/src/Shared/InprocTrackingNativeMethods.cs +++ b/src/Shared/InprocTrackingNativeMethods.cs @@ -4,222 +4,229 @@ #if FEATURE_FILE_TRACKER using System; +using System.Buffers; using System.IO; using System.Runtime.InteropServices; -#if FEATURE_CONSTRAINED_EXECUTION -using System.Runtime.ConstrainedExecution; -#endif using System.Security; +using System.Text; using Microsoft.Build.Shared.FileSystem; -#if FEATURE_RESOURCE_EXPOSURE -using System.Runtime.Versioning; -#endif +using Windows.Win32; +using Windows.Win32.Foundation; #nullable disable namespace Microsoft.Build.Shared { /// - /// Methods that are invoked on FileTracker.dll in order to handle inproc tracking + /// Managed shim over FileTracker.dll — the native Detours-based file-access tracker + /// shipped alongside MSBuild on .NET Framework. /// - /// - /// We want to P/Invoke to the FileTracker methods, but FileTracker.dll is not guaranteed to be on PATH (since it's - /// in the MSBuild directory), and there is no DefaultDllImportSearchPath that explicitly points to us. Thus, we - /// are sneaking around P/Invoke by manually acquiring the method pointers and calling them ourselves. The vast - /// majority of this code was lifted from ndp\fx\src\CLRCompression\ZLibNative.cs, which does the same thing for - /// that assembly. - /// - internal static class InprocTrackingNativeMethods + /// + /// FileTracker.dll is not on PATH (it lives in the MSBuild tools directory) and there is + /// no DllImportSearchPath that explicitly points to it, so the module is loaded by + /// absolute path via LoadLibrary and each export is resolved through + /// GetProcAddress into a typed delegate* unmanaged[Stdcall] function pointer. + /// Slot table from FileTracker.def: StartTrackingContext @2, + /// StartTrackingContextWithRoot @3, EndTrackingContext @4, + /// StopTrackingAndCleanup @5, SuspendTracking @6, ResumeTracking @7, + /// WriteAllTLogs @8, WriteContextTLogs @9, SetThreadCount @10. + /// All exports are HRESULT WINAPI Name(...) with Unicode LPCTSTR arguments + /// (FileTracker.dll is built UNICODE), so the function-pointer signatures take + /// and return . + /// + internal static unsafe class InprocTrackingNativeMethods { -#pragma warning disable format // region formatting is different in net7.0 and net472, and cannot be fixed for both - #region Delegates for the tracking functions - - [UnmanagedFunctionPointer(CallingConvention.StdCall)] -#if FEATURE_SECURITY_PERMISSIONS - [SuppressUnmanagedCodeSecurity] -#endif - private delegate int StartTrackingContextDelegate([In, MarshalAs(UnmanagedType.LPWStr)] string intermediateDirectory, [In, MarshalAs(UnmanagedType.LPWStr)] string taskName); - - [UnmanagedFunctionPointer(CallingConvention.StdCall)] -#if FEATURE_SECURITY_PERMISSIONS - [SuppressUnmanagedCodeSecurity] -#endif - private delegate int StartTrackingContextWithRootDelegate([In, MarshalAs(UnmanagedType.LPWStr)] string intermediateDirectory, [In, MarshalAs(UnmanagedType.LPWStr)] string taskName, [In, MarshalAs(UnmanagedType.LPWStr)] string rootMarker); - - [UnmanagedFunctionPointer(CallingConvention.StdCall)] -#if FEATURE_SECURITY_PERMISSIONS - [SuppressUnmanagedCodeSecurity] -#endif - private delegate int EndTrackingContextDelegate(); - - [UnmanagedFunctionPointer(CallingConvention.StdCall)] -#if FEATURE_SECURITY_PERMISSIONS - [SuppressUnmanagedCodeSecurity] -#endif - private delegate int StopTrackingAndCleanupDelegate(); - - [UnmanagedFunctionPointer(CallingConvention.StdCall)] -#if FEATURE_SECURITY_PERMISSIONS - [SuppressUnmanagedCodeSecurity] -#endif - private delegate int SuspendTrackingDelegate(); - - [UnmanagedFunctionPointer(CallingConvention.StdCall)] -#if FEATURE_SECURITY_PERMISSIONS - [SuppressUnmanagedCodeSecurity] -#endif - private delegate int ResumeTrackingDelegate(); - - [UnmanagedFunctionPointer(CallingConvention.StdCall)] -#if FEATURE_SECURITY_PERMISSIONS - [SuppressUnmanagedCodeSecurity] -#endif - private delegate int WriteAllTLogsDelegate([In, MarshalAs(UnmanagedType.LPWStr)] string intermediateDirectory, [In, MarshalAs(UnmanagedType.LPWStr)] string tlogRootName); - - [UnmanagedFunctionPointer(CallingConvention.StdCall)] -#if FEATURE_SECURITY_PERMISSIONS - [SuppressUnmanagedCodeSecurity] -#endif - private delegate int WriteContextTLogsDelegate([In, MarshalAs(UnmanagedType.LPWStr)] string intermediateDirectory, [In, MarshalAs(UnmanagedType.LPWStr)] string tlogRootName); - - [UnmanagedFunctionPointer(CallingConvention.StdCall)] -#if FEATURE_SECURITY_PERMISSIONS - [SuppressUnmanagedCodeSecurity] -#endif - private delegate int SetThreadCountDelegate(int threadCount); - - #endregion // Delegates for the tracking functions - #region Public API - internal static void StartTrackingContext([In, MarshalAs(UnmanagedType.LPWStr)] string intermediateDirectory, [In, MarshalAs(UnmanagedType.LPWStr)] string taskName) + /// + /// Begin a new tracking context on the current thread. File accesses on this thread + /// (and any worker threads pre-allocated via ) will + /// accumulate into per-context read/write/delete TLog lists keyed by + /// until is called. + /// + /// Directory in which TLog files for this context will be written when / is called. + /// Logical task name; used as the TLog file-name prefix and as the key in the context stack. + internal static void StartTrackingContext(string intermediateDirectory, string taskName) { - int hresult = FileTrackerDllStub.startTrackingContextDelegate(intermediateDirectory, taskName); - Marshal.ThrowExceptionForHR(hresult, new IntPtr(-1)); + fixed (char* pDir = intermediateDirectory) + { + fixed (char* pTask = taskName) + { + FileTrackerDllStub.StartTrackingContext(new PCWSTR(pDir), new PCWSTR(pTask)).ThrowOnFailure(); + } + } } - internal static void StartTrackingContextWithRoot([In, MarshalAs(UnmanagedType.LPWStr)] string intermediateDirectory, [In, MarshalAs(UnmanagedType.LPWStr)] string taskName, [In, MarshalAs(UnmanagedType.LPWStr)] string rootMarker) + /// + /// Same as but also supplies a response file from + /// which the rooting marker (the canonical "this is the build root" identifier + /// embedded in TLog headers) is read. + /// + /// Output directory for TLogs. + /// Logical task name (TLog file-name prefix). + /// Path to a response file containing the rooting-marker string. + internal static void StartTrackingContextWithRoot(string intermediateDirectory, string taskName, string rootMarker) { - int hresult = FileTrackerDllStub.startTrackingContextWithRootDelegate(intermediateDirectory, taskName, rootMarker); - Marshal.ThrowExceptionForHR(hresult, new IntPtr(-1)); + fixed (char* pDir = intermediateDirectory) + { + fixed (char* pTask = taskName) + { + fixed (char* pRoot = rootMarker) + { + FileTrackerDllStub.StartTrackingContextWithRoot(new PCWSTR(pDir), new PCWSTR(pTask), new PCWSTR(pRoot)).ThrowOnFailure(); + } + } + } } + /// + /// Pop the top-most tracking context off the current thread's context stack. After + /// this returns, file accesses on the thread fall through to the next context on the + /// stack (if any) or stop being tracked. + /// internal static void EndTrackingContext() { - int hresult = FileTrackerDllStub.endTrackingContextDelegate(); - Marshal.ThrowExceptionForHR(hresult, new IntPtr(-1)); + FileTrackerDllStub.EndTrackingContext().ThrowOnFailure(); } + /// + /// Tear down every tracking context on every tracked thread and release the + /// process-wide tracking data structures. Tracking is fully disabled after this call + /// until a fresh reactivates it. The FileTracker + /// module itself remains loaded (it must — see remarks on the class). + /// internal static void StopTrackingAndCleanup() { - int hresult = FileTrackerDllStub.stopTrackingAndCleanupDelegate(); - Marshal.ThrowExceptionForHR(hresult, new IntPtr(-1)); + FileTrackerDllStub.StopTrackingAndCleanup().ThrowOnFailure(); } + /// + /// Temporarily pause recording into the current thread's active tracking context. + /// File accesses between this call and are not added to + /// the context's TLog lists. + /// internal static void SuspendTracking() { - int hresult = FileTrackerDllStub.suspendTrackingDelegate(); - Marshal.ThrowExceptionForHR(hresult, new IntPtr(-1)); + FileTrackerDllStub.SuspendTracking().ThrowOnFailure(); } + /// + /// Resume recording into the current thread's active tracking context after a prior + /// . + /// internal static void ResumeTracking() { - int hresult = FileTrackerDllStub.resumeTrackingDelegate(); - Marshal.ThrowExceptionForHR(hresult, new IntPtr(-1)); + FileTrackerDllStub.ResumeTracking().ThrowOnFailure(); } - internal static void WriteAllTLogs([In, MarshalAs(UnmanagedType.LPWStr)] string intermediateDirectory, [In, MarshalAs(UnmanagedType.LPWStr)] string tlogRootName) + /// + /// Flush the accumulated read/write/delete TLogs for every tracking context across + /// every tracked thread to disk under , using + /// as the file-name prefix. The contexts remain live + /// and continue to accumulate entries after this call. + /// + /// Output directory for the TLogs. + /// File-name prefix for the produced *.read.tlog / *.write.tlog / *.delete.tlog files. + internal static void WriteAllTLogs(string intermediateDirectory, string tlogRootName) { - int hresult = FileTrackerDllStub.writeAllTLogsDelegate(intermediateDirectory, tlogRootName); - Marshal.ThrowExceptionForHR(hresult, new IntPtr(-1)); + fixed (char* pDir = intermediateDirectory) + { + fixed (char* pRoot = tlogRootName) + { + FileTrackerDllStub.WriteAllTLogs(new PCWSTR(pDir), new PCWSTR(pRoot)).ThrowOnFailure(); + } + } } - internal static void WriteContextTLogs([In, MarshalAs(UnmanagedType.LPWStr)] string intermediateDirectory, [In, MarshalAs(UnmanagedType.LPWStr)] string tlogRootName) + /// + /// Flush the read/write/delete TLogs for just the current thread's active tracking + /// context to disk and clear those in-memory lists (so subsequent activity in the same + /// context is recorded into freshly empty lists). + /// + /// Output directory for the TLogs. + /// File-name prefix for the produced TLog files. + internal static void WriteContextTLogs(string intermediateDirectory, string tlogRootName) { - int hresult = FileTrackerDllStub.writeContextTLogsDelegate(intermediateDirectory, tlogRootName); - Marshal.ThrowExceptionForHR(hresult, new IntPtr(-1)); + fixed (char* pDir = intermediateDirectory) + { + fixed (char* pRoot = tlogRootName) + { + FileTrackerDllStub.WriteContextTLogs(new PCWSTR(pDir), new PCWSTR(pRoot)).ThrowOnFailure(); + } + } } + /// + /// Declare to FileTracker how many worker threads the host intends to use. FileTracker + /// pre-allocates per-thread TLS slots / counters from this value so that worker + /// threads can attach their own tracking contexts without taking a global lock at + /// runtime. + /// + /// Maximum concurrent worker-thread count. internal static void SetThreadCount(int threadCount) { - int hresult = FileTrackerDllStub.setThreadCountDelegate(threadCount); - Marshal.ThrowExceptionForHR(hresult, new IntPtr(-1)); + FileTrackerDllStub.SetThreadCount(threadCount).ThrowOnFailure(); } #endregion // Public API -#pragma warning restore format + private static class FileTrackerDllStub { - private static readonly Lazy fileTrackerDllName = new Lazy(() => RuntimeInformation.ProcessArchitecture == Architecture.Arm64 ? "FileTrackerA4.dll" : (IntPtr.Size == sizeof(Int32)) ? "FileTracker32.dll" : "FileTracker64.dll"); - - // Handle for FileTracker.dll itself - [SecurityCritical] - private static SafeHandle s_fileTrackerDllHandle; -#pragma warning disable format // region formatting is different in net7.0 and net472, and cannot be fixed for both - #region Function pointers to native functions - - internal static StartTrackingContextDelegate startTrackingContextDelegate; - - internal static StartTrackingContextWithRootDelegate startTrackingContextWithRootDelegate; - - internal static EndTrackingContextDelegate endTrackingContextDelegate; - - internal static StopTrackingAndCleanupDelegate stopTrackingAndCleanupDelegate; - - internal static SuspendTrackingDelegate suspendTrackingDelegate; - - internal static ResumeTrackingDelegate resumeTrackingDelegate; - - internal static WriteAllTLogsDelegate writeAllTLogsDelegate; - - internal static WriteContextTLogsDelegate writeContextTLogsDelegate; - - internal static SetThreadCountDelegate setThreadCountDelegate; - - #endregion // Function pointers to native functions - - #region Declarations of Windows API needed to load the native library - - [DllImport("kernel32.dll", CharSet = CharSet.Ansi, BestFitMapping = false)] -#if FEATURE_RESOURCE_EXPOSURE - [ResourceExposure(ResourceScope.Process)] -#endif + // Architecture-specific FileTracker DLL name. The native module is built per + // architecture (x86 / x64 / arm64); the host process must load the variant whose + // bitness and ISA match its own so that Detours can hook the right import table. + private static readonly Lazy s_fileTrackerDllName = new(() => + RuntimeInformation.ProcessArchitecture == Architecture.Arm64 + ? "FileTrackerA4.dll" + : IntPtr.Size == sizeof(int) + ? "FileTracker32.dll" + : "FileTracker64.dll"); + + // Handle for FileTracker.dll itself. The HMODULE is intentionally never freed: + // FileTracker detours ExitProcess and forcibly unloading it would corrupt CLR + // shutdown (matching the historical SafeLibraryHandle.ReleaseHandle no-op). [SecurityCritical] - private static extern IntPtr GetProcAddress(SafeHandle moduleHandle, String procName); - - [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)] -#if FEATURE_RESOURCE_EXPOSURE - [ResourceExposure(ResourceScope.Machine)] -#endif - [SecurityCritical] - private static extern SafeLibraryHandle LoadLibrary(String libPath); - - #endregion // Declarations of Windows API needed to load the native library - - #region Initialization code + private static HMODULE s_fileTrackerDllHandle; + + // Function-pointer slots, populated by InitDelegates() from the LoadLibrary'd + // module via GetProcAddress. Signatures track the native exports verbatim: + // every export is __stdcall HRESULT-returning and takes Unicode LPCWSTR arguments + // (FileTracker.dll is built UNICODE). + internal static delegate* unmanaged[Stdcall] StartTrackingContext; + internal static delegate* unmanaged[Stdcall] StartTrackingContextWithRoot; + internal static delegate* unmanaged[Stdcall] EndTrackingContext; + internal static delegate* unmanaged[Stdcall] StopTrackingAndCleanup; + internal static delegate* unmanaged[Stdcall] SuspendTracking; + internal static delegate* unmanaged[Stdcall] ResumeTracking; + internal static delegate* unmanaged[Stdcall] WriteAllTLogs; + internal static delegate* unmanaged[Stdcall] WriteContextTLogs; + internal static delegate* unmanaged[Stdcall] SetThreadCount; /// - /// Loads FileTracker.dll into a handle that we can use subsequently to grab the exported methods we're interested in. + /// Loads FileTracker.dll from the MSBuild tools directory into + /// . /// private static void LoadFileTrackerDll() { - // Get the FileTracker in our directory that matches the currently running process string buildToolsPath = FrameworkLocationHelper.GeneratePathToBuildToolsForToolsVersion(MSBuildConstants.CurrentToolsVersion, DotNetFrameworkArchitecture.Current); - string fileTrackerPath = Path.Combine(buildToolsPath, fileTrackerDllName.Value); + string fileTrackerPath = Path.Combine(buildToolsPath, s_fileTrackerDllName.Value); if (!FileSystems.Default.FileExists(fileTrackerPath)) { - throw new DllNotFoundException(fileTrackerDllName.Value); + throw new DllNotFoundException(s_fileTrackerDllName.Value); } - SafeLibraryHandle handle = LoadLibrary(fileTrackerPath); + HMODULE handle; + fixed (char* pPath = fileTrackerPath) + { + handle = PInvoke.LoadLibrary(new PCWSTR(pPath)); + } - if (handle.IsInvalid) + if (handle.IsNull) { - Int32 hresult = Marshal.GetHRForLastWin32Error(); - Marshal.ThrowExceptionForHR(hresult, new IntPtr(-1)); + Marshal.ThrowExceptionForHR(Marshal.GetHRForLastWin32Error(), new IntPtr(-1)); - // If Marshal.ThrowExceptionForHR did not throw, we still need to make sure to throw: + // If Marshal.ThrowExceptionForHR did not throw, force a throw here. throw new InvalidOperationException(); } @@ -227,44 +234,64 @@ private static void LoadFileTrackerDll() } /// - /// Generic code to grab the function pointer for a function exported by FileTracker.dll, given - /// that function's name, and transform that function pointer into a callable delegate. + /// Resolve from the loaded FileTracker module + /// and return its address as an unmanaged function pointer. /// [SecurityCritical] - private static DT CreateDelegate
(String entryPointName) + private static void* GetExport(string entryPointName) { - IntPtr entryPoint = GetProcAddress(s_fileTrackerDllHandle, entryPointName); + // GetProcAddress takes a system-default-code-page (ANSI) string. Encode through + // the real encoder so DBCS / multi-byte characters in an unusual export name + // are handled correctly. Rent from ArrayPool to avoid a per-call allocation. + Encoding ansi = Encoding.Default; + int maxByteCount = ansi.GetMaxByteCount(entryPointName.Length) + 1; + byte[] buffer = ArrayPool.Shared.Rent(maxByteCount); + try + { + int written = ansi.GetBytes(entryPointName, 0, entryPointName.Length, buffer, 0); + buffer[written] = 0; + + FARPROC entryPoint; + fixed (byte* pName = buffer) + { + entryPoint = PInvoke.GetProcAddress(s_fileTrackerDllHandle, new PCSTR(pName)); + } + + if (entryPoint.IsNull) + { + throw new EntryPointNotFoundException(s_fileTrackerDllName.Value + "!" + entryPointName); + } - if (IntPtr.Zero == entryPoint) + return (void*)entryPoint.Value; + } + finally { - throw new EntryPointNotFoundException(fileTrackerDllName.Value + "!" + entryPointName); + ArrayPool.Shared.Return(buffer); } - - return (DT)(Object)Marshal.GetDelegateForFunctionPointer(entryPoint, typeof(DT)); } /// - /// Actually generate all of the delegates that will be called by our public (or rather, internal) surface area methods. + /// Resolve every export FileTracker.dll publishes that the managed shim cares + /// about, populating the function-pointer fields. /// private static void InitDelegates() { - Assumed.NotNull(s_fileTrackerDllHandle, "fileTrackerDllHandle should not be null"); - Assumed.False(s_fileTrackerDllHandle.IsInvalid, "Handle for FileTracker.dll should not be invalid"); - - startTrackingContextDelegate = CreateDelegate("StartTrackingContext"); - startTrackingContextWithRootDelegate = CreateDelegate("StartTrackingContextWithRoot"); - endTrackingContextDelegate = CreateDelegate("EndTrackingContext"); - stopTrackingAndCleanupDelegate = CreateDelegate("StopTrackingAndCleanup"); - suspendTrackingDelegate = CreateDelegate("SuspendTracking"); - resumeTrackingDelegate = CreateDelegate("ResumeTracking"); - writeAllTLogsDelegate = CreateDelegate("WriteAllTLogs"); - writeContextTLogsDelegate = CreateDelegate("WriteContextTLogs"); - setThreadCountDelegate = CreateDelegate("SetThreadCount"); + Assumed.False(s_fileTrackerDllHandle.IsNull, "Handle for FileTracker.dll should not be null"); + + StartTrackingContext = (delegate* unmanaged[Stdcall])GetExport(nameof(StartTrackingContext)); + StartTrackingContextWithRoot = (delegate* unmanaged[Stdcall])GetExport(nameof(StartTrackingContextWithRoot)); + EndTrackingContext = (delegate* unmanaged[Stdcall])GetExport(nameof(EndTrackingContext)); + StopTrackingAndCleanup = (delegate* unmanaged[Stdcall])GetExport(nameof(StopTrackingAndCleanup)); + SuspendTracking = (delegate* unmanaged[Stdcall])GetExport(nameof(SuspendTracking)); + ResumeTracking = (delegate* unmanaged[Stdcall])GetExport(nameof(ResumeTracking)); + WriteAllTLogs = (delegate* unmanaged[Stdcall])GetExport(nameof(WriteAllTLogs)); + WriteContextTLogs = (delegate* unmanaged[Stdcall])GetExport(nameof(WriteContextTLogs)); + SetThreadCount = (delegate* unmanaged[Stdcall])GetExport(nameof(SetThreadCount)); } /// - /// Static constructor -- generates the delegates for all of the export methods from - /// FileTracker.dll that we care about. + /// Type initializer: loads the native module and resolves every export once + /// before any of the function-pointer fields are read. /// [SecuritySafeCritical] static FileTrackerDllStub() @@ -272,35 +299,6 @@ static FileTrackerDllStub() LoadFileTrackerDll(); InitDelegates(); } - - #endregion // Initialization code -#pragma warning restore format - // Specialized handle to make sure we free FileTracker.dll - [SecurityCritical] - private class SafeLibraryHandle : SafeHandle - { - internal SafeLibraryHandle() - : base(IntPtr.Zero, true) - { - } - - public override bool IsInvalid - { - [SecurityCritical] - get - { return IntPtr.Zero == handle; } - } -#if FEATURE_CONSTRAINED_EXECUTION - [ReliabilityContract(Consistency.WillNotCorruptState, Cer.MayFail)] -#endif - [SecurityCritical] - protected override bool ReleaseHandle() - { - // FileTracker expects to continue to exist even through ExitProcess -- if we forcibly unload it now, - // bad things can happen when the CLR attempts to call the (still detoured?) ExitProcess. - return true; - } - } // private class SafeLibraryHandle } } } diff --git a/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs b/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs index c724deb8221..5b813be5b50 100644 --- a/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs +++ b/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs @@ -14,6 +14,7 @@ #if FEATURE_WINDOWSINTEROP using Windows.Win32; using Windows.Win32.Foundation; +using Windows.Win32.System.LibraryLoader; #endif namespace Microsoft.Build.Tasks.UnitTests @@ -157,40 +158,36 @@ public void E2EScenarioTests(string? manifestName, bool expectedResult) [SupportedOSPlatform("windows6.1")] internal sealed class AssemblyNativeResourceManager { - public enum LoadLibraryFlags : uint { LOAD_LIBRARY_AS_DATAFILE = 2 }; - - [DllImport("Kernel32.dll", CharSet = CharSet.Auto, SetLastError = true)] - public static extern IntPtr LoadLibrary(string lpFileName, IntPtr hReservedNull, LoadLibraryFlags dwFlags); - - [DllImport("kernel32.dll", SetLastError = true)] - public static extern IntPtr FindResource(IntPtr hModule, string lpName, string lpType); - - [DllImport("kernel32.dll", SetLastError = true)] - public static extern IntPtr LoadResource(IntPtr hModule, IntPtr hResInfo); - - [DllImport("kernel32.dll", SetLastError = true)] - public static extern IntPtr LockResource(IntPtr hResData); - - [DllImport("kernel32.dll", SetLastError = true)] - public static extern uint SizeofResource(IntPtr hModule, IntPtr hResInfo); - - public static byte[]? GetResourceFromExecutable(string assembly, string lpName, string lpType) + public static unsafe byte[]? GetResourceFromExecutable(string assembly, string lpName, string lpType) { - IntPtr hModule = LoadLibrary(assembly, IntPtr.Zero, LoadLibraryFlags.LOAD_LIBRARY_AS_DATAFILE); + HMODULE hModule; + fixed (char* pAssembly = assembly) + { + hModule = PInvoke.LoadLibraryEx(new PCWSTR(pAssembly), default, LOAD_LIBRARY_FLAGS.LOAD_LIBRARY_AS_DATAFILE); + } + try { - if (hModule != IntPtr.Zero) + if (!hModule.IsNull) { - IntPtr hResource = FindResource(hModule, lpName, lpType); + HRSRC hResource; + fixed (char* pName = lpName) + { + fixed (char* pType = lpType) + { + hResource = PInvoke.FindResource(hModule, new PCWSTR(pName), new PCWSTR(pType)); + } + } + if (hResource != IntPtr.Zero) { - uint resSize = SizeofResource(hModule, hResource); - IntPtr resData = LoadResource(hModule, hResource); + uint resSize = PInvoke.SizeofResource(hModule, hResource); + HGLOBAL resData = PInvoke.LoadResource(hModule, hResource); if (resData != IntPtr.Zero) { byte[] uiBytes = new byte[resSize]; - IntPtr ipMemorySource = LockResource(resData); - Marshal.Copy(ipMemorySource, uiBytes, 0, (int)resSize); + void* pMemorySource = PInvoke.LockResource(resData); + Marshal.Copy((IntPtr)pMemorySource, uiBytes, 0, (int)resSize); return uiBytes; } @@ -199,7 +196,7 @@ public enum LoadLibraryFlags : uint { LOAD_LIBRARY_AS_DATAFILE = 2 }; } finally { - PInvoke.FreeLibrary((HMODULE)hModule); + PInvoke.FreeLibrary(hModule); } return null; diff --git a/src/Tasks.UnitTests/MetadataReader_Tests.cs b/src/Tasks.UnitTests/MetadataReader_Tests.cs index c2c5178cd48..915a9a9f6eb 100644 --- a/src/Tasks.UnitTests/MetadataReader_Tests.cs +++ b/src/Tasks.UnitTests/MetadataReader_Tests.cs @@ -264,5 +264,37 @@ public void ConcurrentPropertyAccess_OnSameInstance_IsThreadSafe() Task.WaitAll(tasks); } + + [Fact] + public void HasAssemblyAttributes_Batch_AgreesWithSingular() + { + // The batch overload was added to share a single GIT acquisition + GetAssemblyFromScope + // across multiple probes on the net472 path. It must return the same per-name result + // the singular `HasAssemblyAttribute(name)` overload returns. Mixes a known-present + // attribute (every SDK-built assembly has TargetFrameworkAttribute) with two clearly + // bogus names so both true and false outcomes are exercised in one call. + using MetadataReader reader = MetadataReader.Create(ThisAssemblyPath); + reader.ShouldNotBeNull(); + + string[] names = + [ + "System.Runtime.Versioning.TargetFrameworkAttribute", + "Definitely.Not.A.Real.Attribute", + "Another.Missing.AttributeName", + ]; + + bool[] batch = new bool[names.Length]; + reader.HasAssemblyAttributes(names, batch); + + for (int i = 0; i < names.Length; i++) + { + batch[i].ShouldBe(reader.HasAssemblyAttribute(names[i]), $"mismatch on '{names[i]}'"); + } + + // Sanity: the well-known one must be present and the bogus ones must be absent. + batch[0].ShouldBeTrue(); + batch[1].ShouldBeFalse(); + batch[2].ShouldBeFalse(); + } } } diff --git a/src/Tasks.UnitTests/Microsoft.Build.Tasks.UnitTests.csproj b/src/Tasks.UnitTests/Microsoft.Build.Tasks.UnitTests.csproj index 1c1696e6440..d46ce18147b 100644 --- a/src/Tasks.UnitTests/Microsoft.Build.Tasks.UnitTests.csproj +++ b/src/Tasks.UnitTests/Microsoft.Build.Tasks.UnitTests.csproj @@ -9,6 +9,7 @@ Microsoft.Build.Tasks.UnitTests true $(DefineConstants);MICROSOFT_BUILD_TASKS_UNITTESTS + true diff --git a/src/Tasks/ManifestUtil/ApplicationManifest.cs b/src/Tasks/ManifestUtil/ApplicationManifest.cs index 22564f94bc6..fa0e7fa37a3 100644 --- a/src/Tasks/ManifestUtil/ApplicationManifest.cs +++ b/src/Tasks/ManifestUtil/ApplicationManifest.cs @@ -960,6 +960,14 @@ private class AssemblyAttributeFlags public readonly bool HasImportedFromTypeLibAttribute; public readonly bool HasSecurityTransparentAttribute; + private static readonly string[] s_attributeNames = + [ + "System.Security.AllowPartiallyTrustedCallersAttribute", + "System.Security.SecurityTransparentAttribute", + "System.Runtime.InteropServices.PrimaryInteropAssemblyAttribute", + "System.Runtime.InteropServices.ImportedFromTypeLibAttribute", + ]; + public AssemblyAttributeFlags(string path) { using (MetadataReader r = MetadataReader.Create(path)) @@ -967,10 +975,15 @@ public AssemblyAttributeFlags(string path) if (r != null) { IsSigned = !String.IsNullOrEmpty(r.PublicKeyToken); - HasAllowPartiallyTrustedCallersAttribute = r.HasAssemblyAttribute("System.Security.AllowPartiallyTrustedCallersAttribute"); - HasSecurityTransparentAttribute = r.HasAssemblyAttribute("System.Security.SecurityTransparentAttribute"); - HasPrimaryInteropAssemblyAttribute = r.HasAssemblyAttribute("System.Runtime.InteropServices.PrimaryInteropAssemblyAttribute"); - HasImportedFromTypeLibAttribute = r.HasAssemblyAttribute("System.Runtime.InteropServices.ImportedFromTypeLibAttribute"); + + // Batched on the net472/COM path to share a single GIT acquisition across + // all four probes; on the netcore path it iterates the cached attribute list. + bool[] results = new bool[s_attributeNames.Length]; + r.HasAssemblyAttributes(s_attributeNames, results); + HasAllowPartiallyTrustedCallersAttribute = results[0]; + HasSecurityTransparentAttribute = results[1]; + HasPrimaryInteropAssemblyAttribute = results[2]; + HasImportedFromTypeLibAttribute = results[3]; } } } diff --git a/src/Tasks/ManifestUtil/MetadataReader.cs b/src/Tasks/ManifestUtil/MetadataReader.cs index 78f8007ad38..7195aac8473 100644 --- a/src/Tasks/ManifestUtil/MetadataReader.cs +++ b/src/Tasks/ManifestUtil/MetadataReader.cs @@ -60,6 +60,21 @@ public static MetadataReader Create(string path) } public bool HasAssemblyAttribute(string name) + { + EnsureCustomAttributes(); + return _customAttributes.Contains(name); + } + + public void HasAssemblyAttributes(string[] names, bool[] results) + { + EnsureCustomAttributes(); + for (int i = 0; i < names.Length; i++) + { + results[i] = _customAttributes.Contains(names[i]); + } + } + + private void EnsureCustomAttributes() { if (_customAttributes == null) { @@ -71,8 +86,6 @@ public bool HasAssemblyAttribute(string name) } } } - - return _customAttributes.Contains(name); } public string Name => Attributes[nameof(Name)]; @@ -359,16 +372,46 @@ public bool HasAssemblyAttribute(string name) { return false; } + return HasAssemblyAttribute(import2.Pointer, assemblyScope, name); + } - // 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 - // not S_OK as "not present" so failure paths cannot leave valueLen indeterminate. + // Batch variant: callers (e.g. AssemblyAttributeFlags) that probe several attributes on + // the same reader pay a single GIT round-trip and a single GetAssemblyFromScope call + // instead of one per attribute. + public void HasAssemblyAttributes(string[] names, bool[] results) + { + using ComScope asmImport = _assemblyImport.GetInterface(); + using ComScope import2 = _import2.GetInterface(); + MdAssembly assemblyScope; + if (asmImport.Pointer->GetAssemblyFromScope(&assemblyScope).Failed || assemblyScope.IsNil) + { + for (int i = 0; i < results.Length; i++) + { + results[i] = false; + } + return; + } + + for (int i = 0; i < names.Length; i++) + { + results[i] = HasAssemblyAttribute(import2, assemblyScope, names[i]); + } + } + + // Takes a borrowed IMetaDataImport2* so callers in a hot path can reuse a single GIT + // round-trip instead of paying one per call. + // + // 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 + // not S_OK as "not present" so failure paths cannot leave valueLen indeterminate. + private static bool HasAssemblyAttribute(IMetaDataImport2* import2, MdAssembly assemblyScope, string name) + { void* valuePtr = null; uint valueLen = 0; HRESULT hr; fixed (char* pName = name) { - hr = import2.Pointer->GetCustomAttributeByName(assemblyScope, pName, &valuePtr, &valueLen); + hr = import2->GetCustomAttributeByName(assemblyScope, pName, &valuePtr, &valueLen); } return hr == HRESULT.S_OK && valueLen != 0; } diff --git a/src/Tasks/Microsoft.Build.Tasks.csproj b/src/Tasks/Microsoft.Build.Tasks.csproj index 4b7de4fb696..4b328798ab6 100644 --- a/src/Tasks/Microsoft.Build.Tasks.csproj +++ b/src/Tasks/Microsoft.Build.Tasks.csproj @@ -647,7 +647,6 @@ - diff --git a/src/UnitTests.Shared/DriveMapping.cs b/src/UnitTests.Shared/DriveMapping.cs index c9034b6902a..a3ddcdc6c2c 100644 --- a/src/UnitTests.Shared/DriveMapping.cs +++ b/src/UnitTests.Shared/DriveMapping.cs @@ -4,6 +4,9 @@ #nullable enable using System.Runtime.InteropServices; using System.Runtime.Versioning; +using Windows.Win32; +using Windows.Win32.Foundation; +using Windows.Win32.Storage.FileSystem; namespace Microsoft.Build.UnitTests.Shared; @@ -11,8 +14,8 @@ internal static class DriveMapping { private const int ERROR_FILE_NOT_FOUND = 2; private const int ERROR_INSUFFICIENT_BUFFER = 122; - private const int DDD_REMOVE_DEFINITION = 2; - private const int DDD_NO_FLAG = 0; + private const DEFINE_DOS_DEVICE_FLAGS DDD_REMOVE_DEFINITION = (DEFINE_DOS_DEVICE_FLAGS)2; + private const DEFINE_DOS_DEVICE_FLAGS DDD_NO_FLAG = 0; // extra space for '\??\'. Not counting for long paths support in tests. private const int MAX_PATH = 259; @@ -21,10 +24,10 @@ internal static class DriveMapping ///
/// Drive letter /// Path to be mapped - [SupportedOSPlatform("windows")] + [SupportedOSPlatform("windows5.1.2600")] public static void MapDrive(char letter, string path) { - if (!DefineDosDevice(DDD_NO_FLAG, ToDeviceName(letter), path)) + if (!PInvoke.DefineDosDevice(DDD_NO_FLAG, ToDeviceName(letter), path)) { NativeMethodsShared.ThrowExceptionForErrorCode(Marshal.GetLastWin32Error()); } @@ -34,10 +37,10 @@ public static void MapDrive(char letter, string path) /// Windows specific. Unmaps drive mapping. /// /// Drive letter. - [SupportedOSPlatform("windows")] + [SupportedOSPlatform("windows5.1.2600")] public static void UnmapDrive(char letter) { - if (!DefineDosDevice(DDD_REMOVE_DEFINITION, ToDeviceName(letter), null)) + if (!PInvoke.DefineDosDevice(DDD_REMOVE_DEFINITION, ToDeviceName(letter), null)) { NativeMethodsShared.ThrowExceptionForErrorCode(Marshal.GetLastWin32Error()); } @@ -48,15 +51,28 @@ public static void UnmapDrive(char letter) /// /// Drive letter. /// Path mapped under specified letter. Empty string if mapping not found. - [SupportedOSPlatform("windows")] - public static string GetDriveMapping(char letter) + [SupportedOSPlatform("windows5.1.2600")] + public static unsafe string GetDriveMapping(char letter) { // since this is just for test purposes - let's not overcomplicate with long paths support char[] buffer = new char[MAX_PATH]; + string deviceName = ToDeviceName(letter); - while (QueryDosDevice(ToDeviceName(letter), buffer, buffer.Length) == 0) + while (true) { - // Return empty string if the drive is not mapped + uint length; + fixed (char* pBuf = buffer) + { + fixed (char* pDevice = deviceName) + { + length = PInvoke.QueryDosDevice(new PCWSTR(pDevice), new PWSTR(pBuf), (uint)buffer.Length); + } + } + if (length != 0) + { + break; + } + int err = Marshal.GetLastWin32Error(); if (err == ERROR_FILE_NOT_FOUND) { @@ -79,12 +95,4 @@ private static string ToDeviceName(char letter) { return $"{char.ToUpper(letter)}:"; } - - [DllImport("kernel32.dll", CharSet = CharSet.Auto, SetLastError = true)] - [SupportedOSPlatform("windows")] - private static extern bool DefineDosDevice([In] int flags, [In] string deviceName, [In] string? path); - - [DllImport("kernel32.dll", CharSet = CharSet.Auto, SetLastError = true)] - [SupportedOSPlatform("windows")] - private static extern int QueryDosDevice([In] string deviceName, [Out] char[] buffer, [In] int bufSize); } diff --git a/src/Utilities.UnitTests/Microsoft.Build.Utilities.UnitTests.csproj b/src/Utilities.UnitTests/Microsoft.Build.Utilities.UnitTests.csproj index fc358d0663e..c3d9235a4a8 100644 --- a/src/Utilities.UnitTests/Microsoft.Build.Utilities.UnitTests.csproj +++ b/src/Utilities.UnitTests/Microsoft.Build.Utilities.UnitTests.csproj @@ -6,6 +6,7 @@ Microsoft.Build.Utilities.UnitTests true Microsoft.Build.Utilities.UnitTests + true @@ -28,9 +29,6 @@ - - NativeMethods.cs - App.config diff --git a/src/Utilities.UnitTests/TrackedDependencies/FileTrackerTests.cs b/src/Utilities.UnitTests/TrackedDependencies/FileTrackerTests.cs index 6eae1c71a1a..afeea2c94d7 100644 --- a/src/Utilities.UnitTests/TrackedDependencies/FileTrackerTests.cs +++ b/src/Utilities.UnitTests/TrackedDependencies/FileTrackerTests.cs @@ -20,12 +20,154 @@ #endif using Xunit; -using BackEndNativeMethods = Microsoft.Build.BackEnd.NativeMethods; +using Windows.Win32; +using Windows.Win32.Foundation; +using Windows.Win32.System.Threading; // PLEASE NOTE: This is a UNICODE file as it contains UNICODE characters! #nullable disable +namespace Microsoft.Build.UnitTests.FileTracking +{ + /// + /// Test-only CreateProcess fixture used by the (currently skipped) FileTracker tests. + /// Routes through Windows.Win32.PInvoke.CreateProcess; keeps the original API + /// surface that the tests were written against so the call sites read the same way. + /// + internal static class BackEndNativeMethods + { + public static readonly IntPtr NullPtr = IntPtr.Zero; + public static readonly IntPtr InvalidHandle = new IntPtr(-1); + + public const uint NORMALPRIORITYCLASS = 0x0020; + public const uint CREATENOWINDOW = 0x08000000; + public const int STARTFUSESTDHANDLES = 0x00000100; + + [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] + internal struct STARTUP_INFO + { + internal int cb; + internal string lpReserved; + internal string lpDesktop; + internal string lpTitle; + internal int dwX; + internal int dwY; + internal int dwXSize; + internal int dwYSize; + internal int dwXCountChars; + internal int dwYCountChars; + internal int dwFillAttribute; + internal int dwFlags; + internal short wShowWindow; + internal short cbReserved2; + internal IntPtr lpReserved2; + internal IntPtr hStdInput; + internal IntPtr hStdOutput; + internal IntPtr hStdError; + } + + [StructLayout(LayoutKind.Sequential)] + internal struct SECURITY_ATTRIBUTES + { + public int nLength; + public IntPtr lpSecurityDescriptor; + public int bInheritHandle; + } + + [StructLayout(LayoutKind.Sequential)] + internal struct PROCESS_INFORMATION + { + public IntPtr hProcess; + public IntPtr hThread; + public int dwProcessId; + public int dwThreadId; + } + + // Forwards to Windows.Win32.PInvoke.CreateProcess with the same parameter shape + // the existing tests use. Note that this code path is currently exercised only by + // tests gated on FEATURE_FILE_TRACKER, all of which are skipped (see issue #649). + public static unsafe bool CreateProcess( + string lpApplicationName, + string lpCommandLine, + ref SECURITY_ATTRIBUTES lpProcessAttributes, + ref SECURITY_ATTRIBUTES lpThreadAttributes, + bool bInheritHandles, + uint dwCreationFlags, + IntPtr lpEnvironment, + string lpCurrentDirectory, + ref STARTUP_INFO lpStartupInfo, + out PROCESS_INFORMATION lpProcessInformation) + { + // The managed STARTUP_INFO above has `string` fields, so it isn't blittable and + // cannot be reinterpret-cast to STARTUPINFOW*. Marshal into a Win32-shaped struct + // for the duration of the call. CreateProcess only consumes lpStartupInfo, so we + // don't need to copy anything back. + Windows.Win32.System.Threading.STARTUPINFOW si = default; + si.cb = (uint)sizeof(Windows.Win32.System.Threading.STARTUPINFOW); + si.dwX = (uint)lpStartupInfo.dwX; + si.dwY = (uint)lpStartupInfo.dwY; + si.dwXSize = (uint)lpStartupInfo.dwXSize; + si.dwYSize = (uint)lpStartupInfo.dwYSize; + si.dwXCountChars = (uint)lpStartupInfo.dwXCountChars; + si.dwYCountChars = (uint)lpStartupInfo.dwYCountChars; + si.dwFillAttribute = (uint)lpStartupInfo.dwFillAttribute; + si.dwFlags = (STARTUPINFOW_FLAGS)lpStartupInfo.dwFlags; + si.wShowWindow = (ushort)lpStartupInfo.wShowWindow; + si.hStdInput = new HANDLE(lpStartupInfo.hStdInput.ToPointer()); + si.hStdOutput = new HANDLE(lpStartupInfo.hStdOutput.ToPointer()); + si.hStdError = new HANDLE(lpStartupInfo.hStdError.ToPointer()); + + Windows.Win32.Security.SECURITY_ATTRIBUTES pSec = new() + { + nLength = (uint)sizeof(Windows.Win32.Security.SECURITY_ATTRIBUTES), + lpSecurityDescriptor = (void*)lpProcessAttributes.lpSecurityDescriptor, + bInheritHandle = lpProcessAttributes.bInheritHandle != 0, + }; + Windows.Win32.Security.SECURITY_ATTRIBUTES tSec = new() + { + nLength = (uint)sizeof(Windows.Win32.Security.SECURITY_ATTRIBUTES), + lpSecurityDescriptor = (void*)lpThreadAttributes.lpSecurityDescriptor, + bInheritHandle = lpThreadAttributes.bInheritHandle != 0, + }; + + Windows.Win32.System.Threading.PROCESS_INFORMATION pi = default; + + bool ok; + fixed (char* pApp = lpApplicationName) + { + fixed (char* pCmd = lpCommandLine) + { + fixed (char* pCwd = lpCurrentDirectory) + { + ok = PInvoke.CreateProcess( + new PCWSTR(pApp), + new PWSTR(pCmd), + &pSec, + &tSec, + bInheritHandles, + (PROCESS_CREATION_FLAGS)dwCreationFlags, + (void*)lpEnvironment, + new PCWSTR(pCwd), + &si, + &pi); + } + } + } + + lpProcessInformation = new PROCESS_INFORMATION + { + hProcess = (IntPtr)pi.hProcess.Value, + hThread = (IntPtr)pi.hThread.Value, + dwProcessId = (int)pi.dwProcessId, + dwThreadId = (int)pi.dwThreadId, + }; + + return ok; + } + } +} + namespace Microsoft.Build.UnitTests.FileTracking { public sealed class FileTrackerTests : IDisposable diff --git a/src/Utilities/Microsoft.Build.Utilities.csproj b/src/Utilities/Microsoft.Build.Utilities.csproj index ac8644ad6cf..5a953724e51 100644 --- a/src/Utilities/Microsoft.Build.Utilities.csproj +++ b/src/Utilities/Microsoft.Build.Utilities.csproj @@ -25,7 +25,6 @@ - From bc2919f097fb0e1d11a2f9d5788bd138d4591a1d Mon Sep 17 00:00:00 2001 From: Jeremy Kuhne Date: Tue, 26 May 2026 17:30:03 -0700 Subject: [PATCH 2/4] Address PR #13872 review: source-build, DriveMapping length, CreateProcess buffer, handle null checks Fixes Source-Build (Managed) CI failure and 4 Copilot review comments. src/Framework/Microsoft.Build.Framework.csproj Add FileSystem/WindowsNative.cs, FileSystem/SafeFileHandle.cs, and Shared/VisualStudio/**/*.cs to the existing FeatureWindowsInterop=false Compile Remove list. These files now reference Windows.Win32.* types, which the source build does not generate. src/UnitTests.Shared/DriveMapping.cs GetDriveMapping was passing buffer.Length to QueryDosDevice and then using buffer.Length - 4 to size the result, embedding trailing NULs and unused buffer when the actual mapping was shorter. Hoist 'length' out of the while loop and bound the substring by (int)length - 5 (strip the four-char '\??\\' prefix and the trailing NUL). src/Utilities.UnitTests/TrackedDependencies/FileTrackerTests.cs Allocate a writable char[] copy of lpCommandLine before pinning it for PInvoke.CreateProcess. CreateProcessW is documented to be allowed to modify the lpCommandLine buffer in place; pinning a managed string would let native code write to immutable string storage. src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs Use HRSRC.IsNull / HGLOBAL.IsNull instead of comparing to IntPtr.Zero to match the CsWin32 strongly-typed-handle convention used elsewhere. src/Tasks/ManifestUtil/MetadataReader.cs HasAssemblyAttributes batch overload now passes import2.Pointer explicitly to the private HasAssemblyAttribute helper. The previous bare 'import2' relied on ComScope's implicit T* conversion and was harder to read. Verified - Source build: dotnet build src/Framework /p:DotNetBuildSourceOnly=true /p:DotNetBuild=true -> 0 warnings, 0 errors (the failing CI leg). - Full build: dotnet build MSBuild.Dev.slnf -> 0 warnings, 0 errors. - Tests: WindowsNative_Tests, MetadataReader_Tests, AddToWin32Manifest_Tests all green on both TFMs. --- src/Framework/Microsoft.Build.Framework.csproj | 3 +++ .../AddToWin32Manifest_Tests.cs | 4 ++-- src/Tasks/ManifestUtil/MetadataReader.cs | 2 +- src/UnitTests.Shared/DriveMapping.cs | 8 +++++--- .../TrackedDependencies/FileTrackerTests.cs | 18 +++++++++++++++++- 5 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/Framework/Microsoft.Build.Framework.csproj b/src/Framework/Microsoft.Build.Framework.csproj index fcac65ccc6f..55f424e11b1 100644 --- a/src/Framework/Microsoft.Build.Framework.csproj +++ b/src/Framework/Microsoft.Build.Framework.csproj @@ -90,8 +90,11 @@ + + + diff --git a/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs b/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs index 5b813be5b50..bff1015a2f5 100644 --- a/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs +++ b/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs @@ -179,11 +179,11 @@ internal sealed class AssemblyNativeResourceManager } } - if (hResource != IntPtr.Zero) + if (!hResource.IsNull) { uint resSize = PInvoke.SizeofResource(hModule, hResource); HGLOBAL resData = PInvoke.LoadResource(hModule, hResource); - if (resData != IntPtr.Zero) + if (!resData.IsNull) { byte[] uiBytes = new byte[resSize]; void* pMemorySource = PInvoke.LockResource(resData); diff --git a/src/Tasks/ManifestUtil/MetadataReader.cs b/src/Tasks/ManifestUtil/MetadataReader.cs index 7195aac8473..31b0e1cb0b3 100644 --- a/src/Tasks/ManifestUtil/MetadataReader.cs +++ b/src/Tasks/ManifestUtil/MetadataReader.cs @@ -394,7 +394,7 @@ public void HasAssemblyAttributes(string[] names, bool[] results) for (int i = 0; i < names.Length; i++) { - results[i] = HasAssemblyAttribute(import2, assemblyScope, names[i]); + results[i] = HasAssemblyAttribute(import2.Pointer, assemblyScope, names[i]); } } diff --git a/src/UnitTests.Shared/DriveMapping.cs b/src/UnitTests.Shared/DriveMapping.cs index a3ddcdc6c2c..41cb7588f39 100644 --- a/src/UnitTests.Shared/DriveMapping.cs +++ b/src/UnitTests.Shared/DriveMapping.cs @@ -57,10 +57,10 @@ public static unsafe string GetDriveMapping(char letter) // since this is just for test purposes - let's not overcomplicate with long paths support char[] buffer = new char[MAX_PATH]; string deviceName = ToDeviceName(letter); + uint length; while (true) { - uint length; fixed (char* pBuf = buffer) { fixed (char* pDevice = deviceName) @@ -87,8 +87,10 @@ public static unsafe string GetDriveMapping(char letter) buffer = new char[buffer.Length * 4]; } - // Translate from the native path semantic - starting with '\??\' - return new string(buffer, 4, buffer.Length - 4); + // Translate from the native path semantic - starting with '\??\'. `length` is the + // number of characters QueryDosDevice copied INCLUDING the trailing NUL; trim the + // prefix (4) and the terminator (1) to get the managed string content. + return new string(buffer, 4, (int)length - 5); } private static string ToDeviceName(char letter) diff --git a/src/Utilities.UnitTests/TrackedDependencies/FileTrackerTests.cs b/src/Utilities.UnitTests/TrackedDependencies/FileTrackerTests.cs index afeea2c94d7..24b2e58e113 100644 --- a/src/Utilities.UnitTests/TrackedDependencies/FileTrackerTests.cs +++ b/src/Utilities.UnitTests/TrackedDependencies/FileTrackerTests.cs @@ -133,10 +133,26 @@ public static unsafe bool CreateProcess( Windows.Win32.System.Threading.PROCESS_INFORMATION pi = default; + // CreateProcessW is documented to be allowed to modify lpCommandLine in place + // (it parses argv[0] out of it on the way in). Pinning a managed string would let + // native code write to immutable string storage, so copy into a fresh char[] — + // sized for the null terminator — and pin that. + char[] cmdLineBuffer; + if (lpCommandLine is null) + { + cmdLineBuffer = null; + } + else + { + cmdLineBuffer = new char[lpCommandLine.Length + 1]; + lpCommandLine.CopyTo(0, cmdLineBuffer, 0, lpCommandLine.Length); + // CLR initializes the array to 0, so cmdLineBuffer[length] is the terminator. + } + bool ok; fixed (char* pApp = lpApplicationName) { - fixed (char* pCmd = lpCommandLine) + fixed (char* pCmd = cmdLineBuffer) { fixed (char* pCwd = lpCurrentDirectory) { From 8483629a770f805735f9c0c9b263b5fc39fd7b5e Mon Sep 17 00:00:00 2001 From: Jeremy Kuhne Date: Tue, 26 May 2026 17:43:41 -0700 Subject: [PATCH 3/4] Address PR #13872 Expert Code Review: defensive Dispose, DangerousAddRef, nullable Five non-blocking hardening / style improvements from the Expert Code Review. src/Framework/VisualStudioLocationHelper.cs - GetInstances: comment the 'no ThrowOnFailure in this codepath by design' exception contract so a future maintainer doesn't introduce one without adding the matching catch. - AcquireSetupConfiguration2: defensive config2.Dispose() on both failure return paths (non-REGDB_E_CLASSNOTREG CoCreate failure + QueryInterface failure). COM correctly nulls the output on failure so Dispose is a no-op today; making it explicit guards against a buggy COM implementation and documents the ownership model. src/Framework/FileSystem/WindowsNative.cs FindNextFileW: wrap PInvoke.FindNextFile with DangerousAddRef / DangerousRelease on the input SafeHandle so a concurrent Dispose() from another thread cannot invalidate the handle mid-call. Existing callers are single-threaded but the cost is negligible. src/Shared/InprocTrackingNativeMethods.cs CreateDelegate: comment Encoding.Default's net472 vs .NET (Core) difference. FEATURE_FILE_TRACKER is net472-only today so the system ANSI code page is correct for GetProcAddress; if the #if guard ever broadens to .NET, switch to a CodePagesEncodingProvider ANSI encoding. src/Framework.UnitTests/FileSystem/WindowsNative_Tests.cs src/Framework.UnitTests/VisualStudioLocationHelper_Tests.cs src/Tasks.UnitTests/AssemblyInformation_Tests.cs Remove '#nullable disable' from these new test files to match the repo guideline that new files use nullable reference types. Both Framework.UnitTests and Tasks.UnitTests build cleanly with full nullable analysis on both TFMs. Verified - Build: src/Framework.UnitTests + src/Tasks.UnitTests both produce 0 warnings / 0 errors after the #nullable disable removals. - Tests: WindowsNative_Tests (6), VisualStudioLocationHelper_Tests (6), MetadataReader_Tests (13), AssemblyInformation_Tests (3) all green on net10.0. --- .../FileSystem/WindowsNative_Tests.cs | 2 -- .../VisualStudioLocationHelper_Tests.cs | 2 -- src/Framework/FileSystem/WindowsNative.cs | 23 ++++++++++++++++--- src/Framework/VisualStudioLocationHelper.cs | 17 ++++++++++++-- src/Shared/InprocTrackingNativeMethods.cs | 6 +++++ 5 files changed, 41 insertions(+), 9 deletions(-) diff --git a/src/Framework.UnitTests/FileSystem/WindowsNative_Tests.cs b/src/Framework.UnitTests/FileSystem/WindowsNative_Tests.cs index 3d3045671ab..2d1e6531f9d 100644 --- a/src/Framework.UnitTests/FileSystem/WindowsNative_Tests.cs +++ b/src/Framework.UnitTests/FileSystem/WindowsNative_Tests.cs @@ -10,8 +10,6 @@ using Shouldly; using Xunit; -#nullable disable - namespace Microsoft.Build.UnitTests { /// diff --git a/src/Framework.UnitTests/VisualStudioLocationHelper_Tests.cs b/src/Framework.UnitTests/VisualStudioLocationHelper_Tests.cs index 7b7e7aa48d4..240e46e8c62 100644 --- a/src/Framework.UnitTests/VisualStudioLocationHelper_Tests.cs +++ b/src/Framework.UnitTests/VisualStudioLocationHelper_Tests.cs @@ -10,8 +10,6 @@ using Shouldly; using Xunit; -#nullable disable - namespace Microsoft.Build.UnitTests { /// diff --git a/src/Framework/FileSystem/WindowsNative.cs b/src/Framework/FileSystem/WindowsNative.cs index e1dc5207251..66c01d0af8c 100644 --- a/src/Framework/FileSystem/WindowsNative.cs +++ b/src/Framework/FileSystem/WindowsNative.cs @@ -265,9 +265,26 @@ public static SafeFindFileHandle FindFirstFileW(string lpFileName, out Win32Find [System.Runtime.Versioning.SupportedOSPlatform("windows5.1.2600")] public static bool FindNextFileW(SafeHandle hFindFile, out Win32FindData lpFindFileData) { - bool ok = Windows.Win32.PInvoke.FindNextFile(new Windows.Win32.Foundation.HANDLE((void*)hFindFile.DangerousGetHandle()), out var raw); - lpFindFileData = ConvertFindData(in raw); - return ok; + // DangerousAddRef pins the SafeHandle's ref count for the duration of the + // native call so a concurrent Dispose() cannot invalidate the handle mid-call. + // Today's enumeration callers are single-threaded but the cost is negligible. + bool refAdded = false; + try + { + hFindFile.DangerousAddRef(ref refAdded); + bool ok = Windows.Win32.PInvoke.FindNextFile( + new Windows.Win32.Foundation.HANDLE((void*)hFindFile.DangerousGetHandle()), + out Windows.Win32.Storage.FileSystem.WIN32_FIND_DATAW raw); + lpFindFileData = ConvertFindData(in raw); + return ok; + } + finally + { + if (refAdded) + { + hFindFile.DangerousRelease(); + } + } } /// diff --git a/src/Framework/VisualStudioLocationHelper.cs b/src/Framework/VisualStudioLocationHelper.cs index a2bb197bd39..6e296f22b39 100644 --- a/src/Framework/VisualStudioLocationHelper.cs +++ b/src/Framework/VisualStudioLocationHelper.cs @@ -35,6 +35,10 @@ internal static IList GetInstances() var validInstances = new List(); #if FEATURE_WINDOWSINTEROP && FEATURE_VISUALSTUDIOSETUP + // No try/catch around the COM call chain by design: AcquireSetupConfiguration2 + // and PopulateInstances check HRESULTs inline and return gracefully on failure. + // If a future maintainer introduces a ThrowOnFailure() in this path, add the + // matching catch — "empty list" is the documented behaviour for absent VS. try { using ComScope config = AcquireSetupConfiguration2(); @@ -80,7 +84,10 @@ private static ComScope AcquireSetupConfiguration2() { // Some other COM failure. The caller swallows errors, so signal "no result" // by returning an empty scope rather than constructing a COMException only - // to have it discarded. + // to have it discarded. CoCreateInstance must null the out-parameter on + // failure per the COM spec, but Dispose defensively so the ownership model + // is explicit even against a buggy COM implementation. + config2.Dispose(); return default; } @@ -93,7 +100,13 @@ private static ComScope AcquireSetupConfiguration2() } HRESULT qiHr = config1.Pointer->QueryInterface(&iidConfig2, config2); - return qiHr.Succeeded ? config2 : default; + if (qiHr.Failed) + { + config2.Dispose(); + return default; + } + + return config2; } /// diff --git a/src/Shared/InprocTrackingNativeMethods.cs b/src/Shared/InprocTrackingNativeMethods.cs index 4ca61fc6a78..9121153afb2 100644 --- a/src/Shared/InprocTrackingNativeMethods.cs +++ b/src/Shared/InprocTrackingNativeMethods.cs @@ -243,6 +243,12 @@ private static void LoadFileTrackerDll() // GetProcAddress takes a system-default-code-page (ANSI) string. Encode through // the real encoder so DBCS / multi-byte characters in an unusual export name // are handled correctly. Rent from ArrayPool to avoid a per-call allocation. + // + // Encoding.Default returns the system ANSI code page on .NET Framework + // (correct for GetProcAddress) but UTF-8 on .NET (Core). FEATURE_FILE_TRACKER + // is only defined for net472 so this branch is correct as-is; if the #if + // guard ever broadens to .NET (Core), switch to a CodePagesEncodingProvider + // ANSI encoding instead. Encoding ansi = Encoding.Default; int maxByteCount = ansi.GetMaxByteCount(entryPointName.Length) + 1; byte[] buffer = ArrayPool.Shared.Rent(maxByteCount); From a65acd82edee3e27a417592725b44682f9c06968 Mon Sep 17 00:00:00 2001 From: Jeremy Kuhne Date: Wed, 27 May 2026 10:57:35 -0700 Subject: [PATCH 4/4] Remove leftover empty elements (PR #13872 review nits) --- src/Build/Microsoft.Build.csproj | 2 -- src/Tasks/Microsoft.Build.Tasks.csproj | 2 -- src/Utilities/Microsoft.Build.Utilities.csproj | 3 --- 3 files changed, 7 deletions(-) diff --git a/src/Build/Microsoft.Build.csproj b/src/Build/Microsoft.Build.csproj index af914f03cdd..34effaabafc 100644 --- a/src/Build/Microsoft.Build.csproj +++ b/src/Build/Microsoft.Build.csproj @@ -64,8 +64,6 @@ - - diff --git a/src/Tasks/Microsoft.Build.Tasks.csproj b/src/Tasks/Microsoft.Build.Tasks.csproj index 4b328798ab6..d072cd27b3d 100644 --- a/src/Tasks/Microsoft.Build.Tasks.csproj +++ b/src/Tasks/Microsoft.Build.Tasks.csproj @@ -646,8 +646,6 @@ - - diff --git a/src/Utilities/Microsoft.Build.Utilities.csproj b/src/Utilities/Microsoft.Build.Utilities.csproj index 5a953724e51..a5d1acb9d61 100644 --- a/src/Utilities/Microsoft.Build.Utilities.csproj +++ b/src/Utilities/Microsoft.Build.Utilities.csproj @@ -24,9 +24,6 @@ - - -