From 5afbdf1bd5ac1f6ec8ebaf236e9abdb8982eeef1 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 25 Oct 2018 21:12:50 +0200 Subject: [PATCH 01/19] Unix: make UseShellExecute execute executables On Windows, UseShellExecute executes executables. This gives it the same behavior on Unix. --- .../Interop.ForkAndExecProcess.cs | 18 +- .../src/System.Diagnostics.Process.csproj | 3 + .../src/System/Diagnostics/Process.Unix.cs | 192 ++++++++++++++---- 3 files changed, 157 insertions(+), 56 deletions(-) diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs b/src/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs index 3aa37279bc50..f404c64f66de 100644 --- a/src/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs +++ b/src/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs @@ -12,7 +12,7 @@ internal static partial class Interop { internal static partial class Sys { - internal static unsafe void ForkAndExecProcess( + internal static unsafe int ForkAndExecProcess( string filename, string[] argv, string[] envp, string cwd, bool redirectStdin, bool redirectStdout, bool redirectStderr, bool setUser, uint userId, uint groupId, @@ -31,17 +31,11 @@ internal static unsafe void ForkAndExecProcess( out lpChildPid, out stdinFd, out stdoutFd, out stderrFd); if (result != 0) { - // Normally we'd simply make this method return the result of the native - // call and allow the caller to use GetLastWin32Error. However, we need - // to free the native arrays after calling the function, and doing so - // stomps on the runtime's captured last error. So we need to access the - // error here, and without SetLastWin32Error available, we can't propagate - // the error to the caller via the normal GetLastWin32Error mechanism. We could - // return 0 on success or the GetLastWin32Error value on failure, but that's - // technically ambiguous, in the case of a failure with a 0 errno. Simplest - // solution then is just to throw here the same exception the Process caller - // would have. This can be revisited if we ever have another call site. - throw new Win32Exception(); + return Marshal.GetLastWin32Error(); + } + else + { + return 0; } } finally diff --git a/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj b/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj index 928f404e3001..5e0e150ea3cf 100644 --- a/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj +++ b/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj @@ -346,6 +346,9 @@ Common\Interop\Unix\Interop.WaitPid.cs + + Common\Interop\Unix\System.Native\Interop.Access.cs + diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index 1042f433b2a9..611d4898db7a 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -291,15 +291,49 @@ private bool StartCore(ProcessStartInfo startInfo) } } - int childPid, stdinFd, stdoutFd, stderrFd; + int stdinFd = -1, stdoutFd = -1, stderrFd = -1; string[] envp = CreateEnvp(startInfo); string cwd = !string.IsNullOrWhiteSpace(startInfo.WorkingDirectory) ? startInfo.WorkingDirectory : null; + bool setCredentials = !string.IsNullOrEmpty(startInfo.UserName); + uint userId = 0; + uint groupId = 0; + if (setCredentials) + { + (userId, groupId) = GetUserAndGroupIds(startInfo); + } + if (startInfo.UseShellExecute) { + // On Windows, UseShellExecute of executables and scripts causes those files to be executed. + // To achieve this on Unix, we check if the file is executable (x-bit). + // Some files may have the x-bit set even when they are not executable. This happens for example + // when a Windows filesystem is mounted on Linux. To handle that, treat it as a regular file + // when exec returns ENOEXEC (file format cannot be executed). + bool isExecuting = false; + filename = ResolveExecutableForShellExecute(startInfo.FileName); + if (filename != null) + { + argv = ParseArgv(startInfo); + + isExecuting = ForkAndExecProcess(filename, argv, envp, cwd, + startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError, + setCredentials, userId, groupId, + out stdinFd, out stdoutFd, out stderrFd, + throwOnNoExec: false); // return false instead of throwing on ENOEXEC + } + // use default program to open file/url - filename = GetPathToOpenFile(); - argv = ParseArgv(startInfo, filename); + if (!isExecuting) + { + filename = GetPathToOpenFile(); + argv = ParseArgv(startInfo, filename); + + ForkAndExecProcess(filename, argv, envp, cwd, + startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError, + setCredentials, userId, groupId, + out stdinFd, out stdoutFd, out stderrFd); + } } else { @@ -309,50 +343,11 @@ private bool StartCore(ProcessStartInfo startInfo) { throw new Win32Exception(SR.DirectoryNotValidAsInput); } - } - - if (string.IsNullOrEmpty(filename)) - { - throw new Win32Exception(Interop.Error.ENOENT.Info().RawErrno); - } - - bool setCredentials = !string.IsNullOrEmpty(startInfo.UserName); - uint userId = 0; - uint groupId = 0; - if (setCredentials) - { - (userId, groupId) = GetUserAndGroupIds(startInfo); - } - // Lock to avoid races with OnSigChild - // By using a ReaderWriterLock we allow multiple processes to start concurrently. - s_processStartLock.EnterReadLock(); - try - { - // Invoke the shim fork/execve routine. It will create pipes for all requested - // redirects, fork a child process, map the pipe ends onto the appropriate stdin/stdout/stderr - // descriptors, and execve to execute the requested process. The shim implementation - // is used to fork/execve as executing managed code in a forked process is not safe (only - // the calling thread will transfer, thread IDs aren't stable across the fork, etc.) - Interop.Sys.ForkAndExecProcess( - filename, argv, envp, cwd, + ForkAndExecProcess(filename, argv, envp, cwd, startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError, - setCredentials, userId, groupId, - out childPid, + setCredentials, userId, groupId, out stdinFd, out stdoutFd, out stderrFd); - - // Ensure we'll reap this process. - // note: SetProcessId will set this if we don't set it first. - _waitStateHolder = new ProcessWaitState.Holder(childPid, isNewChild: true); - - // Store the child's information into this Process object. - Debug.Assert(childPid >= 0); - SetProcessId(childPid); - SetProcessHandle(new SafeProcessHandle(childPid)); - } - finally - { - s_processStartLock.ExitReadLock(); } // Configure the parent's ends of the redirection streams. @@ -381,6 +376,67 @@ private bool StartCore(ProcessStartInfo startInfo) return true; } + private bool ForkAndExecProcess( + string filename, string[] argv, string[] envp, string cwd, + bool redirectStdin, bool redirectStdout, bool redirectStderr, + bool setCredentials, uint userId, uint groupId, + out int stdinFd, out int stdoutFd, out int stderrFd, + bool throwOnNoExec = true) + { + if (string.IsNullOrEmpty(filename)) + { + throw new Win32Exception(Interop.Error.ENOENT.Info().RawErrno); + } + + // Lock to avoid races with OnSigChild + // By using a ReaderWriterLock we allow multiple processes to start concurrently. + s_processStartLock.EnterReadLock(); + try + { + int childPid; + + // Invoke the shim fork/execve routine. It will create pipes for all requested + // redirects, fork a child process, map the pipe ends onto the appropriate stdin/stdout/stderr + // descriptors, and execve to execute the requested process. The shim implementation + // is used to fork/execve as executing managed code in a forked process is not safe (only + // the calling thread will transfer, thread IDs aren't stable across the fork, etc.) + int errno = Interop.Sys.ForkAndExecProcess( + filename, argv, envp, cwd, + redirectStdin, redirectStdout, redirectStderr, + setCredentials, userId, groupId, + out childPid, + out stdinFd, out stdoutFd, out stderrFd); + + if (errno == 0) + { + // Ensure we'll reap this process. + // note: SetProcessId will set this if we don't set it first. + _waitStateHolder = new ProcessWaitState.Holder(childPid, isNewChild: true); + + // Store the child's information into this Process object. + Debug.Assert(childPid >= 0); + SetProcessId(childPid); + SetProcessHandle(new SafeProcessHandle(childPid)); + + return true; + } + else + { + if ((throwOnNoExec == false) + && (Interop.Sys.ConvertErrorPlatformToPal(errno) == Interop.Error.ENOEXEC)) + { + return false; + } + + throw new Win32Exception(errno); + } + } + finally + { + s_processStartLock.ExitReadLock(); + } + } + // ----------------------------- // ---- PAL layer ends here ---- // ----------------------------- @@ -439,6 +495,54 @@ private static string[] CreateEnvp(ProcessStartInfo psi) return envp; } + private static string ResolveExecutableForShellExecute(string filename) + { + // Determine if filename points to an executable file. + // filename may be an absolute path, a relative path or a uri. + + string resolvedFilename = null; + // filename is an absolute path + if (Path.IsPathRooted(filename)) + { + if (File.Exists(filename)) + { + resolvedFilename = filename; + } + } + // filename is a uri + else if (Uri.TryCreate(filename, UriKind.Absolute, out Uri uri)) + { + if (uri.IsFile && uri.Host == "" && File.Exists(uri.LocalPath)) + { + resolvedFilename = uri.LocalPath; + } + } + // filename is a relative path in the working directory + else if (File.Exists(filename)) + { + resolvedFilename = filename; + } + // find filename on PATH + else + { + resolvedFilename = FindProgramInPath(filename); + } + + if (resolvedFilename == null) + { + return null; + } + + if (Interop.Sys.Access(resolvedFilename, Interop.Sys.AccessMode.X_OK) == 0) + { + return resolvedFilename; + } + else + { + return null; + } + } + /// Resolves a path to the filename passed to ProcessStartInfo. /// The filename. /// The resolved path. It can return null in case of URLs. From 5585d7a7a4e39e5b0f9955a153d5911d6e70806d Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 5 Nov 2018 20:35:30 +0100 Subject: [PATCH 02/19] Add cross-platform tests --- .../src/System/Diagnostics/Process.Unix.cs | 18 +++-- .../tests/ProcessTests.cs | 81 +++++++++++++++++++ 2 files changed, 92 insertions(+), 7 deletions(-) diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index 611d4898db7a..098885d66a5f 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -517,15 +517,19 @@ private static string ResolveExecutableForShellExecute(string filename) resolvedFilename = uri.LocalPath; } } - // filename is a relative path in the working directory - else if (File.Exists(filename)) - { - resolvedFilename = filename; - } - // find filename on PATH else { - resolvedFilename = FindProgramInPath(filename); + string relativeFile = Path.Combine(Directory.GetCurrentDirectory(), filename); + // filename is a relative path in the working directory + if (File.Exists(relativeFile)) + { + resolvedFilename = relativeFile; + } + // find filename on PATH + else + { + resolvedFilename = FindProgramInPath(filename); + } } if (resolvedFilename == null) diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.cs b/src/System.Diagnostics.Process/tests/ProcessTests.cs index ef42b7201c1c..c6f9d624f9d9 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.cs @@ -218,6 +218,87 @@ public void ProcessStart_UseShellExecute_OnWindows_DoesNotThrow(bool isFolder) } } + [Theory] + [InlineData(true)] + [InlineData(false)] + public void ProcessStart_UseShellExecute_Executes(bool filenameAsUrl) + { + string filename = GetTestFilePath(); + if (PlatformDetection.IsWindows) + { + filename += ".bat"; + } + WriteScriptFile(filename, returnValue: 42); + + if (filenameAsUrl) + { + filename = new Uri(filename).ToString(); + } + + using (var process = Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName = filename })) + { + process.WaitForExit(); + Assert.Equal(42, process.ExitCode); + } + } + + [Fact] + public void ProcessStart_UseShellExecute_ExecuteOrder() + { + // Create a directory that we will use as PATH + string path = GetTestFileName(); + Directory.CreateDirectory(path); + + RemoteInvokeOptions options = new RemoteInvokeOptions(); + options.StartInfo.UseShellExecute = true; + options.StartInfo.EnvironmentVariables["PATH"] = path; + RemoteInvoke(pathDirectory => + { + // Create two identically named scripts, one in the working directory and one on PATH. + string scriptFilename = $"script_{nameof(ProcessStart_UseShellExecute_ExecuteOrder)}"; + if (PlatformDetection.IsWindows) + { + scriptFilename += ".bat"; + } + const int workingDirReturnValue = 1; + const int pathDirReturnValue = 2; + WriteScriptFile(Path.Combine(pathDirectory, scriptFilename), returnValue: pathDirReturnValue); + WriteScriptFile(Path.Combine(Directory.GetCurrentDirectory(), scriptFilename), returnValue: workingDirReturnValue); + + // Execute the script and verify we prefer the one in the working directory. + using (var process = Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName = scriptFilename })) + { + process.WaitForExit(); + Assert.Equal(workingDirReturnValue, process.ExitCode); + } + + // Remove the script in the working directory and verify we now use the one on PATH. + File.Delete(scriptFilename); + using (var process = Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName = scriptFilename })) + { + process.WaitForExit(); + Assert.Equal(pathDirReturnValue, process.ExitCode); + } + + return SuccessExitCode; + }, path, options).Dispose(); + } + + private void WriteScriptFile(string filename, int returnValue) + { + if (PlatformDetection.IsWindows) + { + File.WriteAllText(filename, $"exit {returnValue}"); + } + else + { + File.WriteAllText(filename, $"#!/bin/sh\nexit {returnValue}\n"); + // set x-bit + int mode = Convert.ToInt32("744", 8); + Assert.Equal(0, chmod(filename, mode)); + } + } + [Fact] [ActiveIssue(31908, TargetFrameworkMonikers.Uap)] public void TestExitCode() From d20d97667a6e1c844e216fb89b980723dc3422c4 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 6 Nov 2018 08:23:59 +0100 Subject: [PATCH 03/19] fix Windows test build failure --- src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs | 3 --- src/System.Diagnostics.Process/tests/ProcessTests.cs | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index dfce8f1ff60f..ba6e2ef67384 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -711,9 +711,6 @@ private void RunTestAsSudo(Func testMethod, string arg) Assert.Equal(0, p.ExitCode); } - [DllImport("libc")] - private static extern int chmod(string path, int mode); - [DllImport("libc")] private static extern uint geteuid(); diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.cs b/src/System.Diagnostics.Process/tests/ProcessTests.cs index c6f9d624f9d9..1d425a560e9a 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.cs @@ -1813,5 +1813,8 @@ private SecureString AsSecureString(string str) return secureString; } + + [DllImport("libc")] + private static extern int chmod(string path, int mode); } } From 4de225be953af22cfdafef25e08940eca43b57e1 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 6 Nov 2018 09:12:37 +0100 Subject: [PATCH 04/19] Add test for non-executable file with x-bit --- .../tests/ProcessTests.Unix.cs | 34 +++++++++++++++++++ .../tests/ProcessTests.cs | 1 - 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index ba6e2ef67384..9d9f086e7747 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -155,6 +155,40 @@ public void ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled(bool } } + [Fact] + public void ProcessStart_UseShellExecute_OnUnix_FallsBackWhenNotRealExecutable() + { + // Create a script that we'll use to 'open' the file by putting it on PATH + // with the appropriate name. + string path = GetTestFileName(); + Directory.CreateDirectory(path); + WriteScriptFile(Path.Combine(path, s_allowedProgramsToRun[0]), returnValue: 42); + + // Create a file that has the x-bit set, but which isn't a valid script. + string filename = GetTestFileName(); + WriteScriptFile(filename, returnValue: 0); + File.WriteAllText(filename, $"not a script"); + int mode = Convert.ToInt32("744", 8); + Assert.Equal(0, chmod(filename, mode)); + + RemoteInvokeOptions options = new RemoteInvokeOptions(); + options.StartInfo.EnvironmentVariables["PATH"] = path; + RemoteInvoke(fileToOpen => + { + using (var px = Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName = fileToOpen })) + { + Assert.NotNull(px); + if (!RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) // on OSX, process name is dotnet for some reason. Refer to #23972 + { + Assert.Equal(s_allowedProgramsToRun[0], px.ProcessName); + } + px.WaitForExit(); + Assert.True(px.HasExited); + Assert.Equal(42, px.ExitCode); + } + }, filename, options).Dispose(); + } + [Theory, InlineData("vi")] [PlatformSpecific(TestPlatforms.Linux)] [OuterLoop("Opens program")] diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.cs b/src/System.Diagnostics.Process/tests/ProcessTests.cs index 1d425a560e9a..621118adea84 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.cs @@ -250,7 +250,6 @@ public void ProcessStart_UseShellExecute_ExecuteOrder() Directory.CreateDirectory(path); RemoteInvokeOptions options = new RemoteInvokeOptions(); - options.StartInfo.UseShellExecute = true; options.StartInfo.EnvironmentVariables["PATH"] = path; RemoteInvoke(pathDirectory => { From 417ebd19f312a5d666c4b5d7919fe7e0637567b3 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 6 Nov 2018 10:12:42 +0100 Subject: [PATCH 05/19] Skip tests on unsupported platforms --- .../tests/ProcessTests.Unix.cs | 4 ---- src/System.Diagnostics.Process/tests/ProcessTests.cs | 11 +++++++---- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index 9d9f086e7747..656870a145ae 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -178,10 +178,6 @@ public void ProcessStart_UseShellExecute_OnUnix_FallsBackWhenNotRealExecutable() using (var px = Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName = fileToOpen })) { Assert.NotNull(px); - if (!RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) // on OSX, process name is dotnet for some reason. Refer to #23972 - { - Assert.Equal(s_allowedProgramsToRun[0], px.ProcessName); - } px.WaitForExit(); Assert.True(px.HasExited); Assert.Equal(42, px.ExitCode); diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.cs b/src/System.Diagnostics.Process/tests/ProcessTests.cs index 621118adea84..40e116196473 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.cs @@ -218,9 +218,10 @@ public void ProcessStart_UseShellExecute_OnWindows_DoesNotThrow(bool isFolder) } } - [Theory] - [InlineData(true)] - [InlineData(false)] + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsServerCore), + nameof(PlatformDetection.IsNotWindowsNanoServer), nameof(PlatformDetection.IsNotWindowsIoTCore))] + [SkipOnTargetFramework(TargetFrameworkMonikers.Uap, "not supported on UAP")] + [InlineData(true), InlineData(false)] public void ProcessStart_UseShellExecute_Executes(bool filenameAsUrl) { string filename = GetTestFilePath(); @@ -242,7 +243,9 @@ public void ProcessStart_UseShellExecute_Executes(bool filenameAsUrl) } } - [Fact] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsServerCore), + nameof(PlatformDetection.IsNotWindowsNanoServer), nameof(PlatformDetection.IsNotWindowsIoTCore))] + [SkipOnTargetFramework(TargetFrameworkMonikers.Uap, "not supported on UAP")] public void ProcessStart_UseShellExecute_ExecuteOrder() { // Create a directory that we will use as PATH From fa37504e50978341eed07a18887f7a342fd592e8 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 6 Nov 2018 11:15:11 +0100 Subject: [PATCH 06/19] Unix: only allow specific Verbs --- .../src/System.Diagnostics.Process.csproj | 6 ++-- .../src/System/Diagnostics/Process.Unix.cs | 20 +++++++++++ .../tests/ProcessTests.Unix.cs | 35 +++++++++++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj b/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj index 5e0e150ea3cf..3623ce4a40ff 100644 --- a/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj +++ b/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj @@ -39,6 +39,9 @@ Common\System\PasteArguments.cs + + Common\Interop\Windows\Interop.Errors.cs + @@ -223,9 +226,6 @@ Common\Interop\Windows\kernel32\Interop.CreatePipe.cs - - Common\Interop\Windows\Interop.Errors.cs - Common\Interop\Windows\kernel32\Interop.ThreadOptions.cs diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index 098885d66a5f..695b27437e2b 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -305,6 +305,11 @@ private bool StartCore(ProcessStartInfo startInfo) if (startInfo.UseShellExecute) { + if (!IsValidVerb(startInfo.Verb)) + { + throw new Win32Exception(Interop.Errors.ERROR_NO_ASSOCIATION); + } + // On Windows, UseShellExecute of executables and scripts causes those files to be executed. // To achieve this on Unix, we check if the file is executable (x-bit). // Some files may have the x-bit set even when they are not executable. This happens for example @@ -758,6 +763,21 @@ private ProcessWaitState GetWaitState() return _waitStateHolder._state; } + private static readonly string[] s_validVerbs = new string[] { null, "", "open" }; + + private static bool IsValidVerb(string verb) + { + for (int i = 0; i < s_validVerbs.Length; i++) + { + if (s_validVerbs[i] == verb) + { + return true; + } + } + + return false; + } + private static (uint userId, uint groupId) GetUserAndGroupIds(ProcessStartInfo startInfo) { Debug.Assert(!string.IsNullOrEmpty(startInfo.UserName)); diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index 656870a145ae..8d9dd93dd5b7 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -185,6 +185,41 @@ public void ProcessStart_UseShellExecute_OnUnix_FallsBackWhenNotRealExecutable() }, filename, options).Dispose(); } + [Theory] + [InlineData((string)null, true)] + [InlineData("", true)] + [InlineData("open", true)] + [InlineData("invalid", false)] + public void ProcessStart_UseShellExecute_OnUnix_ValidVerbs(string verb, bool isValid) + { + // Create a script that we'll use to 'open' the file by putting it on PATH + // with the appropriate name. + string path = GetTestFileName(); + Directory.CreateDirectory(path); + WriteScriptFile(Path.Combine(path, s_allowedProgramsToRun[0]), returnValue: 42); + + RemoteInvokeOptions options = new RemoteInvokeOptions(); + options.StartInfo.EnvironmentVariables["PATH"] = path; + RemoteInvoke((argVerb, argValid) => + { + var psi = new ProcessStartInfo { UseShellExecute = true, FileName = "/", Verb = argVerb }; + if (bool.Parse(argValid)) + { + using (var px = Process.Start(psi)) + { + Assert.NotNull(px); + px.WaitForExit(); + Assert.True(px.HasExited); + Assert.Equal(42, px.ExitCode); + } + } + else + { + Assert.Throws(() => Process.Start(psi)); + } + }, verb, isValid.ToString(), options).Dispose(); + } + [Theory, InlineData("vi")] [PlatformSpecific(TestPlatforms.Linux)] [OuterLoop("Opens program")] From 56341fe6d9975b408d38d7ee1ae0cf39d8408ec4 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 6 Nov 2018 11:21:09 +0100 Subject: [PATCH 07/19] Limit Unix tests to run on Linux --- src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index 8d9dd93dd5b7..38411818a5de 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -125,6 +125,7 @@ public void ProcessStart_UseShellExecute_OnUnix_OpenMissingFile_DoesNotThrow() [Theory, InlineData(true), InlineData(false)] [OuterLoop("Opens program")] + [PlatformSpecific(TestPlatforms.Linux)] // s_allowedProgramsToRun is Linux specific public void ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled(bool isFolder) { string programToOpen = s_allowedProgramsToRun.FirstOrDefault(program => IsProgramInstalled(program)); @@ -156,6 +157,7 @@ public void ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled(bool } [Fact] + [PlatformSpecific(TestPlatforms.Linux)] // s_allowedProgramsToRun is Linux specific public void ProcessStart_UseShellExecute_OnUnix_FallsBackWhenNotRealExecutable() { // Create a script that we'll use to 'open' the file by putting it on PATH @@ -190,6 +192,7 @@ public void ProcessStart_UseShellExecute_OnUnix_FallsBackWhenNotRealExecutable() [InlineData("", true)] [InlineData("open", true)] [InlineData("invalid", false)] + [PlatformSpecific(TestPlatforms.Linux)] // s_allowedProgramsToRun is Linux specific public void ProcessStart_UseShellExecute_OnUnix_ValidVerbs(string verb, bool isValid) { // Create a script that we'll use to 'open' the file by putting it on PATH From a11365cdab5108cc2d435322db0595e04145b4cd Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 6 Nov 2018 12:22:51 +0100 Subject: [PATCH 08/19] Fix test for verb null --- src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index 38411818a5de..864cc3a4fe38 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -205,6 +205,11 @@ public void ProcessStart_UseShellExecute_OnUnix_ValidVerbs(string verb, bool isV options.StartInfo.EnvironmentVariables["PATH"] = path; RemoteInvoke((argVerb, argValid) => { + if (argVerb == "") + { + argVerb = null; + } + var psi = new ProcessStartInfo { UseShellExecute = true, FileName = "/", Verb = argVerb }; if (bool.Parse(argValid)) { @@ -220,7 +225,7 @@ public void ProcessStart_UseShellExecute_OnUnix_ValidVerbs(string verb, bool isV { Assert.Throws(() => Process.Start(psi)); } - }, verb, isValid.ToString(), options).Dispose(); + }, verb ?? "", isValid.ToString(), options).Dispose(); } [Theory, InlineData("vi")] From 15d5e291cdc6aa7cc1f5055c9f6ca890c9163287 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 7 Nov 2018 10:28:29 +0100 Subject: [PATCH 09/19] Improve verb check --- .../src/System/Diagnostics/Process.Unix.cs | 29 +++++++++---------- .../tests/ProcessTests.Unix.cs | 1 + 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index 695b27437e2b..d3a516acad9d 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -305,7 +305,19 @@ private bool StartCore(ProcessStartInfo startInfo) if (startInfo.UseShellExecute) { - if (!IsValidVerb(startInfo.Verb)) + string verb = startInfo.Verb; + if (string.IsNullOrEmpty(verb)) + { + // Default to 'open'. + verb = "open"; + } + else + { + // Verbs are case-insensitive. + verb = verb.ToLowerInvariant(); + } + + if (verb != "open") { throw new Win32Exception(Interop.Errors.ERROR_NO_ASSOCIATION); } @@ -763,21 +775,6 @@ private ProcessWaitState GetWaitState() return _waitStateHolder._state; } - private static readonly string[] s_validVerbs = new string[] { null, "", "open" }; - - private static bool IsValidVerb(string verb) - { - for (int i = 0; i < s_validVerbs.Length; i++) - { - if (s_validVerbs[i] == verb) - { - return true; - } - } - - return false; - } - private static (uint userId, uint groupId) GetUserAndGroupIds(ProcessStartInfo startInfo) { Debug.Assert(!string.IsNullOrEmpty(startInfo.UserName)); diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index 864cc3a4fe38..d03e42540081 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -191,6 +191,7 @@ public void ProcessStart_UseShellExecute_OnUnix_FallsBackWhenNotRealExecutable() [InlineData((string)null, true)] [InlineData("", true)] [InlineData("open", true)] + [InlineData("Open", true)] [InlineData("invalid", false)] [PlatformSpecific(TestPlatforms.Linux)] // s_allowedProgramsToRun is Linux specific public void ProcessStart_UseShellExecute_OnUnix_ValidVerbs(string verb, bool isValid) From 76b092a2c8029d38e0caa63c6e211a40af9b408f Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 7 Nov 2018 10:50:02 +0100 Subject: [PATCH 10/19] test: make working directory a temp location --- src/System.Diagnostics.Process/tests/ProcessTests.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.cs b/src/System.Diagnostics.Process/tests/ProcessTests.cs index 40e116196473..a411df3e64b6 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.cs @@ -252,8 +252,13 @@ public void ProcessStart_UseShellExecute_ExecuteOrder() string path = GetTestFileName(); Directory.CreateDirectory(path); + // Create a directory that will be our working directory + string wd = GetTestFileName(); + Directory.CreateDirectory(wd); + RemoteInvokeOptions options = new RemoteInvokeOptions(); options.StartInfo.EnvironmentVariables["PATH"] = path; + options.startInfo.WorkingDirectory = wd; RemoteInvoke(pathDirectory => { // Create two identically named scripts, one in the working directory and one on PATH. From 84b28b0e576a20b541447ff1bed561761ce4bcca Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 7 Nov 2018 11:21:35 +0100 Subject: [PATCH 11/19] test: refactor WriteScriptFile --- .../tests/ProcessTests.Unix.cs | 7 +++--- .../tests/ProcessTests.cs | 25 ++++++++----------- 2 files changed, 13 insertions(+), 19 deletions(-) diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index d03e42540081..84e6c44e2236 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -164,11 +164,10 @@ public void ProcessStart_UseShellExecute_OnUnix_FallsBackWhenNotRealExecutable() // with the appropriate name. string path = GetTestFileName(); Directory.CreateDirectory(path); - WriteScriptFile(Path.Combine(path, s_allowedProgramsToRun[0]), returnValue: 42); + WriteScriptFile(path, s_allowedProgramsToRun[0], returnValue: 42); // Create a file that has the x-bit set, but which isn't a valid script. - string filename = GetTestFileName(); - WriteScriptFile(filename, returnValue: 0); + string filename = WriteScriptFile(TestDirectory, GetTestFileName(), returnValue: 0); File.WriteAllText(filename, $"not a script"); int mode = Convert.ToInt32("744", 8); Assert.Equal(0, chmod(filename, mode)); @@ -200,7 +199,7 @@ public void ProcessStart_UseShellExecute_OnUnix_ValidVerbs(string verb, bool isV // with the appropriate name. string path = GetTestFileName(); Directory.CreateDirectory(path); - WriteScriptFile(Path.Combine(path, s_allowedProgramsToRun[0]), returnValue: 42); + WriteScriptFile(path, s_allowedProgramsToRun[0], returnValue: 42); RemoteInvokeOptions options = new RemoteInvokeOptions(); options.StartInfo.EnvironmentVariables["PATH"] = path; diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.cs b/src/System.Diagnostics.Process/tests/ProcessTests.cs index a411df3e64b6..2381c5ed21f6 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.cs @@ -224,12 +224,7 @@ public void ProcessStart_UseShellExecute_OnWindows_DoesNotThrow(bool isFolder) [InlineData(true), InlineData(false)] public void ProcessStart_UseShellExecute_Executes(bool filenameAsUrl) { - string filename = GetTestFilePath(); - if (PlatformDetection.IsWindows) - { - filename += ".bat"; - } - WriteScriptFile(filename, returnValue: 42); + string filename = WriteScriptFile(TestDirectory, GetTestFileName(), returnValue: 42); if (filenameAsUrl) { @@ -258,19 +253,16 @@ public void ProcessStart_UseShellExecute_ExecuteOrder() RemoteInvokeOptions options = new RemoteInvokeOptions(); options.StartInfo.EnvironmentVariables["PATH"] = path; - options.startInfo.WorkingDirectory = wd; + options.StartInfo.WorkingDirectory = wd; RemoteInvoke(pathDirectory => { // Create two identically named scripts, one in the working directory and one on PATH. - string scriptFilename = $"script_{nameof(ProcessStart_UseShellExecute_ExecuteOrder)}"; - if (PlatformDetection.IsWindows) - { - scriptFilename += ".bat"; - } const int workingDirReturnValue = 1; const int pathDirReturnValue = 2; - WriteScriptFile(Path.Combine(pathDirectory, scriptFilename), returnValue: pathDirReturnValue); - WriteScriptFile(Path.Combine(Directory.GetCurrentDirectory(), scriptFilename), returnValue: workingDirReturnValue); + string pathScriptFile = WriteScriptFile(pathDirectory, "script", returnValue: pathDirReturnValue); + string wdScriptFile = WriteScriptFile(Directory.GetCurrentDirectory(), "script", returnValue: workingDirReturnValue); + string scriptFilename = Path.GetFileName(pathScriptFile); + Assert.Equal(scriptFilename, Path.GetFileName(wdScriptFile)); // Execute the script and verify we prefer the one in the working directory. using (var process = Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName = scriptFilename })) @@ -291,10 +283,12 @@ public void ProcessStart_UseShellExecute_ExecuteOrder() }, path, options).Dispose(); } - private void WriteScriptFile(string filename, int returnValue) + private string WriteScriptFile(string directory, string name, int returnValue) { + string filename = Path.Combine(directory, name); if (PlatformDetection.IsWindows) { + filename += ".bat"; File.WriteAllText(filename, $"exit {returnValue}"); } else @@ -304,6 +298,7 @@ private void WriteScriptFile(string filename, int returnValue) int mode = Convert.ToInt32("744", 8); Assert.Equal(0, chmod(filename, mode)); } + return filename; } [Fact] From 9773b1c95fa9051a8992594d653a6b33a9dac0bc Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 7 Nov 2018 11:33:21 +0100 Subject: [PATCH 12/19] test: refactor temp dir creation --- .../src/System/IO/FileCleanupTestBase.cs | 8 ++++++++ src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs | 6 ++---- src/System.Diagnostics.Process/tests/ProcessTests.cs | 7 ++----- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/CoreFx.Private.TestUtilities/src/System/IO/FileCleanupTestBase.cs b/src/CoreFx.Private.TestUtilities/src/System/IO/FileCleanupTestBase.cs index 8aa81f084928..d1ca590e68fe 100644 --- a/src/CoreFx.Private.TestUtilities/src/System/IO/FileCleanupTestBase.cs +++ b/src/CoreFx.Private.TestUtilities/src/System/IO/FileCleanupTestBase.cs @@ -76,6 +76,14 @@ protected virtual void Dispose(bool disposing) /// protected string TestDirectory { get; } + /// Creates a test directory that is associated with the call site. + protected string CreateTestDirectory(int? index = null, [CallerMemberName] string memberName = null, [CallerLineNumber] int lineNumber = 0) + { + string path = GetTestFilePath(index, memberName, lineNumber); + Directory.CreateDirectory(path); + return path; + } + /// Gets a test file full path that is associated with the call site. /// An optional index value to use as a suffix on the file name. Typically a loop index. /// The member name of the function calling this method. diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index 84e6c44e2236..bb96fc8ce5bc 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -162,8 +162,7 @@ public void ProcessStart_UseShellExecute_OnUnix_FallsBackWhenNotRealExecutable() { // Create a script that we'll use to 'open' the file by putting it on PATH // with the appropriate name. - string path = GetTestFileName(); - Directory.CreateDirectory(path); + string path = CreateTestDirectory(); WriteScriptFile(path, s_allowedProgramsToRun[0], returnValue: 42); // Create a file that has the x-bit set, but which isn't a valid script. @@ -197,8 +196,7 @@ public void ProcessStart_UseShellExecute_OnUnix_ValidVerbs(string verb, bool isV { // Create a script that we'll use to 'open' the file by putting it on PATH // with the appropriate name. - string path = GetTestFileName(); - Directory.CreateDirectory(path); + string path = CreateTestDirectory(); WriteScriptFile(path, s_allowedProgramsToRun[0], returnValue: 42); RemoteInvokeOptions options = new RemoteInvokeOptions(); diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.cs b/src/System.Diagnostics.Process/tests/ProcessTests.cs index 2381c5ed21f6..75c8611dc430 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.cs @@ -244,12 +244,9 @@ public void ProcessStart_UseShellExecute_Executes(bool filenameAsUrl) public void ProcessStart_UseShellExecute_ExecuteOrder() { // Create a directory that we will use as PATH - string path = GetTestFileName(); - Directory.CreateDirectory(path); - + string path = CreateTestDirectory(); // Create a directory that will be our working directory - string wd = GetTestFileName(); - Directory.CreateDirectory(wd); + string wd = CreateTestDirectory(); RemoteInvokeOptions options = new RemoteInvokeOptions(); options.StartInfo.EnvironmentVariables["PATH"] = path; From b9951150a857a4bcaf66e23b698e440c90be470b Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 7 Nov 2018 13:03:48 +0100 Subject: [PATCH 13/19] test: fix build failure --- .../ref/CoreFx.Private.TestUtilities.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/CoreFx.Private.TestUtilities/ref/CoreFx.Private.TestUtilities.cs b/src/CoreFx.Private.TestUtilities/ref/CoreFx.Private.TestUtilities.cs index 28114ba3fb5d..0b50908aad33 100644 --- a/src/CoreFx.Private.TestUtilities/ref/CoreFx.Private.TestUtilities.cs +++ b/src/CoreFx.Private.TestUtilities/ref/CoreFx.Private.TestUtilities.cs @@ -188,6 +188,7 @@ protected FileCleanupTestBase() { } public void Dispose() { } protected virtual void Dispose(bool disposing) { } ~FileCleanupTestBase() { } + protected string CreateTestDirectory(System.Nullable index = default(System.Nullable), [System.Runtime.CompilerServices.CallerMemberNameAttribute]string memberName = null, [System.Runtime.CompilerServices.CallerLineNumberAttribute]int lineNumber = 0) { throw null; } protected string GetTestFileName(System.Nullable index = default(System.Nullable), [System.Runtime.CompilerServices.CallerMemberNameAttribute]string memberName = null, [System.Runtime.CompilerServices.CallerLineNumberAttribute]int lineNumber = 0) { throw null; } protected string GetTestFilePath(System.Nullable index = default(System.Nullable), [System.Runtime.CompilerServices.CallerMemberNameAttribute]string memberName = null, [System.Runtime.CompilerServices.CallerLineNumberAttribute]int lineNumber = 0) { throw null; } } From 946b7d267ea74c3acea5b747d73ed41b09fd3005 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 7 Nov 2018 22:53:15 +0100 Subject: [PATCH 14/19] Condense verb check --- .../src/System/Diagnostics/Process.Unix.cs | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index d3a516acad9d..32ef143a479f 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -306,18 +306,8 @@ private bool StartCore(ProcessStartInfo startInfo) if (startInfo.UseShellExecute) { string verb = startInfo.Verb; - if (string.IsNullOrEmpty(verb)) - { - // Default to 'open'. - verb = "open"; - } - else - { - // Verbs are case-insensitive. - verb = verb.ToLowerInvariant(); - } - - if (verb != "open") + if (!string.IsNullOrEmpty(verb) && + !string.Equals(verb, "open", StringComparison.InvariantCultureIgnoreCase)) { throw new Win32Exception(Interop.Errors.ERROR_NO_ASSOCIATION); } From 39bfa71bbe5773b364fe2a5d33967714a5e87939 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 8 Nov 2018 12:41:53 +0100 Subject: [PATCH 15/19] Find the executable in the ProcessStartInfo.WorkingDirectory --- .../src/System/Diagnostics/Process.Unix.cs | 15 ++++++++----- .../tests/ProcessTests.cs | 21 +++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index 32ef143a479f..de511008c8e3 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -318,7 +318,7 @@ private bool StartCore(ProcessStartInfo startInfo) // when a Windows filesystem is mounted on Linux. To handle that, treat it as a regular file // when exec returns ENOEXEC (file format cannot be executed). bool isExecuting = false; - filename = ResolveExecutableForShellExecute(startInfo.FileName); + filename = ResolveExecutableForShellExecute(startInfo.FileName, cwd); if (filename != null) { argv = ParseArgv(startInfo); @@ -502,7 +502,7 @@ private static string[] CreateEnvp(ProcessStartInfo psi) return envp; } - private static string ResolveExecutableForShellExecute(string filename) + private static string ResolveExecutableForShellExecute(string filename, string workingDirectory) { // Determine if filename points to an executable file. // filename may be an absolute path, a relative path or a uri. @@ -524,13 +524,18 @@ private static string ResolveExecutableForShellExecute(string filename) resolvedFilename = uri.LocalPath; } } + // filename is relative else { - string relativeFile = Path.Combine(Directory.GetCurrentDirectory(), filename); + // The WorkingDirectory property specifies the location of the executable. + // If WorkingDirectory is an empty string, the current directory is understood to contain the executable. + workingDirectory = workingDirectory != null ? Path.GetFullPath(workingDirectory) : + Directory.GetCurrentDirectory(); + string filenameInWorkingDirectory = Path.Combine(workingDirectory, filename); // filename is a relative path in the working directory - if (File.Exists(relativeFile)) + if (File.Exists(filenameInWorkingDirectory)) { - resolvedFilename = relativeFile; + resolvedFilename = filenameInWorkingDirectory; } // find filename on PATH else diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.cs b/src/System.Diagnostics.Process/tests/ProcessTests.cs index 75c8611dc430..c3ce977d1f41 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.cs @@ -280,6 +280,27 @@ public void ProcessStart_UseShellExecute_ExecuteOrder() }, path, options).Dispose(); } + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsServerCore), + nameof(PlatformDetection.IsNotWindowsNanoServer), nameof(PlatformDetection.IsNotWindowsIoTCore))] + [SkipOnTargetFramework(TargetFrameworkMonikers.Uap, "not supported on UAP")] + public void ProcessStart_UseShellExecute_WorkingDirectory() + { + // Create a directory that will ProcessStartInfo.WorkingDirectory + // and add a script. + string wd = CreateTestDirectory(); + string filename = Path.GetFileName(WriteScriptFile(wd, GetTestFileName(), returnValue: 42)); + + // Verify UseShellExecute finds the script in the WorkingDirectory. + Assert.False(Path.IsPathRooted(filename)); + using (var process = Process.Start(new ProcessStartInfo { UseShellExecute = true, + FileName = filename, + WorkingDirectory = wd })) + { + process.WaitForExit(); + Assert.Equal(42, process.ExitCode); + } + } + private string WriteScriptFile(string directory, string name, int returnValue) { string filename = Path.Combine(directory, name); From 0c178b3b970b7709a19049371425fb83aef103e1 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 8 Nov 2018 14:28:12 +0100 Subject: [PATCH 16/19] Don't pass Arguments when opening a document --- .../src/System/Diagnostics/Process.Unix.cs | 38 ++++++++++------- .../tests/ProcessTests.Unix.cs | 42 +++++++++++++++++++ 2 files changed, 64 insertions(+), 16 deletions(-) diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index de511008c8e3..3099b0d8fed5 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -334,7 +334,7 @@ private bool StartCore(ProcessStartInfo startInfo) if (!isExecuting) { filename = GetPathToOpenFile(); - argv = ParseArgv(startInfo, filename); + argv = ParseArgv(startInfo, filename, ignoreArguments: true); ForkAndExecProcess(filename, argv, envp, cwd, startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError, @@ -456,34 +456,40 @@ private bool ForkAndExecProcess( /// Converts the filename and arguments information from a ProcessStartInfo into an argv array. /// The ProcessStartInfo. - /// alternative resolved path to use as first argument + /// Resolved executable to open ProcessStartInfo.FileName + /// Don't pass ProcessStartInfo.Arguments /// The argv array. - private static string[] ParseArgv(ProcessStartInfo psi, string alternativePath = null) + private static string[] ParseArgv(ProcessStartInfo psi, string resolvedExe = null, bool ignoreArguments = false) { - string argv0 = psi.FileName; // when no alternative path exists, pass filename (instead of resolved path) as argv[0], to match what caller supplied - if (string.IsNullOrEmpty(psi.Arguments) && string.IsNullOrEmpty(alternativePath) && psi.ArgumentList.Count == 0) + // Avoid building List. + if (string.IsNullOrEmpty(resolvedExe) && + (ignoreArguments || (string.IsNullOrEmpty(psi.Arguments) && psi.ArgumentList.Count == 0))) { - return new string[] { argv0 }; + return new string[] { psi.FileName }; } var argvList = new List(); - if (!string.IsNullOrEmpty(alternativePath)) + if (!string.IsNullOrEmpty(resolvedExe)) { - argvList.Add(alternativePath); - if (alternativePath.Contains("kfmclient")) + argvList.Add(resolvedExe); + if (resolvedExe.Contains("kfmclient")) { argvList.Add("openURL"); // kfmclient needs OpenURL } } - argvList.Add(argv0); - if (!string.IsNullOrEmpty(psi.Arguments)) - { - ParseArgumentsIntoList(psi.Arguments, argvList); - } - else + argvList.Add(psi.FileName); + + if (!ignoreArguments) { - argvList.AddRange(psi.ArgumentList); + if (!string.IsNullOrEmpty(psi.Arguments)) + { + ParseArgumentsIntoList(psi.Arguments, argvList); + } + else + { + argvList.AddRange(psi.ArgumentList); + } } return argvList.ToArray(); } diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index bb96fc8ce5bc..647ae9693e24 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -185,6 +185,48 @@ public void ProcessStart_UseShellExecute_OnUnix_FallsBackWhenNotRealExecutable() }, filename, options).Dispose(); } + [Fact] + [PlatformSpecific(TestPlatforms.Linux)] // test relies on xdg-open + public void ProcessStart_UseShellExecute_OnUnix_DocumentFile_IgnoresArguments() + { + Assert.Equal(s_allowedProgramsToRun[0], "xdg-open"); + + if (!IsProgramInstalled("xdg-open")) + { + return; + } + + // Open a file that doesn't exist with an argument that xdg-open considers invalid. + using (var px = Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName = "/nosuchfile", Arguments = "invalid_arg" })) + { + Assert.NotNull(px); + px.WaitForExit(); + // xdg-open returns different failure exit codes, 1 indicates an error in command line syntax. + Assert.NotEqual(0, px.ExitCode); // the command failed + Assert.NotEqual(1, px.ExitCode); // the failure is not due to the invalid argument + } + } + + [Fact] + [PlatformSpecific(TestPlatforms.Linux)] + public void ProcessStart_UseShellExecute_OnUnix_Executable_PassesArguments() + { + string testFilePath = GetTestFilePath(); + Assert.False(File.Exists(testFilePath)); + + // Start a process that will create a file pass the filename as Arguments. + using (var px = Process.Start(new ProcessStartInfo { UseShellExecute = true, + FileName = "touch", + Arguments = testFilePath })) + { + Assert.NotNull(px); + px.WaitForExit(); + Assert.Equal(0, px.ExitCode); + } + + Assert.True(File.Exists(testFilePath)); + } + [Theory] [InlineData((string)null, true)] [InlineData("", true)] From fe1f74a67509e6e96591b1b7895ff960ed8380c3 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 21 Nov 2018 15:48:41 +0100 Subject: [PATCH 17/19] PR Feedback --- .../ref/CoreFx.Private.TestUtilities.cs | 1 - .../src/System/IO/FileCleanupTestBase.cs | 8 -------- .../src/System/Diagnostics/Process.Unix.cs | 4 ++-- .../tests/ProcessTests.Unix.cs | 7 ++++--- src/System.Diagnostics.Process/tests/ProcessTests.cs | 9 ++++++--- 5 files changed, 12 insertions(+), 17 deletions(-) diff --git a/src/CoreFx.Private.TestUtilities/ref/CoreFx.Private.TestUtilities.cs b/src/CoreFx.Private.TestUtilities/ref/CoreFx.Private.TestUtilities.cs index 0b50908aad33..28114ba3fb5d 100644 --- a/src/CoreFx.Private.TestUtilities/ref/CoreFx.Private.TestUtilities.cs +++ b/src/CoreFx.Private.TestUtilities/ref/CoreFx.Private.TestUtilities.cs @@ -188,7 +188,6 @@ protected FileCleanupTestBase() { } public void Dispose() { } protected virtual void Dispose(bool disposing) { } ~FileCleanupTestBase() { } - protected string CreateTestDirectory(System.Nullable index = default(System.Nullable), [System.Runtime.CompilerServices.CallerMemberNameAttribute]string memberName = null, [System.Runtime.CompilerServices.CallerLineNumberAttribute]int lineNumber = 0) { throw null; } protected string GetTestFileName(System.Nullable index = default(System.Nullable), [System.Runtime.CompilerServices.CallerMemberNameAttribute]string memberName = null, [System.Runtime.CompilerServices.CallerLineNumberAttribute]int lineNumber = 0) { throw null; } protected string GetTestFilePath(System.Nullable index = default(System.Nullable), [System.Runtime.CompilerServices.CallerMemberNameAttribute]string memberName = null, [System.Runtime.CompilerServices.CallerLineNumberAttribute]int lineNumber = 0) { throw null; } } diff --git a/src/CoreFx.Private.TestUtilities/src/System/IO/FileCleanupTestBase.cs b/src/CoreFx.Private.TestUtilities/src/System/IO/FileCleanupTestBase.cs index d1ca590e68fe..8aa81f084928 100644 --- a/src/CoreFx.Private.TestUtilities/src/System/IO/FileCleanupTestBase.cs +++ b/src/CoreFx.Private.TestUtilities/src/System/IO/FileCleanupTestBase.cs @@ -76,14 +76,6 @@ protected virtual void Dispose(bool disposing) /// protected string TestDirectory { get; } - /// Creates a test directory that is associated with the call site. - protected string CreateTestDirectory(int? index = null, [CallerMemberName] string memberName = null, [CallerLineNumber] int lineNumber = 0) - { - string path = GetTestFilePath(index, memberName, lineNumber); - Directory.CreateDirectory(path); - return path; - } - /// Gets a test file full path that is associated with the call site. /// An optional index value to use as a suffix on the file name. Typically a loop index. /// The member name of the function calling this method. diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index 3099b0d8fed5..c138f784a049 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -306,8 +306,8 @@ private bool StartCore(ProcessStartInfo startInfo) if (startInfo.UseShellExecute) { string verb = startInfo.Verb; - if (!string.IsNullOrEmpty(verb) && - !string.Equals(verb, "open", StringComparison.InvariantCultureIgnoreCase)) + if (verb != null && + !string.Equals(verb, "open", StringComparison.OrdinalIgnoreCase)) { throw new Win32Exception(Interop.Errors.ERROR_NO_ASSOCIATION); } diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index 647ae9693e24..a687dd6a9e35 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -125,7 +125,6 @@ public void ProcessStart_UseShellExecute_OnUnix_OpenMissingFile_DoesNotThrow() [Theory, InlineData(true), InlineData(false)] [OuterLoop("Opens program")] - [PlatformSpecific(TestPlatforms.Linux)] // s_allowedProgramsToRun is Linux specific public void ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled(bool isFolder) { string programToOpen = s_allowedProgramsToRun.FirstOrDefault(program => IsProgramInstalled(program)); @@ -162,7 +161,8 @@ public void ProcessStart_UseShellExecute_OnUnix_FallsBackWhenNotRealExecutable() { // Create a script that we'll use to 'open' the file by putting it on PATH // with the appropriate name. - string path = CreateTestDirectory(); + string path = Path.Combine(TestDirectory, "Path"); + Directory.CreateDirectory(path); WriteScriptFile(path, s_allowedProgramsToRun[0], returnValue: 42); // Create a file that has the x-bit set, but which isn't a valid script. @@ -238,7 +238,8 @@ public void ProcessStart_UseShellExecute_OnUnix_ValidVerbs(string verb, bool isV { // Create a script that we'll use to 'open' the file by putting it on PATH // with the appropriate name. - string path = CreateTestDirectory(); + string path = Path.Combine(TestDirectory, "Path"); + Directory.CreateDirectory(path); WriteScriptFile(path, s_allowedProgramsToRun[0], returnValue: 42); RemoteInvokeOptions options = new RemoteInvokeOptions(); diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.cs b/src/System.Diagnostics.Process/tests/ProcessTests.cs index c3ce977d1f41..aacd6871b5d6 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.cs @@ -244,9 +244,11 @@ public void ProcessStart_UseShellExecute_Executes(bool filenameAsUrl) public void ProcessStart_UseShellExecute_ExecuteOrder() { // Create a directory that we will use as PATH - string path = CreateTestDirectory(); + string path = Path.Combine(TestDirectory, "Path"); + Directory.CreateDirectory(path); // Create a directory that will be our working directory - string wd = CreateTestDirectory(); + string wd = Path.Combine(TestDirectory, "WorkingDirectory"); + Directory.CreateDirectory(wd); RemoteInvokeOptions options = new RemoteInvokeOptions(); options.StartInfo.EnvironmentVariables["PATH"] = path; @@ -287,7 +289,8 @@ public void ProcessStart_UseShellExecute_WorkingDirectory() { // Create a directory that will ProcessStartInfo.WorkingDirectory // and add a script. - string wd = CreateTestDirectory(); + string wd = Path.Combine(TestDirectory, "WorkingDirectory"); + Directory.CreateDirectory(wd); string filename = Path.GetFileName(WriteScriptFile(wd, GetTestFileName(), returnValue: 42)); // Verify UseShellExecute finds the script in the WorkingDirectory. From fa1c0cdd2c52bb461ff0f8f4f2cf9e8bf0dc47d3 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 21 Nov 2018 17:59:00 +0100 Subject: [PATCH 18/19] Fix verb check --- .../src/System/Diagnostics/Process.Unix.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index c138f784a049..ec378393cdb5 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -306,7 +306,7 @@ private bool StartCore(ProcessStartInfo startInfo) if (startInfo.UseShellExecute) { string verb = startInfo.Verb; - if (verb != null && + if (verb != string.Empty && !string.Equals(verb, "open", StringComparison.OrdinalIgnoreCase)) { throw new Win32Exception(Interop.Errors.ERROR_NO_ASSOCIATION); From 86ef599f419c795e52a6f16acb40af6ddd01c106 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 28 Nov 2018 17:09:55 +0100 Subject: [PATCH 19/19] PR feedback --- .../Interop.ForkAndExecProcess.cs | 9 +------- .../src/System/Diagnostics/Process.Unix.cs | 5 ++--- .../tests/ProcessTests.Unix.cs | 13 ++++++++++++ .../tests/ProcessTests.Windows.cs | 19 +++++++++++++++++ .../tests/ProcessTests.cs | 21 ------------------- .../System.Diagnostics.Process.Tests.csproj | 1 + 6 files changed, 36 insertions(+), 32 deletions(-) create mode 100644 src/System.Diagnostics.Process/tests/ProcessTests.Windows.cs diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs b/src/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs index f404c64f66de..d986121faedf 100644 --- a/src/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs +++ b/src/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs @@ -29,14 +29,7 @@ internal static unsafe int ForkAndExecProcess( redirectStdin ? 1 : 0, redirectStdout ? 1 : 0, redirectStderr ? 1 :0, setUser ? 1 : 0, userId, groupId, out lpChildPid, out stdinFd, out stdoutFd, out stderrFd); - if (result != 0) - { - return Marshal.GetLastWin32Error(); - } - else - { - return 0; - } + return result == 0 ? 0 : Marshal.GetLastWin32Error(); } finally { diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index ec378393cdb5..39f764a20869 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -429,8 +429,8 @@ private bool ForkAndExecProcess( } else { - if ((throwOnNoExec == false) - && (Interop.Sys.ConvertErrorPlatformToPal(errno) == Interop.Error.ENOEXEC)) + if (!throwOnNoExec && + new Interop.ErrorInfo(errno).Error == Interop.Error.ENOEXEC) { return false; } @@ -461,7 +461,6 @@ private bool ForkAndExecProcess( /// The argv array. private static string[] ParseArgv(ProcessStartInfo psi, string resolvedExe = null, bool ignoreArguments = false) { - // Avoid building List. if (string.IsNullOrEmpty(resolvedExe) && (ignoreArguments || (string.IsNullOrEmpty(psi.Arguments) && psi.ArgumentList.Count == 0))) { diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index a687dd6a9e35..9c0d9a1733fc 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -825,6 +825,9 @@ private void RunTestAsSudo(Func testMethod, string arg) Assert.Equal(0, p.ExitCode); } + [DllImport("libc")] + private static extern int chmod(string path, int mode); + [DllImport("libc")] private static extern uint geteuid(); @@ -832,5 +835,15 @@ private void RunTestAsSudo(Func testMethod, string arg) private static extern int seteuid(uint euid); private static readonly string[] s_allowedProgramsToRun = new string[] { "xdg-open", "gnome-open", "kfmclient" }; + + private string WriteScriptFile(string directory, string name, int returnValue) + { + string filename = Path.Combine(directory, name); + File.WriteAllText(filename, $"#!/bin/sh\nexit {returnValue}\n"); + // set x-bit + int mode = Convert.ToInt32("744", 8); + Assert.Equal(0, chmod(filename, mode)); + return filename; + } } } diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Windows.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Windows.cs new file mode 100644 index 000000000000..d45ff2afc10f --- /dev/null +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Windows.cs @@ -0,0 +1,19 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.IO; + +namespace System.Diagnostics.Tests +{ + public partial class ProcessTests + { + private string WriteScriptFile(string directory, string name, int returnValue) + { + string filename = Path.Combine(directory, name); + filename += ".bat"; + File.WriteAllText(filename, $"exit {returnValue}"); + return filename; + } + } +} diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.cs b/src/System.Diagnostics.Process/tests/ProcessTests.cs index aacd6871b5d6..14855808ae61 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.cs @@ -304,24 +304,6 @@ public void ProcessStart_UseShellExecute_WorkingDirectory() } } - private string WriteScriptFile(string directory, string name, int returnValue) - { - string filename = Path.Combine(directory, name); - if (PlatformDetection.IsWindows) - { - filename += ".bat"; - File.WriteAllText(filename, $"exit {returnValue}"); - } - else - { - File.WriteAllText(filename, $"#!/bin/sh\nexit {returnValue}\n"); - // set x-bit - int mode = Convert.ToInt32("744", 8); - Assert.Equal(0, chmod(filename, mode)); - } - return filename; - } - [Fact] [ActiveIssue(31908, TargetFrameworkMonikers.Uap)] public void TestExitCode() @@ -1836,8 +1818,5 @@ private SecureString AsSecureString(string str) return secureString; } - - [DllImport("libc")] - private static extern int chmod(string path, int mode); } } diff --git a/src/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj b/src/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj index fec63bc1be22..07e38b048445 100644 --- a/src/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj +++ b/src/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj @@ -30,6 +30,7 @@ +