Make Process.Start throw Win32Exception when child process doesn't exec. (#25266)

tmds · stephentoub · commit c53b84b0a95d · 2017-12-01T10:44:08.000-05:00
* Make Process.Start throw Win32Exception when child process doesn't exec.

* Update tests

* Fix test name

* On Windows, use powershell.exe for test

* Only run test when test program is found

* Make read and write more robust

* TestStartWithBadWorkingDirectory: skip on UWP

* TestStartWithBadWorkingDirectory: skip UWP using Attribute
diff --git a/src/Native/Unix/System.Native/pal_process.cpp b/src/Native/Unix/System.Native/pal_process.cpp
@@ -92,6 +92,58 @@ static int Dup2WithInterruptedRetry(int oldfd, int newfd)
     return result;
 }
 
+static ssize_t WriteSize(int fd, const void* buffer, size_t count)
+{
+    ssize_t rv = 0;
+    while (count > 0)
+    {
+        ssize_t result = 0;
+        while (CheckInterrupted(result = write(fd, buffer, count)));
+        if (result > 0)
+        {
+            rv += result;
+            buffer = reinterpret_cast<const uint8_t*>(buffer) + result;
+            count -= static_cast<size_t>(result);
+        }
+        else
+        {
+            return -1;
+        }
+    }
+    return rv;
+}
+
+static ssize_t ReadSize(int fd, void* buffer, size_t count)
+{
+    ssize_t rv = 0;
+    while (count > 0)
+    {
+        ssize_t result = 0;
+        while (CheckInterrupted(result = read(fd, buffer, count)));
+        if (result > 0)
+        {
+            rv += result;
+            buffer = reinterpret_cast<uint8_t*>(buffer) + result;
+            count -= static_cast<size_t>(result);
+        }
+        else
+        {
+            return -1;
+        }
+    }
+    return rv;
+}
+
+__attribute__((noreturn))
+static void ExitChild(int pipeToParent, int error)
+{
+    if (pipeToParent != -1)
+    {
+        WriteSize(pipeToParent, &error, sizeof(error));
+    }
+    _exit(error != 0 ? error : EXIT_FAILURE);
+}
+
 extern "C" int32_t SystemNative_ForkAndExecProcess(const char* filename,
                                       char* const argv[],
                                       char* const envp[],
@@ -153,6 +205,7 @@ extern "C" int32_t SystemNative_ForkAndExecProcess(const char* filename,
     // child process is actually transitioned to the target program.  This avoids problems
     // where the parent process uses members of Process, like ProcessName, when the Process
     // is still the clone of this one. This is a best-effort attempt, so ignore any errors.
+    // If the child fails to exec we use the pipe to pass the errno to the parent process.
 #if HAVE_PIPE2
     pipe2(waitForChildToExecPipe, O_CLOEXEC);
 #endif
@@ -172,7 +225,7 @@ extern "C" int32_t SystemNative_ForkAndExecProcess(const char* filename,
             (redirectStdout && Dup2WithInterruptedRetry(stdoutFds[WRITE_END_OF_PIPE], STDOUT_FILENO) == -1) ||
             (redirectStderr && Dup2WithInterruptedRetry(stderrFds[WRITE_END_OF_PIPE], STDERR_FILENO) == -1))
         {
-            _exit(errno != 0 ? errno : EXIT_FAILURE);
+            ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno);
         }
 
         // Change to the designated working directory, if one was specified
@@ -182,13 +235,13 @@ extern "C" int32_t SystemNative_ForkAndExecProcess(const char* filename,
             while (CheckInterrupted(result = chdir(cwd)));
             if (result == -1)
             {
-                _exit(errno != 0 ? errno : EXIT_FAILURE);
+                ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno);
             }
         }
 
         // Finally, execute the new process.  execve will not return if it's successful.
         execve(filename, argv, envp);
-        _exit(errno != 0 ? errno : EXIT_FAILURE); // execve failed
+        ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno); // execve failed
     }
 
     // This is the parent process. processId == pid of the child
@@ -212,10 +265,15 @@ extern "C" int32_t SystemNative_ForkAndExecProcess(const char* filename,
     CloseIfOpen(waitForChildToExecPipe[WRITE_END_OF_PIPE]);
     if (waitForChildToExecPipe[READ_END_OF_PIPE] != -1)
     {
-        int ignored;
+        int childError;
         if (success)
         {
-            while (CheckInterrupted(read(waitForChildToExecPipe[READ_END_OF_PIPE], &ignored, 1)));
+            ssize_t result = ReadSize(waitForChildToExecPipe[READ_END_OF_PIPE], &childError, sizeof(childError));
+            if (result == sizeof(childError))
+            {
+                success = false;
+                priorErrno = childError;
+            }
         }
         CloseIfOpen(waitForChildToExecPipe[READ_END_OF_PIPE]);
     }
diff --git a/src/System.Diagnostics.Process/tests/Performance/System.Diagnostics.Process.Performance.Tests.csproj b/src/System.Diagnostics.Process/tests/Performance/System.Diagnostics.Process.Performance.Tests.csproj
@@ -17,6 +17,9 @@
     <Compile Include="$(CommonPath)\System\PasteArguments.cs">
       <Link>Common\System\PasteArguments.cs</Link>
     </Compile>
+    <Compile Include="$(CommonPath)\System\IO\StringParser.cs">
+      <Link>Common\System\IO\StringParser.cs</Link>
+    </Compile>
     <Compile Include="$(CommonTestPath)\System\PerfUtils.cs">
       <Link>Common\System\PerfUtils.cs</Link>
     </Compile>
diff --git a/src/System.Diagnostics.Process/tests/ProcessTestBase.cs b/src/System.Diagnostics.Process/tests/ProcessTestBase.cs
@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.
 
 using System.Collections.Generic;
+using System.IO;
 using System.Reflection;
 using System.Threading;
 using Xunit;
@@ -69,5 +70,31 @@ protected void StartSleepKillWait(Process p)
             p.Kill();
             Assert.True(p.WaitForExit(WaitInMS));
         }
+
+        /// <summary>
+        /// Checks if the program is installed
+        /// </summary>
+        /// <param name="program"></param>
+        /// <returns></returns>
+        protected static bool IsProgramInstalled(string program)
+        {
+            string path;
+            string pathEnvVar = Environment.GetEnvironmentVariable("PATH");
+            char separator = PlatformDetection.IsWindows ? ';' : ':';
+            if (pathEnvVar != null)
+            {
+                var pathParser = new StringParser(pathEnvVar, separator, skipEmpty: true);
+                while (pathParser.MoveNext())
+                {
+                    string subPath = pathParser.ExtractCurrent();
+                    path = Path.Combine(subPath, program);
+                    if (File.Exists(path))
+                    {
+                        return true;
+                    }
+                }
+            }
+            return false;
+        }
     }
 }
diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs
@@ -266,6 +266,7 @@ public void TestStartOnUnixWithBadPermissions()
         }
 
         [Fact]
+        [PlatformSpecific(~TestPlatforms.OSX)] // OSX doesn't support throwing on Process.Start
         public void TestStartOnUnixWithBadFormat()
         {
             string path = GetTestFilePath();
@@ -274,6 +275,20 @@ public void TestStartOnUnixWithBadFormat()
 
             Assert.Equal(0, chmod(path, mode)); // execute permissions
 
+            Win32Exception e = Assert.Throws<Win32Exception>(() => Process.Start(path));
+            Assert.NotEqual(0, e.NativeErrorCode);
+        }
+
+        [Fact]
+        [PlatformSpecific(TestPlatforms.OSX)] // OSX doesn't support throwing on Process.Start
+        public void TestStartOnOSXWithBadFormat()
+        {
+            string path = GetTestFilePath();
+            File.Create(path).Dispose();
+            int mode = Convert.ToInt32("744", 8);
+
+            Assert.Equal(0, chmod(path, mode)); // execute permissions
+
             using (Process p = Process.Start(path))
             {
                 p.WaitForExit();
@@ -285,30 +300,5 @@ public void TestStartOnUnixWithBadFormat()
         private static extern int chmod(string path, int mode);
 
         private readonly string[] s_allowedProgramsToRun = new string[] { "xdg-open", "gnome-open", "kfmclient" };
-
-        /// <summary>
-        /// Checks if the program is installed
-        /// </summary>
-        /// <param name="program"></param>
-        /// <returns></returns>
-        private bool IsProgramInstalled(string program)
-        {
-            string path;
-            string pathEnvVar = Environment.GetEnvironmentVariable("PATH");
-            if (pathEnvVar != null)
-            {
-                var pathParser = new StringParser(pathEnvVar, ':', skipEmpty: true);
-                while (pathParser.MoveNext())
-                {
-                    string subPath = pathParser.ExtractCurrent();
-                    path = Path.Combine(subPath, program);
-                    if (File.Exists(path))
-                    {
-                        return true;
-                    }
-                }
-            }
-            return false;
-        }
     }
 }
diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.cs b/src/System.Diagnostics.Process/tests/ProcessTests.cs
@@ -135,7 +135,43 @@ public void ProcessStart_TryOpenFolder_UseShellExecuteIsFalse_ThrowsWin32Excepti
         {
             Win32Exception e = Assert.Throws<Win32Exception>(() => Process.Start(new ProcessStartInfo { UseShellExecute = false, FileName = Path.GetTempPath() }));
         }
-        
+
+        [Fact]
+        [PlatformSpecific(~TestPlatforms.OSX)] // OSX doesn't support throwing on Process.Start
+        [SkipOnTargetFramework(TargetFrameworkMonikers.Uap)] // UWP overrides WorkingDirectory (https://github.com/dotnet/corefx/pull/25266#issuecomment-347719832).
+        public void TestStartWithBadWorkingDirectory()
+        {
+            string program;
+            string workingDirectory;
+            if (PlatformDetection.IsWindows)
+            {
+                program = "powershell.exe";
+                workingDirectory = @"C:\does-not-exist";
+            }
+            else
+            {
+                program = "uname";
+                workingDirectory = "/does-not-exist";
+            }
+
+            if (IsProgramInstalled(program))
+            {
+                var psi = new ProcessStartInfo
+                {
+                    FileName = program,
+                    UseShellExecute = false,
+                    WorkingDirectory = workingDirectory
+                };
+
+                Win32Exception e = Assert.Throws<Win32Exception>(() => Process.Start(psi));
+                Assert.NotEqual(0, e.NativeErrorCode);
+            }
+            else
+            {
+                Console.WriteLine($"Program {program} is not installed on this machine.");
+            }
+        }
+
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.HasWindowsShell))]
         [SkipOnTargetFramework(TargetFrameworkMonikers.Uap, "not supported on UAP")]
         [OuterLoop("Launches File Explorer")]
