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..d986121faedf 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, @@ -29,20 +29,7 @@ internal static partial class Sys 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) - { - // 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 result == 0 ? 0 : Marshal.GetLastWin32Error(); } 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..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 @@ -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..39f764a20869 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,56 @@ 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) { + string verb = startInfo.Verb; + if (verb != string.Empty && + !string.Equals(verb, "open", StringComparison.OrdinalIgnoreCase)) + { + 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 + // 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, cwd); + 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, ignoreArguments: true); + + ForkAndExecProcess(filename, argv, envp, cwd, + startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError, + setCredentials, userId, groupId, + out stdinFd, out stdoutFd, out stderrFd); + } } else { @@ -309,50 +350,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 +383,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 && + new Interop.ErrorInfo(errno).Error == Interop.Error.ENOEXEC) + { + return false; + } + + throw new Win32Exception(errno); + } + } + finally + { + s_processStartLock.ExitReadLock(); + } + } + // ----------------------------- // ---- PAL layer ends here ---- // ----------------------------- @@ -393,34 +456,39 @@ private bool StartCore(ProcessStartInfo startInfo) /// 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) + 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(); } @@ -439,6 +507,63 @@ private static string[] CreateEnvp(ProcessStartInfo psi) return envp; } + 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. + + 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 relative + else + { + // 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(filenameInWorkingDirectory)) + { + resolvedFilename = filenameInWorkingDirectory; + } + // 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. diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index dfce8f1ff60f..9c0d9a1733fc 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -155,6 +155,120 @@ 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 + // with the appropriate name. + 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. + 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)); + + 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); + px.WaitForExit(); + Assert.True(px.HasExited); + Assert.Equal(42, px.ExitCode); + } + }, 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)] + [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) + { + // Create a script that we'll use to 'open' the file by putting it on PATH + // with the appropriate name. + string path = Path.Combine(TestDirectory, "Path"); + Directory.CreateDirectory(path); + WriteScriptFile(path, s_allowedProgramsToRun[0], returnValue: 42); + + RemoteInvokeOptions options = new RemoteInvokeOptions(); + 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)) + { + 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")] @@ -721,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 ef42b7201c1c..14855808ae61 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.cs @@ -218,6 +218,92 @@ public void ProcessStart_UseShellExecute_OnWindows_DoesNotThrow(bool isFolder) } } + [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 = WriteScriptFile(TestDirectory, GetTestFileName(), 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); + } + } + + [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 + string path = Path.Combine(TestDirectory, "Path"); + Directory.CreateDirectory(path); + // Create a directory that will be our working directory + string wd = Path.Combine(TestDirectory, "WorkingDirectory"); + 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. + const int workingDirReturnValue = 1; + const int pathDirReturnValue = 2; + 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 })) + { + 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(); + } + + [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 = Path.Combine(TestDirectory, "WorkingDirectory"); + Directory.CreateDirectory(wd); + 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); + } + } + [Fact] [ActiveIssue(31908, TargetFrameworkMonikers.Uap)] public void TestExitCode() 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 @@ +