From 328fb66241eda56c8e42e3a153eafa48bbded767 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Thu, 22 Mar 2018 16:26:30 -0700 Subject: [PATCH] Simplify interop on Windows CreateProcess path (dotnet/corefx#28372) This makes it a bit smaller, faster and allocate less. And also make it compile with CoreRT. Commit migrated from https://github.com/dotnet/corefx/commit/909b3557472df23ec8850361c0c31c4248f297d9 --- .../Interop.CreateProcessWithLogon.cs | 4 +- .../Windows/kernel32/Interop.CreateProcess.cs | 78 +++----- .../src/System/Diagnostics/Process.Windows.cs | 173 ++++++++---------- 3 files changed, 99 insertions(+), 156 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/advapi32/Interop.CreateProcessWithLogon.cs b/src/libraries/Common/src/Interop/Windows/advapi32/Interop.CreateProcessWithLogon.cs index a12cc167c0375..80eae1076b32d 100644 --- a/src/libraries/Common/src/Interop/Windows/advapi32/Interop.CreateProcessWithLogon.cs +++ b/src/libraries/Common/src/Interop/Windows/advapi32/Interop.CreateProcessWithLogon.cs @@ -21,8 +21,8 @@ internal static extern bool CreateProcessWithLogonW( int creationFlags, IntPtr environmentBlock, string lpCurrentDirectory, - Interop.Kernel32.STARTUPINFO lpStartupInfo, - [Out] Interop.Kernel32.PROCESS_INFORMATION lpProcessInformation); + ref Interop.Kernel32.STARTUPINFO lpStartupInfo, + ref Interop.Kernel32.PROCESS_INFORMATION lpProcessInformation); [Flags] internal enum LogonFlags diff --git a/src/libraries/Common/src/Interop/Windows/kernel32/Interop.CreateProcess.cs b/src/libraries/Common/src/Interop/Windows/kernel32/Interop.CreateProcess.cs index 0081972088e1d..5446010d2d418 100644 --- a/src/libraries/Common/src/Interop/Windows/kernel32/Interop.CreateProcess.cs +++ b/src/libraries/Common/src/Interop/Windows/kernel32/Interop.CreateProcess.cs @@ -21,68 +21,40 @@ internal static extern bool CreateProcess( int dwCreationFlags, IntPtr lpEnvironment, string lpCurrentDirectory, - STARTUPINFO lpStartupInfo, - [Out] PROCESS_INFORMATION lpProcessInformation + ref STARTUPINFO lpStartupInfo, + ref PROCESS_INFORMATION lpProcessInformation ); [StructLayout(LayoutKind.Sequential)] - internal sealed class PROCESS_INFORMATION + internal struct PROCESS_INFORMATION { - internal IntPtr hProcess = IntPtr.Zero; - internal IntPtr hThread = IntPtr.Zero; - internal int dwProcessId = 0; - internal int dwThreadId = 0; + internal IntPtr hProcess; + internal IntPtr hThread; + internal int dwProcessId; + internal int dwThreadId; } [StructLayout(LayoutKind.Sequential)] - internal sealed class STARTUPINFO : IDisposable + internal struct STARTUPINFO { internal int cb; - internal IntPtr lpReserved = IntPtr.Zero; - internal IntPtr lpDesktop = IntPtr.Zero; - internal IntPtr lpTitle = IntPtr.Zero; - internal int dwX = 0; - internal int dwY = 0; - internal int dwXSize = 0; - internal int dwYSize = 0; - internal int dwXCountChars = 0; - internal int dwYCountChars = 0; - internal int dwFillAttribute = 0; - internal int dwFlags = 0; - internal short wShowWindow = 0; - internal short cbReserved2 = 0; - internal IntPtr lpReserved2 = IntPtr.Zero; - internal SafeFileHandle hStdInput = new SafeFileHandle(IntPtr.Zero, false); - internal SafeFileHandle hStdOutput = new SafeFileHandle(IntPtr.Zero, false); - internal SafeFileHandle hStdError = new SafeFileHandle(IntPtr.Zero, false); - - internal - STARTUPINFO() - { - cb = Marshal.SizeOf(); - } - - public void Dispose() - { - // close the handles created for child process - if (hStdInput != null && !hStdInput.IsInvalid) - { - hStdInput.Dispose(); - hStdInput = null; - } - - if (hStdOutput != null && !hStdOutput.IsInvalid) - { - hStdOutput.Dispose(); - hStdOutput = null; - } - - if (hStdError != null && !hStdError.IsInvalid) - { - hStdError.Dispose(); - hStdError = null; - } - } + internal IntPtr lpReserved; + internal IntPtr lpDesktop; + internal IntPtr 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; } } } diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs index e6beed24708a4..d3bc24b578661 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs @@ -437,7 +437,7 @@ private void SetWorkingSetLimitsCore(IntPtr? newMin, IntPtr? newMax, out IntPtr /// Starts the process using the supplied start info. /// The start info with which to start the process. - private bool StartWithCreateProcess(ProcessStartInfo startInfo) + private unsafe bool StartWithCreateProcess(ProcessStartInfo startInfo) { // See knowledge base article Q190351 for an explanation of the following code. Noteworthy tricky points: // * The handles are duplicated as non-inheritable before they are passed to CreateProcess so @@ -453,47 +453,53 @@ private bool StartWithCreateProcess(ProcessStartInfo startInfo) Interop.Kernel32.SECURITY_ATTRIBUTES unused_SecAttrs = new Interop.Kernel32.SECURITY_ATTRIBUTES(); SafeProcessHandle procSH = new SafeProcessHandle(); SafeThreadHandle threadSH = new SafeThreadHandle(); - bool retVal; - int errorCode = 0; // handles used in parent process - SafeFileHandle standardInputWritePipeHandle = null; - SafeFileHandle standardOutputReadPipeHandle = null; - SafeFileHandle standardErrorReadPipeHandle = null; - GCHandle environmentHandle = new GCHandle(); + SafeFileHandle parentInputPipeHandle = null; + SafeFileHandle childInputPipeHandle = null; + SafeFileHandle parentOutputPipeHandle = null; + SafeFileHandle childOutputPipeHandle = null; + SafeFileHandle parentErrorPipeHandle = null; + SafeFileHandle childErrorPipeHandle = null; lock (s_createProcessLock) { try { + startupInfo.cb = sizeof(Interop.Kernel32.STARTUPINFO); + // set up the streams if (startInfo.RedirectStandardInput || startInfo.RedirectStandardOutput || startInfo.RedirectStandardError) { if (startInfo.RedirectStandardInput) { - CreatePipe(out standardInputWritePipeHandle, out startupInfo.hStdInput, true); + CreatePipe(out parentInputPipeHandle, out childInputPipeHandle, true); } else { - startupInfo.hStdInput = new SafeFileHandle(Interop.Kernel32.GetStdHandle(Interop.Kernel32.HandleTypes.STD_INPUT_HANDLE), false); + childInputPipeHandle = new SafeFileHandle(Interop.Kernel32.GetStdHandle(Interop.Kernel32.HandleTypes.STD_INPUT_HANDLE), false); } if (startInfo.RedirectStandardOutput) { - CreatePipe(out standardOutputReadPipeHandle, out startupInfo.hStdOutput, false); + CreatePipe(out parentOutputPipeHandle, out childOutputPipeHandle, false); } else { - startupInfo.hStdOutput = new SafeFileHandle(Interop.Kernel32.GetStdHandle(Interop.Kernel32.HandleTypes.STD_OUTPUT_HANDLE), false); + childOutputPipeHandle = new SafeFileHandle(Interop.Kernel32.GetStdHandle(Interop.Kernel32.HandleTypes.STD_OUTPUT_HANDLE), false); } if (startInfo.RedirectStandardError) { - CreatePipe(out standardErrorReadPipeHandle, out startupInfo.hStdError, false); + CreatePipe(out parentErrorPipeHandle, out childErrorPipeHandle, false); } else { - startupInfo.hStdError = new SafeFileHandle(Interop.Kernel32.GetStdHandle(Interop.Kernel32.HandleTypes.STD_ERROR_HANDLE), false); + childErrorPipeHandle = new SafeFileHandle(Interop.Kernel32.GetStdHandle(Interop.Kernel32.HandleTypes.STD_ERROR_HANDLE), false); } + startupInfo.hStdInput = childInputPipeHandle.DangerousGetHandle(); + startupInfo.hStdOutput = childOutputPipeHandle.DangerousGetHandle(); + startupInfo.hStdError = childErrorPipeHandle.DangerousGetHandle(); + startupInfo.dwFlags = Interop.Advapi32.StartupInfoOptions.STARTF_USESTDHANDLES; } @@ -502,18 +508,19 @@ private bool StartWithCreateProcess(ProcessStartInfo startInfo) if (startInfo.CreateNoWindow) creationFlags |= Interop.Advapi32.StartupInfoOptions.CREATE_NO_WINDOW; // set up the environment block parameter - IntPtr environmentPtr = (IntPtr)0; + string environmentBlock = null; if (startInfo._environmentVariables != null) { creationFlags |= Interop.Advapi32.StartupInfoOptions.CREATE_UNICODE_ENVIRONMENT; - byte[] environmentBytes = EnvironmentVariablesToByteArray(startInfo._environmentVariables); - environmentHandle = GCHandle.Alloc(environmentBytes, GCHandleType.Pinned); - environmentPtr = environmentHandle.AddrOfPinnedObject(); + environmentBlock = GetEnvironmentVariablesBlock(startInfo._environmentVariables); } string workingDirectory = startInfo.WorkingDirectory; if (workingDirectory == string.Empty) workingDirectory = Directory.GetCurrentDirectory(); + bool retVal; + int errorCode = 0; + if (startInfo.UserName.Length != 0) { if (startInfo.Password != null && startInfo.PasswordInClearText != null) @@ -527,138 +534,105 @@ private bool StartWithCreateProcess(ProcessStartInfo startInfo) logonFlags = Interop.Advapi32.LogonFlags.LOGON_WITH_PROFILE; } - if (startInfo.Password != null) + fixed (char* passwordInClearTextPtr = startInfo.PasswordInClearText ?? string.Empty) + fixed (char* environmentBlockPtr = environmentBlock) { - IntPtr passwordPtr = Marshal.SecureStringToGlobalAllocUnicode(startInfo.Password); + IntPtr passwordPtr = (startInfo.Password != null) ? + Marshal.SecureStringToGlobalAllocUnicode(startInfo.Password) : IntPtr.Zero; + try { retVal = Interop.Advapi32.CreateProcessWithLogonW( startInfo.UserName, startInfo.Domain, - passwordPtr, + (passwordPtr != IntPtr.Zero) ? passwordPtr : (IntPtr)passwordInClearTextPtr, logonFlags, null, // we don't need this since all the info is in commandLine commandLine, creationFlags, - environmentPtr, + (IntPtr)environmentBlockPtr, workingDirectory, - startupInfo, // pointer to STARTUPINFO - processInfo // pointer to PROCESS_INFORMATION + ref startupInfo, // pointer to STARTUPINFO + ref processInfo // pointer to PROCESS_INFORMATION ); - if (!retVal) errorCode = Marshal.GetLastWin32Error(); } - finally { Marshal.ZeroFreeGlobalAllocUnicode(passwordPtr); } - } - else - { - unsafe - { - fixed (char* passwordPtr = startInfo.PasswordInClearText ?? string.Empty) - { - retVal = Interop.Advapi32.CreateProcessWithLogonW( - startInfo.UserName, - startInfo.Domain, - (IntPtr)passwordPtr, - logonFlags, - null, // we don't need this since all the info is in commandLine - commandLine, - creationFlags, - environmentPtr, - workingDirectory, - startupInfo, // pointer to STARTUPINFO - processInfo // pointer to PROCESS_INFORMATION - ); - - } - } - if (!retVal) - errorCode = Marshal.GetLastWin32Error(); - } - if (processInfo.hProcess != IntPtr.Zero && processInfo.hProcess != (IntPtr)INVALID_HANDLE_VALUE) - procSH.InitialSetHandle(processInfo.hProcess); - if (processInfo.hThread != IntPtr.Zero && processInfo.hThread != (IntPtr)INVALID_HANDLE_VALUE) - threadSH.InitialSetHandle(processInfo.hThread); - if (!retVal) - { - if (errorCode == Interop.Errors.ERROR_BAD_EXE_FORMAT || errorCode == Interop.Errors.ERROR_EXE_MACHINE_TYPE_MISMATCH) + finally { - throw new Win32Exception(errorCode, SR.InvalidApplication); + if (passwordPtr != IntPtr.Zero) + Marshal.ZeroFreeGlobalAllocUnicode(passwordPtr); } - throw new Win32Exception(errorCode); } } else { - retVal = Interop.Kernel32.CreateProcess( + fixed (char* environmentBlockPtr = environmentBlock) + { + retVal = Interop.Kernel32.CreateProcess( null, // we don't need this since all the info is in commandLine commandLine, // pointer to the command line string ref unused_SecAttrs, // address to process security attributes, we don't need to inherit the handle ref unused_SecAttrs, // address to thread security attributes. true, // handle inheritance flag creationFlags, // creation flags - environmentPtr, // pointer to new environment block + (IntPtr)environmentBlockPtr, // pointer to new environment block workingDirectory, // pointer to current directory name - startupInfo, // pointer to STARTUPINFO - processInfo // pointer to PROCESS_INFORMATION + ref startupInfo, // pointer to STARTUPINFO + ref processInfo // pointer to PROCESS_INFORMATION ); - if (!retVal) - errorCode = Marshal.GetLastWin32Error(); - if (processInfo.hProcess != (IntPtr)0 && processInfo.hProcess != (IntPtr)INVALID_HANDLE_VALUE) - procSH.InitialSetHandle(processInfo.hProcess); - if (processInfo.hThread != (IntPtr)0 && processInfo.hThread != (IntPtr)INVALID_HANDLE_VALUE) - threadSH.InitialSetHandle(processInfo.hThread); - - if (!retVal) + if (!retVal) + errorCode = Marshal.GetLastWin32Error(); + } + } + + if (processInfo.hProcess != IntPtr.Zero && processInfo.hProcess != (IntPtr)INVALID_HANDLE_VALUE) + procSH.InitialSetHandle(processInfo.hProcess); + if (processInfo.hThread != IntPtr.Zero && processInfo.hThread != (IntPtr)INVALID_HANDLE_VALUE) + threadSH.InitialSetHandle(processInfo.hThread); + + if (!retVal) + { + if (errorCode == Interop.Errors.ERROR_BAD_EXE_FORMAT || errorCode == Interop.Errors.ERROR_EXE_MACHINE_TYPE_MISMATCH) { - if (errorCode == Interop.Errors.ERROR_BAD_EXE_FORMAT || errorCode == Interop.Errors.ERROR_EXE_MACHINE_TYPE_MISMATCH) - { - throw new Win32Exception(errorCode, SR.InvalidApplication); - } - throw new Win32Exception(errorCode); + throw new Win32Exception(errorCode, SR.InvalidApplication); } + throw new Win32Exception(errorCode); } } finally { - // free environment block - if (environmentHandle.IsAllocated) - { - environmentHandle.Free(); - } + childInputPipeHandle?.Dispose(); + childOutputPipeHandle?.Dispose(); + childErrorPipeHandle?.Dispose(); - startupInfo.Dispose(); + threadSH?.Dispose(); } } if (startInfo.RedirectStandardInput) { Encoding enc = startInfo.StandardInputEncoding ?? GetEncoding((int)Interop.Kernel32.GetConsoleCP()); - _standardInput = new StreamWriter(new FileStream(standardInputWritePipeHandle, FileAccess.Write, 4096, false), enc, 4096); + _standardInput = new StreamWriter(new FileStream(parentInputPipeHandle, FileAccess.Write, 4096, false), enc, 4096); _standardInput.AutoFlush = true; } if (startInfo.RedirectStandardOutput) { Encoding enc = startInfo.StandardOutputEncoding ?? GetEncoding((int)Interop.Kernel32.GetConsoleOutputCP()); - _standardOutput = new StreamReader(new FileStream(standardOutputReadPipeHandle, FileAccess.Read, 4096, false), enc, true, 4096); + _standardOutput = new StreamReader(new FileStream(parentOutputPipeHandle, FileAccess.Read, 4096, false), enc, true, 4096); } if (startInfo.RedirectStandardError) { Encoding enc = startInfo.StandardErrorEncoding ?? GetEncoding((int)Interop.Kernel32.GetConsoleOutputCP()); - _standardError = new StreamReader(new FileStream(standardErrorReadPipeHandle, FileAccess.Read, 4096, false), enc, true, 4096); + _standardError = new StreamReader(new FileStream(parentErrorPipeHandle, FileAccess.Read, 4096, false), enc, true, 4096); } - bool ret = false; - if (!procSH.IsInvalid) - { - SetProcessHandle(procSH); - SetProcessId((int)processInfo.dwProcessId); - threadSH.Dispose(); - ret = true; - } + if (procSH.IsInvalid) + return false; - return ret; + SetProcessHandle(procSH); + SetProcessId((int)processInfo.dwProcessId); + return true; } private static Encoding GetEncoding(int codePage) @@ -901,11 +875,10 @@ private void CreatePipe(out SafeFileHandle parentHandle, out SafeFileHandle chil } } - private static byte[] EnvironmentVariablesToByteArray(IDictionary sd) + private static string GetEnvironmentVariablesBlock(IDictionary sd) { // get the keys string[] keys = new string[sd.Count]; - byte[] envBlock = null; sd.Keys.CopyTo(keys, 0); // sort both by the keys @@ -926,10 +899,8 @@ private static byte[] EnvironmentVariablesToByteArray(IDictionary