From 44eb20dc8f71d84f604c41f0f7e3a900d5e1ec59 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 31 Oct 2016 09:41:13 -0700 Subject: [PATCH] Fixes #2534 by replacing expensive WMI query with Win32 API calls (#2535) * Fixes #2534 by replacing expensive WMI query with Win32 API calls * fix break on unix build * added tests for #2535 * although test passed, fixing exception that shows up * fixed Describe text * addressing code review feedback * addressing review feedback to comment on why sleep is needed added check that test processes are created before we try to kill them * fixed test to timeout and pending fix for #2561 --- build.psm1 | 16 ++-- .../CoreCLR/CorePsStub.cs | 25 ++++++ .../utils/PInvokeDllNames.cs | 6 ++ .../utils/PlatformInvokes.cs | 62 +++++++++++++ .../utils/PsUtils.cs | 89 ++++++++++--------- .../NativeCommandProcessor.Tests.ps1 | 55 ++++++++++++ .../CreateChildProcess/CreateChildProcess.cs | 25 ++++++ test/tools/CreateChildProcess/project.json | 28 ++++++ 8 files changed, 261 insertions(+), 45 deletions(-) create mode 100644 test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 create mode 100644 test/tools/CreateChildProcess/CreateChildProcess.cs create mode 100644 test/tools/CreateChildProcess/project.json diff --git a/build.psm1 b/build.psm1 index 3c2393019c..0425324ea3 100644 --- a/build.psm1 +++ b/build.psm1 @@ -597,13 +597,19 @@ function Publish-PSTestTools { Find-Dotnet + $tools = "$PSScriptRoot/test/tools/EchoArgs","$PSScriptRoot/test/tools/CreateChildProcess" # Publish EchoArgs so it can be run by tests - Push-Location "$PSScriptRoot/test/tools/EchoArgs" - try { - dotnet publish --output bin - } finally { - Pop-Location + + foreach ($tool in $tools) + { + Push-Location $tool + try { + dotnet publish --output bin + } finally { + Pop-Location + } } + } function Start-PSPester { diff --git a/src/System.Management.Automation/CoreCLR/CorePsStub.cs b/src/System.Management.Automation/CoreCLR/CorePsStub.cs index dd67a97f8f..faf91ed3c9 100644 --- a/src/System.Management.Automation/CoreCLR/CorePsStub.cs +++ b/src/System.Management.Automation/CoreCLR/CorePsStub.cs @@ -502,6 +502,31 @@ namespace Microsoft.PowerShell.CoreClr.Stubs } } + /// + /// Stub for SafeHandleMinusOneIsInvalid + /// + public abstract class SafeHandleMinusOneIsInvalid : SafeHandle + { + /// + /// Constructor + /// + protected SafeHandleMinusOneIsInvalid(bool ownsHandle) + : base(new IntPtr(-1), ownsHandle) + { + } + + /// + /// IsInvalid + /// + public override bool IsInvalid + { + get + { + return handle == new IntPtr(-1); + } + } + } + #endregion SafeHandle_Related #region Misc_Types diff --git a/src/System.Management.Automation/utils/PInvokeDllNames.cs b/src/System.Management.Automation/utils/PInvokeDllNames.cs index 11df77185e..54a46aef18 100644 --- a/src/System.Management.Automation/utils/PInvokeDllNames.cs +++ b/src/System.Management.Automation/utils/PInvokeDllNames.cs @@ -132,6 +132,9 @@ namespace System.Management.Automation internal const string ReadConsoleInputDllName = "api-ms-win-core-console-l1-1-0.dll"; /*117*/ internal const string GetVersionExDllName = "api-ms-win-core-sysinfo-l1-1-0.dll"; /*118*/ internal const string FormatMessageDllName = "api-ms-win-core-localization-l1-2-0.dll"; /*119*/ + internal const string CreateToolhelp32SnapshotDllName = "api-ms-win-core-toolhelp-l1-1-0"; /*120*/ + internal const string Process32FirstDllName = "api-ms-win-core-toolhelp-l1-1-0"; /*121*/ + internal const string Process32NextDllName = "api-ms-win-core-toolhelp-l1-1-0"; /*122*/ #else internal const string QueryDosDeviceDllName = "kernel32.dll"; /*1*/ internal const string CreateSymbolicLinkDllName = "kernel32.dll"; /*2*/ @@ -251,6 +254,9 @@ namespace System.Management.Automation internal const string ReadConsoleInputDllName = "kernel32.dll"; /*117*/ internal const string GetVersionExDllName = "kernel32.dll"; /*118*/ internal const string FormatMessageDllName = "wevtapi.dll"; /*119*/ + internal const string CreateToolhelp32SnapshotDllName = "kernel32.dll"; /*120*/ + internal const string Process32FirstDllName = "kernel32.dll"; /*121*/ + internal const string Process32NextDllName = "kernel32.dll"; /*122*/ #endif } } diff --git a/src/System.Management.Automation/utils/PlatformInvokes.cs b/src/System.Management.Automation/utils/PlatformInvokes.cs index 95abb623d3..6416092eed 100644 --- a/src/System.Management.Automation/utils/PlatformInvokes.cs +++ b/src/System.Management.Automation/utils/PlatformInvokes.cs @@ -736,6 +736,68 @@ namespace System.Management.Automation [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)] public static extern IntPtr GetStdHandle(uint handleId); +#endif + + #endregion + + #region CreateToolhelp32Snapshot + +#if !UNIX + + [DllImport(PinvokeDllNames.CreateToolhelp32SnapshotDllName, SetLastError = true)] + internal static extern SafeSnapshotHandle CreateToolhelp32Snapshot(SnapshotFlags flags, uint id); + [DllImport(PinvokeDllNames.Process32FirstDllName, SetLastError = true)] + internal static extern bool Process32First(SafeSnapshotHandle hSnapshot, ref PROCESSENTRY32 lppe); + [DllImport(PinvokeDllNames.Process32NextDllName, SetLastError = true)] + internal static extern bool Process32Next(SafeSnapshotHandle hSnapshot, ref PROCESSENTRY32 lppe); + + internal sealed class SafeSnapshotHandle : SafeHandleMinusOneIsInvalid + { + internal SafeSnapshotHandle() : base(true) + { + } + + [SecurityPermission(SecurityAction.LinkDemand, UnmanagedCode = true)] + internal SafeSnapshotHandle(IntPtr handle) : base(true) + { + base.SetHandle(handle); + } + + protected override bool ReleaseHandle() + { + return CloseHandle(base.handle); + } + } + + [Flags] + internal enum SnapshotFlags : uint + { + HeapList = 0x00000001, + Process = 0x00000002, + Thread = 0x00000004, + Module = 0x00000008, + Module32 = 0x00000010, + All = (HeapList | Process | Thread | Module), + Inherit = 0x80000000, + NoHeaps = 0x40000000 + } + [StructLayout(LayoutKind.Sequential)] + internal struct PROCESSENTRY32 + { + public uint dwSize; + public uint cntUsage; + public uint th32ProcessID; + public IntPtr th32DefaultHeapID; + public uint th32ModuleID; + public uint cntThreads; + public uint th32ParentProcessID; + public int pcPriClassBase; + public uint dwFlags; + [MarshalAs(UnmanagedType.ByValTStr, SizeConst = 260)] public string szExeFile; + }; + + internal const int ERROR_NO_MORE_FILES = 0x12; + #endif #endregion diff --git a/src/System.Management.Automation/utils/PsUtils.cs b/src/System.Management.Automation/utils/PsUtils.cs index 33bfeaa722..c06ab86345 100644 --- a/src/System.Management.Automation/utils/PsUtils.cs +++ b/src/System.Management.Automation/utils/PsUtils.cs @@ -4,7 +4,6 @@ Copyright (c) Microsoft Corporation. All rights reserved. using System.Collections; using System.Diagnostics; -using System.Globalization; using System.IO; using System.Linq; using System.Reflection; @@ -16,7 +15,10 @@ using System.Xml; using Microsoft.Win32; using System.Collections.Generic; using System.Management.Automation.Language; -using Microsoft.Management.Infrastructure; +#if CORECLR +// Use stubs for SerializableAttribute, SecurityPermissionAttribute, ReliabilityContractAttribute and ISerializable related types. +using Microsoft.PowerShell.CoreClr.Stubs; +#endif namespace System.Management.Automation { @@ -91,57 +93,64 @@ namespace System.Management.Automation /// /// Retrieve the parent process of a process. /// - /// This is an extremely expensive operation, as WMI - /// needs to work with an ugly Win32 API. The Win32 API - /// creates a snapshot of every process in the system, which - /// you then need to iterate through to find your process and - /// its parent PID. - /// - /// Also, since this is PID based, this API is only reliable - /// when the process has not yet exited. + /// Previously this code used WMI, but WMI is causing a CPU spike whenever the query gets called as it results in + /// tzres.dll and tzres.mui.dll being loaded into every process to conver the time information to local format. + /// For perf reasons, we result to P/Invoke. /// /// /// The process we want to find the /// parent of internal static Process GetParentProcess(Process current) { - string wmiQuery = String.Format(CultureInfo.CurrentCulture, - "Select * From Win32_Process Where Handle='{0}'", - current.Id); + int parentPid = 0; - using (CimSession cimSession = CimSession.Create(null)) +#if !UNIX + PlatformInvokes.PROCESSENTRY32 pe32 = new PlatformInvokes.PROCESSENTRY32 { }; + pe32.dwSize = (uint)ClrFacade.SizeOf(); + + using (PlatformInvokes.SafeSnapshotHandle hSnapshot = PlatformInvokes.CreateToolhelp32Snapshot(PlatformInvokes.SnapshotFlags.Process, (uint)current.Id)) { - IEnumerable processCollection = - cimSession.QueryInstances("root/cimv2", "WQL", wmiQuery); - - int parentPid = - processCollection.Select( - cimProcess => - Convert.ToInt32(cimProcess.CimInstanceProperties["ParentProcessId"].Value, - CultureInfo.CurrentCulture)).FirstOrDefault(); - - if (parentPid == 0) - return null; - - try + if (!PlatformInvokes.Process32First(hSnapshot, ref pe32)) { - Process returnProcess = Process.GetProcessById(parentPid); - - // Ensure the process started before the current - // process, as it could have gone away and had the - // PID recycled. - if (returnProcess.StartTime <= current.StartTime) - return returnProcess; - else + int errno = Marshal.GetLastWin32Error(); + if (errno == PlatformInvokes.ERROR_NO_MORE_FILES) + { return null; + } } - catch (ArgumentException) + do { - // GetProcessById throws an ArgumentException when - // you reach the top of the chain -- Explorer.exe - // has a parent process, but you cannot retrieve it. + if (pe32.th32ProcessID == (uint)current.Id) + { + parentPid = (int)pe32.th32ParentProcessID; + break; + } + + } while (PlatformInvokes.Process32Next(hSnapshot, ref pe32)); + } +#endif + + if (parentPid == 0) + return null; + + try + { + Process returnProcess = Process.GetProcessById(parentPid); + + // Ensure the process started before the current + // process, as it could have gone away and had the + // PID recycled. + if (returnProcess.StartTime <= current.StartTime) + return returnProcess; + else return null; - } + } + catch (ArgumentException) + { + // GetProcessById throws an ArgumentException when + // you reach the top of the chain -- Explorer.exe + // has a parent process, but you cannot retrieve it. + return null; } } diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 new file mode 100644 index 0000000000..d39fb55f2f --- /dev/null +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 @@ -0,0 +1,55 @@ +Describe "Native Command Processor" -tags "Feature" { + + BeforeAll { + # Find where test/powershell is so we can find the createchildprocess command relative to it + $powershellTestDir = $PSScriptRoot + while ($powershellTestDir -notmatch 'test[\\/]powershell$') { + $powershellTestDir = Split-Path $powershellTestDir + } + $createchildprocess = Join-Path (Split-Path $powershellTestDir) tools/CreateChildProcess/bin/createchildprocess + } + + # If powershell receives a StopProcessing, it should kill the native process and all child processes + + # this test should pass and no longer Penidng when #2561 is fixed + It "Should kill native process tree" { + + Test-Path $createchildprocess | Should Be $true + + # make sure no test processes are running + # on Linux, the Process class truncates the name so filter using Where-Object + Get-Process | Where-Object {$_.Name -like 'createchildproc'} | Stop-Process + + [int] $numToCreate = 2 + + $ps = [PowerShell]::Create().AddCommand($createchildprocess) + $ps.AddParameter($numToCreate) + $async = $ps.BeginInvoke() + $ps.InvocationStateInfo.State | Should Be "Running" + + [bool] $childrenCreated = $false + while (-not $childrenCreated) + { + $childprocesses = Get-Process | Where-Object {$_.Name -like 'createchildproc'} + if ($childprocesses.count -eq $numToCreate+1) + { + $childrenCreated = $true + } + } + + $startTime = Get-Date + $beginsync = $ps.BeginStop($null, $async) + # wait no more than 5 secs for the processes to be terminated, otherwise test has failed + while (((Get-Date) - $startTime).TotalSeconds -lt 5) + { + if (($childprocesses.hasexited -eq $true).count -eq $numToCreate+1) + { + break + } + } + $childprocesses = Get-Process | Where-Object {$_.Name -like 'createchildproc'} + $count = $childprocesses.count + $childprocesses | Stop-Process + $count | Should Be 0 + } +} diff --git a/test/tools/CreateChildProcess/CreateChildProcess.cs b/test/tools/CreateChildProcess/CreateChildProcess.cs new file mode 100644 index 0000000000..c37272cf63 --- /dev/null +++ b/test/tools/CreateChildProcess/CreateChildProcess.cs @@ -0,0 +1,25 @@ +using System; +using System.Diagnostics; +using System.Threading; + +namespace CreateChildProcess +{ + class Program + { + static void Main(string[] args) + { + if (args.Length > 0) + { + uint num = UInt32.Parse(args[0]); + for (uint i = 0; i < num; i++) + { + Process child = new Process(); + child.StartInfo.FileName = Process.GetCurrentProcess().MainModule.FileName; + child.Start(); + } + } + // sleep is needed so the process doesn't exit before the test case kill it + Thread.Sleep(100000); + } + } +} diff --git a/test/tools/CreateChildProcess/project.json b/test/tools/CreateChildProcess/project.json new file mode 100644 index 0000000000..ec9f31427b --- /dev/null +++ b/test/tools/CreateChildProcess/project.json @@ -0,0 +1,28 @@ +{ + "name": "createchildprocess", + "version": "1.0.0-*", + "description": "Very simple little console app that creates child processes of itself", + + "buildOptions": { + "emitEntryPoint": true + }, + + "frameworks": { + "netcoreapp1.0": { + "dependencies": { + "Microsoft.NETCore.App": "1.1.0-preview1-001100-00" + } + } + }, + + "runtimes": { + "ubuntu.16.04-x64": { }, + "ubuntu.14.04-x64": { }, + "debian.8-x64": { }, + "centos.7-x64": { }, + "win7-x64": { }, + "win81-x64": { }, + "win10-x64": { }, + "osx.10.11-x64": { } + } +}