From 80392a9aa7a8cc8760e85003f2efd6beb3f081d7 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 16 Aug 2021 08:17:55 +0200 Subject: [PATCH 1/5] don't run LongModuleFileNamesAreSupported test on x86 (for some reason it's flaky) --- .../System.Diagnostics.Process/tests/ProcessModuleTests.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs index 59fae43b84f19..c53a5b6c7b1f2 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs @@ -93,9 +93,10 @@ public void ModulesAreDisposedWhenProcessIsDisposed() } public static bool Is_LongModuleFileNamesAreSupported_TestEnabled - => PathFeatures.AreAllLongPathsAvailable() // we want to test long paths + => OperatingSystem.IsWindows() // it's specific to Windows + && PathFeatures.AreAllLongPathsAvailable() // we want to test long paths && !PlatformDetection.IsMonoRuntime // Assembly.LoadFile used the way this test is implemented fails on Mono - && OperatingSystem.IsWindowsVersionAtLeast(8); // it's specific to Windows and does not work on Windows 7 + && PlatformDetection.Is64BitProcess; // for some reason it's flaky on x86 [ConditionalFact(typeof(ProcessModuleTests), nameof(Is_LongModuleFileNamesAreSupported_TestEnabled))] public void LongModuleFileNamesAreSupported() From 97c28576ad3a98c0b26aee025b6fc1e5c6f4ba03 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 16 Aug 2021 11:46:31 +0200 Subject: [PATCH 2/5] apply code review suggestion --- .../System.Diagnostics.Process/tests/ProcessModuleTests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs index c53a5b6c7b1f2..2000655ef145c 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs @@ -4,6 +4,7 @@ using System.IO; using System.Linq; using System.Reflection; +using System.Runtime.InteropServices; using Microsoft.DotNet.RemoteExecutor; using Xunit; @@ -96,7 +97,7 @@ public static bool Is_LongModuleFileNamesAreSupported_TestEnabled => OperatingSystem.IsWindows() // it's specific to Windows && PathFeatures.AreAllLongPathsAvailable() // we want to test long paths && !PlatformDetection.IsMonoRuntime // Assembly.LoadFile used the way this test is implemented fails on Mono - && PlatformDetection.Is64BitProcess; // for some reason it's flaky on x86 + && RuntimeInformation.ProcessArchitecture != Architecture.X86; // for some reason it's flaky on x86 [ConditionalFact(typeof(ProcessModuleTests), nameof(Is_LongModuleFileNamesAreSupported_TestEnabled))] public void LongModuleFileNamesAreSupported() From ee99091cde1adb3b88a314c6ab58b748f13edaf6 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 16 Aug 2021 16:53:28 +0200 Subject: [PATCH 3/5] ensure type called "Interop" has no namespace, so Interop files from $(CommonPath)Interop\ can be included in the test project --- .../tests/Interop.Unix.cs | 12 +- .../tests/Interop.cs | 212 +++++++++--------- 2 files changed, 108 insertions(+), 116 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/Interop.Unix.cs b/src/libraries/System.Diagnostics.Process/tests/Interop.Unix.cs index ddab95fab7548..f6cb0cedc3e42 100644 --- a/src/libraries/System.Diagnostics.Process/tests/Interop.Unix.cs +++ b/src/libraries/System.Diagnostics.Process/tests/Interop.Unix.cs @@ -1,16 +1,10 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using Microsoft.Win32.SafeHandles; -using System.ComponentModel; using System.Runtime.InteropServices; -using System.Security.Principal; -namespace System.Diagnostics.Tests +internal static partial class Interop { - internal static partial class Interop - { - [DllImport("libc")] - internal static extern int getsid(int pid); - } + [DllImport("libc")] + internal static extern int getsid(int pid); } diff --git a/src/libraries/System.Diagnostics.Process/tests/Interop.cs b/src/libraries/System.Diagnostics.Process/tests/Interop.cs index 0bcbb62f9bb2b..949034c2c482e 100644 --- a/src/libraries/System.Diagnostics.Process/tests/Interop.cs +++ b/src/libraries/System.Diagnostics.Process/tests/Interop.cs @@ -1,145 +1,143 @@ // 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 System.ComponentModel; using System.Runtime.InteropServices; using System.Security.Principal; -namespace System.Diagnostics.Tests +internal static partial class Interop { - internal static partial class Interop + [StructLayout(LayoutKind.Sequential, Size = 40)] + public struct PROCESS_MEMORY_COUNTERS { - [StructLayout(LayoutKind.Sequential, Size = 40)] - public struct PROCESS_MEMORY_COUNTERS - { - public uint cb; - public uint PageFaultCount; - public uint PeakWorkingSetSize; - public uint WorkingSetSize; - public uint QuotaPeakPagedPoolUsage; - public uint QuotaPagedPoolUsage; - public uint QuotaPeakNonPagedPoolUsage; - public uint QuotaNonPagedPoolUsage; - public uint PagefileUsage; - public uint PeakPagefileUsage; - } + public uint cb; + public uint PageFaultCount; + public uint PeakWorkingSetSize; + public uint WorkingSetSize; + public uint QuotaPeakPagedPoolUsage; + public uint QuotaPagedPoolUsage; + public uint QuotaPeakNonPagedPoolUsage; + public uint QuotaNonPagedPoolUsage; + public uint PagefileUsage; + public uint PeakPagefileUsage; + } - [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] - internal struct USER_INFO_1 - { - public string usri1_name; - public string usri1_password; - public uint usri1_password_age; - public uint usri1_priv; - public string usri1_home_dir; - public string usri1_comment; - public uint usri1_flags; - public string usri1_script_path; - } + [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] + internal struct USER_INFO_1 + { + public string usri1_name; + public string usri1_password; + public uint usri1_password_age; + public uint usri1_priv; + public string usri1_home_dir; + public string usri1_comment; + public uint usri1_flags; + public string usri1_script_path; + } - [StructLayout(LayoutKind.Sequential)] - public struct TOKEN_USER - { - public SID_AND_ATTRIBUTES User; - } + [StructLayout(LayoutKind.Sequential)] + public struct TOKEN_USER + { + public SID_AND_ATTRIBUTES User; + } - [StructLayout(LayoutKind.Sequential)] - public struct SID_AND_ATTRIBUTES - { - public IntPtr Sid; - public int Attributes; - } + [StructLayout(LayoutKind.Sequential)] + public struct SID_AND_ATTRIBUTES + { + public IntPtr Sid; + public int Attributes; + } - [DllImport("kernel32.dll")] - public static extern bool GetProcessWorkingSetSizeEx(SafeProcessHandle hProcess, out IntPtr lpMinimumWorkingSetSize, out IntPtr lpMaximumWorkingSetSize, out uint flags); + [DllImport("kernel32.dll")] + public static extern bool GetProcessWorkingSetSizeEx(SafeProcessHandle hProcess, out IntPtr lpMinimumWorkingSetSize, out IntPtr lpMaximumWorkingSetSize, out uint flags); - [DllImport("kernel32.dll")] - internal static extern bool ProcessIdToSessionId(uint dwProcessId, out uint pSessionId); + [DllImport("kernel32.dll")] + internal static extern bool ProcessIdToSessionId(uint dwProcessId, out uint pSessionId); - [DllImport("kernel32.dll")] - public static extern int GetProcessId(SafeProcessHandle nativeHandle); + [DllImport("kernel32.dll")] + public static extern int GetProcessId(SafeProcessHandle nativeHandle); - [DllImport("kernel32.dll")] - internal static extern int GetConsoleCP(); + [DllImport("kernel32.dll")] + internal static extern int GetConsoleCP(); - [DllImport("kernel32.dll")] - internal static extern int GetConsoleOutputCP(); + [DllImport("kernel32.dll")] + internal static extern int GetConsoleOutputCP(); - [DllImport("kernel32.dll")] - internal static extern int SetConsoleCP(int codePage); + [DllImport("kernel32.dll")] + internal static extern int SetConsoleCP(int codePage); - [DllImport("kernel32.dll")] - internal static extern int SetConsoleOutputCP(int codePage); + [DllImport("kernel32.dll")] + internal static extern int SetConsoleOutputCP(int codePage); - [DllImport("netapi32.dll", CharSet = CharSet.Unicode, SetLastError = true)] - internal static extern uint NetUserAdd(string servername, uint level, ref USER_INFO_1 buf, out uint parm_err); + [DllImport("netapi32.dll", CharSet = CharSet.Unicode, SetLastError = true)] + internal static extern uint NetUserAdd(string servername, uint level, ref USER_INFO_1 buf, out uint parm_err); - [DllImport("netapi32.dll", CharSet = CharSet.Unicode)] - internal static extern uint NetUserDel(string servername, string username); + [DllImport("netapi32.dll", CharSet = CharSet.Unicode)] + internal static extern uint NetUserDel(string servername, string username); - [DllImport("advapi32.dll")] - internal static extern bool OpenProcessToken(SafeProcessHandle ProcessHandle, uint DesiredAccess, out SafeProcessHandle TokenHandle); + [DllImport("advapi32.dll")] + internal static extern bool OpenProcessToken(SafeProcessHandle ProcessHandle, uint DesiredAccess, out SafeProcessHandle TokenHandle); - [DllImport("advapi32.dll")] - internal static extern bool GetTokenInformation(SafeProcessHandle TokenHandle, uint TokenInformationClass, IntPtr TokenInformation, int TokenInformationLength, ref int ReturnLength); + [DllImport("advapi32.dll")] + internal static extern bool GetTokenInformation(SafeProcessHandle TokenHandle, uint TokenInformationClass, IntPtr TokenInformation, int TokenInformationLength, ref int ReturnLength); - [DllImport("shell32.dll")] - internal static extern int SHChangeNotify(int eventId, int flags, IntPtr item1, IntPtr item2); + [DllImport("shell32.dll")] + internal static extern int SHChangeNotify(int eventId, int flags, IntPtr item1, IntPtr item2); - internal static void NetUserAdd(string username, string password) - { - USER_INFO_1 userInfo = new USER_INFO_1(); - userInfo.usri1_name = username; - userInfo.usri1_password = password; - userInfo.usri1_priv = 1; + internal static void NetUserAdd(string username, string password) + { + USER_INFO_1 userInfo = new USER_INFO_1(); + userInfo.usri1_name = username; + userInfo.usri1_password = password; + userInfo.usri1_priv = 1; - uint parm_err; - uint result = NetUserAdd(null, 1, ref userInfo, out parm_err); + uint parm_err; + uint result = NetUserAdd(null, 1, ref userInfo, out parm_err); - if (result != ExitCodes.NERR_Success) - { - // most likely result == ERROR_ACCESS_DENIED - // due to running without elevated privileges - throw new Win32Exception((int)result); - } + if (result != ExitCodes.NERR_Success) + { + // most likely result == ERROR_ACCESS_DENIED + // due to running without elevated privileges + throw new Win32Exception((int)result); } + } - internal static bool ProcessTokenToSid(SafeProcessHandle token, out SecurityIdentifier sid) + internal static bool ProcessTokenToSid(SafeProcessHandle token, out SecurityIdentifier sid) + { + bool ret = false; + sid = null; + IntPtr tu = IntPtr.Zero; + try { - bool ret = false; - sid = null; - IntPtr tu = IntPtr.Zero; - try - { - TOKEN_USER tokUser; - const int bufLength = 256; - - tu = Marshal.AllocHGlobal(bufLength); - int cb = bufLength; - ret = GetTokenInformation(token, 1, tu, cb, ref cb); - if (ret) - { - tokUser = Marshal.PtrToStructure(tu); - sid = new SecurityIdentifier(tokUser.User.Sid); - } - return ret; - } - catch (Exception) - { - return false; - } - finally + TOKEN_USER tokUser; + const int bufLength = 256; + + tu = Marshal.AllocHGlobal(bufLength); + int cb = bufLength; + ret = GetTokenInformation(token, 1, tu, cb, ref cb); + if (ret) { - if (tu != IntPtr.Zero) - Marshal.FreeHGlobal(tu); + tokUser = Marshal.PtrToStructure(tu); + sid = new SecurityIdentifier(tokUser.User.Sid); } + return ret; } - - internal static class ExitCodes + catch (Exception) + { + return false; + } + finally { - internal const uint NERR_Success = 0; - internal const uint NERR_UserNotFound = 2221; + if (tu != IntPtr.Zero) + Marshal.FreeHGlobal(tu); } } + + internal static class ExitCodes + { + internal const uint NERR_Success = 0; + internal const uint NERR_UserNotFound = 2221; + } } From 14c02bc8c4a2523bbd5cc2ce83a57d06e438d4d7 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 16 Aug 2021 16:54:40 +0200 Subject: [PATCH 4/5] try JanK suggestion and use LoadLibrary instead of Assembly.LoadFile --- .../tests/ProcessModuleTests.Windows.cs | 50 +++++++++++++++++++ .../tests/ProcessModuleTests.cs | 41 +-------------- .../System.Diagnostics.Process.Tests.csproj | 9 +++- 3 files changed, 59 insertions(+), 41 deletions(-) create mode 100644 src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.Windows.cs diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.Windows.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.Windows.cs new file mode 100644 index 0000000000000..79cbe0ea8e531 --- /dev/null +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.Windows.cs @@ -0,0 +1,50 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.IO; +using System.Linq; +using System.Runtime.InteropServices; +using Xunit; + +namespace System.Diagnostics.Tests +{ + public partial class ProcessModuleTests : ProcessTestBase + { + [ConditionalFact(typeof(PathFeatures), nameof(PathFeatures.AreAllLongPathsAvailable))] + public void LongModuleFileNamesAreSupported() + { + // To be able to test Long Path support for ProcessModule.FileName we need a .dll that has a path > 260 chars. + // Since Long Paths support can be disabled (see the ConditionalFact attribute usage above), + // we just copy "LongName.dll" from bin to a temp directory with a long name and load it from there. + // Loading from new path is possible because the type exposed by the assembly is not referenced in any explicit way. + const string libraryName = "LongPath.dll"; + const int minPathLength = 261; + + string testBinPath = Path.GetDirectoryName(typeof(ProcessModuleTests).Assembly.Location); + string libraryToCopy = Path.Combine(testBinPath, libraryName); + Assert.True(File.Exists(libraryToCopy), $"{libraryName} was not present in bin folder '{testBinPath}'"); + + string directoryWithLongName = Path.Combine(TestDirectory, new string('a', Math.Max(1, minPathLength - TestDirectory.Length))); + Directory.CreateDirectory(directoryWithLongName); + + string longNamePath = Path.Combine(directoryWithLongName, libraryName); + Assert.True(longNamePath.Length > minPathLength); + + File.Copy(libraryToCopy, longNamePath); + Assert.True(File.Exists(longNamePath)); + + IntPtr moduleHandle = Interop.Kernel32.LoadLibrary(longNamePath); + Assert.False(moduleHandle == IntPtr.Zero, $"LoadLibrary failed with {Marshal.GetLastWin32Error()}"); + + try + { + string[] modulePaths = Process.GetCurrentProcess().Modules.Cast().Select(module => module.FileName).ToArray(); + Assert.Contains(longNamePath, modulePaths); + } + finally + { + Interop.Kernel32.FreeLibrary(moduleHandle); + } + } + } +} diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs index 2000655ef145c..33f84717da719 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs @@ -1,16 +1,13 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.IO; using System.Linq; -using System.Reflection; -using System.Runtime.InteropServices; using Microsoft.DotNet.RemoteExecutor; using Xunit; namespace System.Diagnostics.Tests { - public class ProcessModuleTests : ProcessTestBase + public partial class ProcessModuleTests : ProcessTestBase { [Fact] public void TestModuleProperties() @@ -92,41 +89,5 @@ public void ModulesAreDisposedWhenProcessIsDisposed() process.Dispose(); Assert.Equal(expectedCount, disposedCount); } - - public static bool Is_LongModuleFileNamesAreSupported_TestEnabled - => OperatingSystem.IsWindows() // it's specific to Windows - && PathFeatures.AreAllLongPathsAvailable() // we want to test long paths - && !PlatformDetection.IsMonoRuntime // Assembly.LoadFile used the way this test is implemented fails on Mono - && RuntimeInformation.ProcessArchitecture != Architecture.X86; // for some reason it's flaky on x86 - - [ConditionalFact(typeof(ProcessModuleTests), nameof(Is_LongModuleFileNamesAreSupported_TestEnabled))] - public void LongModuleFileNamesAreSupported() - { - // To be able to test Long Path support for ProcessModule.FileName we need a .dll that has a path > 260 chars. - // Since Long Paths support can be disabled (see the ConditionalFact attribute usage above), - // we just copy "LongName.dll" from bin to a temp directory with a long name and load it from there. - // Loading from new path is possible because the type exposed by the assembly is not referenced in any explicit way. - const string libraryName = "LongPath.dll"; - const int minPathLength = 261; - - string testBinPath = Path.GetDirectoryName(typeof(ProcessModuleTests).Assembly.Location); - string libraryToCopy = Path.Combine(testBinPath, libraryName); - Assert.True(File.Exists(libraryToCopy), $"{libraryName} was not present in bin folder '{testBinPath}'"); - - string directoryWithLongName = Path.Combine(TestDirectory, new string('a', Math.Max(1, minPathLength - TestDirectory.Length))); - Directory.CreateDirectory(directoryWithLongName); - - string longNamePath = Path.Combine(directoryWithLongName, libraryName); - Assert.True(longNamePath.Length > minPathLength); - - File.Copy(libraryToCopy, longNamePath); - Assert.True(File.Exists(longNamePath)); - - Assembly loaded = Assembly.LoadFile(longNamePath); - Assert.Equal(longNamePath, loaded.Location); - - string[] modulePaths = Process.GetCurrentProcess().Modules.Cast().Select(module => module.FileName).ToArray(); - Assert.Contains(longNamePath, modulePaths); - } } } diff --git a/src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj b/src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj index 776b69684d3d7..bfcfb2cc3b71c 100644 --- a/src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj +++ b/src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj @@ -1,4 +1,4 @@ - + true $(DefineConstants);TargetsWindows @@ -37,10 +37,17 @@ + + + + From 487748df5daffbcaa3f2101dbd724ebd88d16eab Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 16 Aug 2021 20:11:44 +0200 Subject: [PATCH 5/5] don't try to check module file path if we can't load the module itself (x86) --- .../tests/ProcessModuleTests.Windows.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.Windows.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.Windows.cs index 79cbe0ea8e531..ca5360ef66a71 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.Windows.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.Windows.cs @@ -34,7 +34,12 @@ public void LongModuleFileNamesAreSupported() Assert.True(File.Exists(longNamePath)); IntPtr moduleHandle = Interop.Kernel32.LoadLibrary(longNamePath); - Assert.False(moduleHandle == IntPtr.Zero, $"LoadLibrary failed with {Marshal.GetLastWin32Error()}"); + if (moduleHandle == IntPtr.Zero) + { + Assert.Equal(126, Marshal.GetLastWin32Error()); // ERROR_MOD_NOT_FOUND + Assert.Equal(Architecture.X86, RuntimeInformation.ProcessArchitecture); + return; // we have failed to load the module on x86, we skip the rest of the test (it's best effort) + } try {