diff --git a/src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerProcess.cs b/src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerProcess.cs index 0b45971e1c..041e28fa77 100644 --- a/src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerProcess.cs +++ b/src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerProcess.cs @@ -84,8 +84,8 @@ public int ProcessId /// Launches the MTP application at and waits for it to connect back. /// /// - /// Path to the test application. May be a managed .dll (launched via its sibling apphost - /// .exe when present, otherwise via dotnet <dll>) or a native .exe. + /// Path to the test application. May be a managed .dll (launched via its sibling apphost when that + /// apphost is usable on the current OS, otherwise via dotnet <dll>) or a native executable. /// /// Client options (name, connection timeout, environment, logger). public static MtpServerProcess Start(string source, MtpServerClientOptions? options = null) @@ -284,17 +284,19 @@ private static string GetStandardError(StringBuilder buffer) } } - private static LaunchCommand BuildLaunch(string source, int port) + internal static LaunchCommand BuildLaunch(string source, int port) { string serverArgs = $"{ServerArgument} {ClientPortArgument} {port} {NoBannerArgument}"; string workingDirectory = Path.GetDirectoryName(source) ?? Directory.GetCurrentDirectory(); string extension = Path.GetExtension(source); // A managed .NET assembly must be launched through its apphost (preferred) or `dotnet `. + // The apphost is only preferred when it is actually launchable here: a candidate that merely + // exists is not enough (see IsUsableApphost), and an unusable one falls back to `dotnet `. if (extension.Equals(".dll", StringComparison.OrdinalIgnoreCase)) { string apphost = GetAppHostPath(source); - return File.Exists(apphost) + return IsUsableApphost(apphost) ? new LaunchCommand(apphost, serverArgs, workingDirectory) : new LaunchCommand("dotnet", $"\"{source}\" {serverArgs}", workingDirectory); } @@ -307,7 +309,7 @@ private static LaunchCommand BuildLaunch(string source, int port) // A named launch descriptor rather than a value tuple: System.ValueTuple is not in the .NET // Framework before 4.7, and this source is compiled into consumers that may target net462 without // referencing the System.ValueTuple package. A tiny class keeps the package dependency-free. - private sealed class LaunchCommand + internal sealed class LaunchCommand { public LaunchCommand(string fileName, string arguments, string workingDirectory) { @@ -335,12 +337,55 @@ private static string GetAppHostPath(string managedAssembly) #else bool isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); #endif + + // The probe is OS-aware rather than always appending ".exe": a test payload built on a Windows + // agent and executed on a Linux machine (the dotnet/aspnetcore Helix layout) ships a Windows PE + // `Foo.exe` next to `Foo.dll`. Probing for ".exe" on Linux finds that Windows binary and launching + // it aborts the run, which is the CI failure fixed in microsoft/vstest#16336. A Unix apphost has + // no extension, so asking for the right name per OS never selects the foreign one. string appHostFileName = isWindows ? nameWithoutExtension + ".exe" : nameWithoutExtension; return Path.Combine(directory, appHostFileName); } + /// + /// Determines whether can actually be launched on the current operating system. + /// + /// + /// Existence is not sufficient on Unix. Archive formats used to move test payloads between agents (zip in + /// particular) do not carry the POSIX permission bits, so an extensionless apphost that survives a + /// Windows-build/Linux-run round trip can arrive without its execute bit. Launching such a file throws + /// Permission denied instead of degrading, which is the second half of the fix in + /// microsoft/vstest#16336. Requiring an execute bit lets the caller fall back to dotnet <dll>, + /// which needs no permissions on the apphost at all. + /// + internal static bool IsUsableApphost(string apphost) + { + if (!File.Exists(apphost)) + { + return false; + } + +#if NET7_0_OR_GREATER + // Fenced on the target framework because File.GetUnixFileMode is .NET 7+. Deliberately not the + // package's modern-.NET compilation symbol: that one records which JSON slice a consumer compiles + // and is defined only for net8.0+, while NuGet serves the net5.0 slice to net5.0, net6.0 and net7.0 + // consumers alike. Fencing on it would drop a Linux net7.0 consumer back to the existence-only path + // even though it has the API. net462, netstandard2.0, net5.0 and net6.0 consumers genuinely lack the + // API and keep the existence check above, which stays correct because GetAppHostPath already refuses + // to consider a Windows ".exe" on Unix, so the only case left uncovered there is an extensionless + // file that lost its execute bit. + if (!OperatingSystem.IsWindows()) + { + const UnixFileMode ExecuteBits = UnixFileMode.UserExecute | UnixFileMode.GroupExecute | UnixFileMode.OtherExecute; + return (File.GetUnixFileMode(apphost) & ExecuteBits) != 0; + } +#endif + + return true; + } + private static void SafeStop(TcpListener listener, IMtpClientLogger logger) { try diff --git a/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MtpServerClientSourcePackageTests.cs b/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MtpServerClientSourcePackageTests.cs index cf5b6340d0..5df8f24676 100644 --- a/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MtpServerClientSourcePackageTests.cs +++ b/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MtpServerClientSourcePackageTests.cs @@ -337,6 +337,56 @@ public void SourcePackage_ShipsBuildTargets_AndNet462SafetyGuardsSurviveTransfor string.Join(Environment.NewLine, missingGuard)); } + [TestMethod] + public void SourcePackage_UnixApphostExecuteBitCheck_IsFencedOnTargetFramework_NotOnTheModernDotnetSymbol() + { + // File.GetUnixFileMode is .NET 7+, so the apphost execute-bit check is fenced on NET7_0_OR_GREATER, + // which the pack transform leaves alone and every net7.0+ consumer defines. It must NOT ride on + // MTP_CLIENT_USE_MODERN_DOTNET (what the transform makes of NETCOREAPP): that symbol records which + // JSON slice the consumer compiles and is defined only for net8.0+. NuGet serves the net5.0 slice to + // net5.0, net6.0 and net7.0 consumers, so reusing it here would drop a Linux net7.0 consumer back to + // the existence-only check, select a non-executable apphost, and abort with 'Permission denied' + // instead of falling back to `dotnet `. + const string Logical = "Client/MtpServerProcess.cs"; + const string Fence = "#if NET7_0_OR_GREATER"; + const string GuardedCall = "File.GetUnixFileMode("; + + List offenders = []; + foreach (string tfm in Package.TargetFrameworks) + { + if (!Package.PackedTextByTfm[tfm].TryGetValue(Logical, out string? text)) + { + offenders.Add($"{tfm}: '{Logical}' is not packed."); + continue; + } + + int call = text.IndexOf(GuardedCall, StringComparison.Ordinal); + if (call < 0) + { + offenders.Add($"{tfm}: no '{GuardedCall}' call, so the Unix apphost execute-bit check is gone."); + continue; + } + + int fence = text.IndexOf(Fence, StringComparison.Ordinal); + if (fence < 0 || call < fence) + { + offenders.Add($"{tfm}: '{GuardedCall}' is not fenced on '{Fence}'."); + continue; + } + + int endIf = text.IndexOf("#endif", fence, StringComparison.Ordinal); + if (endIf >= 0 && call > endIf) + { + offenders.Add($"{tfm}: '{GuardedCall}' sits outside the '{Fence}' block."); + } + } + + Assert.IsEmpty( + offenders, + $"Every consumer that has File.GetUnixFileMode must require an execute bit before selecting a Unix apphost:{Environment.NewLine}" + + string.Join(Environment.NewLine, offenders)); + } + [TestMethod] public void SourcePackage_JsoniteNamespace_IsPackageQualified_NotTopLevel() { diff --git a/test/UnitTests/Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests/MtpServerProcessTests.cs b/test/UnitTests/Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests/MtpServerProcessTests.cs new file mode 100644 index 0000000000..412882e885 --- /dev/null +++ b/test/UnitTests/Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests/MtpServerProcessTests.cs @@ -0,0 +1,203 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests; + +/// +/// Tests for how chooses between a sibling apphost and dotnet <dll>. +/// +/// +/// The scenario these guard is a test payload built on a Windows agent and executed on a Linux machine. Two +/// things go wrong there: a Windows PE App.exe travels next to App.dll and is not a Linux +/// executable, and a zip round trip does not carry POSIX permission bits, so the real extensionless apphost +/// can arrive without its execute bit. Launching either one aborts the run with Permission denied, so +/// the launcher has to reject both and fall back to dotnet <dll>. +/// +[TestClass] +public sealed class MtpServerProcessTests +{ + // Any value works: BuildLaunch only formats the port into the argument string, it never binds it. + private const int Port = 12345; + + [TestMethod] + public void BuildLaunchWhenSourceIsExeLaunchesItDirectly() + { + using var temp = TempDirectory.Create(); + string exe = temp.CreateFile("App.exe"); + + MtpServerProcess.LaunchCommand launch = MtpServerProcess.BuildLaunch(exe, Port); + + Assert.AreEqual(exe, launch.FileName, "A native executable source must be launched directly."); + Assert.AreEqual(temp.Path, launch.WorkingDirectory); + Assert.Contains("--server", launch.Arguments); + Assert.Contains($"--client-port {Port}", launch.Arguments); + Assert.Contains("--no-banner", launch.Arguments); + } + + [TestMethod] + public void BuildLaunchWhenDllHasNoApphostFallsBackToDotnet() + { + using var temp = TempDirectory.Create(); + string dll = temp.CreateFile("App.dll"); + + MtpServerProcess.LaunchCommand launch = MtpServerProcess.BuildLaunch(dll, Port); + + Assert.AreEqual("dotnet", launch.FileName, "Without an apphost the assembly must be launched by the muxer."); + Assert.Contains($"\"{dll}\"", launch.Arguments, "The muxer needs the quoted assembly path as its first argument."); + Assert.AreEqual(temp.Path, launch.WorkingDirectory); + } + + [TestMethod] + [OSCondition(ConditionMode.Include, OperatingSystems.Windows, IgnoreMessage = "A '.exe' apphost is only launchable on Windows.")] + public void BuildLaunchOnWindowsSelectsSiblingExeApphost() + { + using var temp = TempDirectory.Create(); + string dll = temp.CreateFile("App.dll"); + string exe = temp.CreateFile("App.exe"); + + MtpServerProcess.LaunchCommand launch = MtpServerProcess.BuildLaunch(dll, Port); + + Assert.AreEqual(exe, launch.FileName, "On Windows the sibling '.exe' apphost is the preferred launch target."); + } + + [TestMethod] + [OSCondition(ConditionMode.Exclude, OperatingSystems.Windows, IgnoreMessage = "Asserts that a Windows PE sibling is rejected, which only matters off Windows.")] + public void BuildLaunchOnUnixIgnoresSiblingWindowsExeAndFallsBackToDotnet() + { + using var temp = TempDirectory.Create(); + string dll = temp.CreateFile("App.dll"); + + // The Windows apphost that rode along in the payload. It exists, but it is a Windows PE binary. + _ = temp.CreateFile("App.exe"); + + MtpServerProcess.LaunchCommand launch = MtpServerProcess.BuildLaunch(dll, Port); + + Assert.AreEqual("dotnet", launch.FileName, "A Windows '.exe' must never be selected as the apphost on Unix."); + } + +#if NET + [TestMethod] + [OSCondition(ConditionMode.Exclude, OperatingSystems.Windows, IgnoreMessage = "Unix file modes are a Unix concept.")] + [UnsupportedOSPlatform("windows")] + public void BuildLaunchOnUnixSelectsExecutableExtensionlessApphost() + { + using var temp = TempDirectory.Create(); + string dll = temp.CreateFile("App.dll"); + string apphost = temp.CreateFile("App"); + MakeExecutable(apphost); + + MtpServerProcess.LaunchCommand launch = MtpServerProcess.BuildLaunch(dll, Port); + + Assert.AreEqual(apphost, launch.FileName, "An executable extensionless apphost is the preferred launch target on Unix."); + } + + [TestMethod] + [OSCondition(ConditionMode.Exclude, OperatingSystems.Windows, IgnoreMessage = "Unix file modes are a Unix concept.")] + [UnsupportedOSPlatform("windows")] + public void BuildLaunchOnUnixIgnoresNonExecutableApphostAndFallsBackToDotnet() + { + using var temp = TempDirectory.Create(); + string dll = temp.CreateFile("App.dll"); + + // The apphost survived the trip but the archive dropped its permission bits. + string apphost = temp.CreateFile("App"); + File.SetUnixFileMode(apphost, UnixFileMode.UserRead | UnixFileMode.UserWrite); + + MtpServerProcess.LaunchCommand launch = MtpServerProcess.BuildLaunch(dll, Port); + + Assert.AreEqual( + "dotnet", + launch.FileName, + "Starting a file with no execute bit throws 'Permission denied', so the launch must fall back to the muxer."); + } +#endif + + [TestMethod] + public void IsUsableApphostReturnsFalseWhenFileMissing() + { + using var temp = TempDirectory.Create(); + + Assert.IsFalse(MtpServerProcess.IsUsableApphost(Path.Combine(temp.Path, "Missing"))); + } + + [TestMethod] + [OSCondition(ConditionMode.Include, OperatingSystems.Windows, IgnoreMessage = "Windows has no execute bit, so this asserts the Windows-only branch.")] + public void IsUsableApphostOnWindowsReturnsTrueForExistingFile() + { + using var temp = TempDirectory.Create(); + string apphost = temp.CreateFile("App.exe"); + + Assert.IsTrue(MtpServerProcess.IsUsableApphost(apphost), "Windows has no execute bit, so existence is the whole check there."); + } + +#if NET + [TestMethod] + [OSCondition(ConditionMode.Exclude, OperatingSystems.Windows, IgnoreMessage = "Unix file modes are a Unix concept.")] + [UnsupportedOSPlatform("windows")] + public void IsUsableApphostOnUnixReturnsFalseForNonExecutableFile() + { + using var temp = TempDirectory.Create(); + string apphost = temp.CreateFile("App"); + File.SetUnixFileMode(apphost, UnixFileMode.UserRead | UnixFileMode.UserWrite); + + Assert.IsFalse(MtpServerProcess.IsUsableApphost(apphost)); + } + + [TestMethod] + [OSCondition(ConditionMode.Exclude, OperatingSystems.Windows, IgnoreMessage = "Unix file modes are a Unix concept.")] + [UnsupportedOSPlatform("windows")] + public void IsUsableApphostOnUnixReturnsTrueForExecutableFile() + { + using var temp = TempDirectory.Create(); + string apphost = temp.CreateFile("App"); + MakeExecutable(apphost); + + Assert.IsTrue(MtpServerProcess.IsUsableApphost(apphost)); + } + + /// + /// Grants the owner execute permission only, so the test also proves the check does not require all three + /// execute bits. + /// + [UnsupportedOSPlatform("windows")] + private static void MakeExecutable(string path) + => File.SetUnixFileMode(path, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); +#endif + + private sealed class TempDirectory : IDisposable + { + private TempDirectory(string path) => Path = path; + + public string Path { get; } + + public static TempDirectory Create() + { + string path = System.IO.Path.Combine(System.IO.Path.GetTempPath(), $"mtp-apphost-{Guid.NewGuid():N}"); + _ = Directory.CreateDirectory(path); + return new TempDirectory(path); + } + + /// + /// Creates an empty file in the directory and returns its full path. The content is irrelevant: the + /// launcher only looks at the path, its existence, and its permissions. + /// + public string CreateFile(string fileName) + { + string fullPath = System.IO.Path.Combine(Path, fileName); + File.WriteAllText(fullPath, string.Empty); + return fullPath; + } + + public void Dispose() + { + try + { + Directory.Delete(Path, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup: a temp directory left behind must never fail a test. + } + } + } +}