Process.Start() failure should include path (#46417)

* Adding ProcessStartException * Reverting ProcessStartException * First version * Fixing merge issue * Using interop call instead of intermediate exception to get error msg * Fixing typo * Change checks for exception message * Update src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Co-authored-by: Adam Sitnik <[email protected]> * Update src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs Co-authored-by: Adam Sitnik <[email protected]> * Using common method to create exception and updating error message * Fixing merge mistake * Using AssertExtensions.ThrowsContains * Fixing code for unix * Address PR feedback * Apply suggestions from code review Co-authored-by: Stephen Toub <[email protected]> * Fix ErrorInfo type reference Co-authored-by: Adam Sitnik <[email protected]> Co-authored-by: Jeff Handley <[email protected]> Co-authored-by: Jeff Handley <[email protected]> Co-authored-by: Stephen Toub <[email protected]>
dotnet · Jul 14, 2021 · 720279c · 720279c
1 parent 2e2670e
commit 720279c
Show file tree

Hide file tree

Showing 8 changed files with 51 additions and 30 deletions.
diff --git a/src/installer/pkg/snap/snapcraft.yaml b/src/installer/pkg/snap/snapcraft.yaml
diff --git a/src/libraries/System.Diagnostics.Process/src/Resources/Strings.resx b/src/libraries/System.Diagnostics.Process/src/Resources/Strings.resx
@@ -185,6 +185,9 @@
   <data name="InvalidApplication" xml:space="preserve">
     <value>The specified executable is not a valid application for this OS platform.</value>
   </data>
+  <data name="ErrorStartingProcess" xml:space="preserve">
+    <value>An error occurred trying to start process '{0}' with working directory '{1}'. {2}</value>
+  </data>
   <data name="StandardOutputEncodingNotAllowed" xml:space="preserve">
     <value>StandardOutputEncoding is only supported when standard output is redirected.</value>
   </data>

diff --git a/src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj b/src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj
@@ -46,6 +46,8 @@
   <ItemGroup Condition=" '$(TargetsWindows)' == 'true'">
     <Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.EnumProcessModules.cs"
              Link="Common\Interop\Windows\Kernel32\Interop.EnumProcessModules.cs" />
+    <Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.FormatMessage.cs"
+             Link="Common\Interop\Windows\Kernel32\Interop.FormatMessage.cs" />
     <Compile Include="$(CommonPath)Interop\Windows\NtDll\Interop.NtQueryInformationProcess.cs"
              Link="Common\Interop\Windows\NtDll\Interop.NtQueryInformationProcess.cs" />
     <Compile Include="$(CommonPath)Interop\Windows\NtDll\Interop.NtQuerySystemInformation.cs"

diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
@@ -432,8 +432,8 @@ private bool StartCore(ProcessStartInfo startInfo)
                 {
                     argv = ParseArgv(startInfo);
 
-                    isExecuting = ForkAndExecProcess(filename, argv, envp, cwd,
-                        startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError,
+                    isExecuting = ForkAndExecProcess(
+                        startInfo, filename, argv, envp, cwd,
                         setCredentials, userId, groupId, groups,
                         out stdinFd, out stdoutFd, out stderrFd, usesTerminal,
                         throwOnNoExec: false); // return false instead of throwing on ENOEXEC
@@ -445,8 +445,8 @@ private bool StartCore(ProcessStartInfo startInfo)
                     filename = GetPathToOpenFile();
                     argv = ParseArgv(startInfo, filename, ignoreArguments: true);
 
-                    ForkAndExecProcess(filename, argv, envp, cwd,
-                        startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError,
+                    ForkAndExecProcess(
+                        startInfo, filename, argv, envp, cwd,
                         setCredentials, userId, groupId, groups,
                         out stdinFd, out stdoutFd, out stderrFd, usesTerminal);
                 }
@@ -460,8 +460,8 @@ private bool StartCore(ProcessStartInfo startInfo)
                     throw new Win32Exception(SR.DirectoryNotValidAsInput);
                 }
 
-                ForkAndExecProcess(filename, argv, envp, cwd,
-                    startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError,
+                ForkAndExecProcess(
+                    startInfo, filename, argv, envp, cwd,
                     setCredentials, userId, groupId, groups,
                     out stdinFd, out stdoutFd, out stderrFd, usesTerminal);
             }
@@ -494,15 +494,16 @@ private bool StartCore(ProcessStartInfo startInfo)
         }
 
         private bool ForkAndExecProcess(
-            string? filename, string[] argv, string[] envp, string? cwd,
-            bool redirectStdin, bool redirectStdout, bool redirectStderr,
-            bool setCredentials, uint userId, uint groupId, uint[]? groups,
+            ProcessStartInfo startInfo, string? resolvedFilename, string[] argv,
+            string[] envp, string? cwd, bool setCredentials, uint userId,
+            uint groupId, uint[]? groups,
             out int stdinFd, out int stdoutFd, out int stderrFd,
             bool usesTerminal, bool throwOnNoExec = true)
         {
-            if (string.IsNullOrEmpty(filename))
+            if (string.IsNullOrEmpty(resolvedFilename))
             {
-                throw new Win32Exception(Interop.Error.ENOENT.Info().RawErrno);
+                Interop.ErrorInfo errno = Interop.Error.ENOENT.Info();
+                throw CreateExceptionForErrorStartingProcess(errno.GetErrorMessage(), errno.RawErrno, startInfo.FileName, cwd);
             }
 
             // Lock to avoid races with OnSigChild
@@ -523,11 +524,10 @@ private bool ForkAndExecProcess(
                 // 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,
+                    resolvedFilename, argv, envp, cwd,
+                    startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError,
                     setCredentials, userId, groupId, groups,
-                    out childPid,
-                    out stdinFd, out stdoutFd, out stderrFd);
+                    out childPid, out stdinFd, out stdoutFd, out stderrFd);
 
                 if (errno == 0)
                 {
@@ -550,7 +550,7 @@ private bool ForkAndExecProcess(
                         return false;
                     }
 
-                    throw new Win32Exception(errno);
+                    throw CreateExceptionForErrorStartingProcess(new Interop.ErrorInfo(errno).GetErrorMessage(), errno, resolvedFilename, cwd);
                 }
             }
             finally

diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Win32.cs
@@ -3,6 +3,7 @@
 
 using System.Collections.Generic;
 using System.ComponentModel;
+using System.IO;
 using System.Runtime.InteropServices;
 using System.Text;
 using System.Threading;
@@ -78,22 +79,23 @@ private unsafe bool StartWithShellExecuteEx(ProcessStartInfo startInfo)
                 ShellExecuteHelper executeHelper = new ShellExecuteHelper(&shellExecuteInfo);
                 if (!executeHelper.ShellExecuteOnSTAThread())
                 {
-                    int error = executeHelper.ErrorCode;
-                    if (error == 0)
+                    int errorCode = executeHelper.ErrorCode;
+                    if (errorCode == 0)
                     {
-                        error = GetShellError(shellExecuteInfo.hInstApp);
+                        errorCode = GetShellError(shellExecuteInfo.hInstApp);
                     }
 
-                    switch (error)
+                    switch (errorCode)
                     {
-                        case Interop.Errors.ERROR_BAD_EXE_FORMAT:
-                        case Interop.Errors.ERROR_EXE_MACHINE_TYPE_MISMATCH:
-                            throw new Win32Exception(error, SR.InvalidApplication);
                         case Interop.Errors.ERROR_CALL_NOT_IMPLEMENTED:
                             // This happens on Windows Nano
                             throw new PlatformNotSupportedException(SR.UseShellExecuteNotSupported);
                         default:
-                            throw new Win32Exception(error);
+                            string nativeErrorMessage = errorCode == Interop.Errors.ERROR_BAD_EXE_FORMAT || errorCode == Interop.Errors.ERROR_EXE_MACHINE_TYPE_MISMATCH
+                                ? SR.InvalidApplication
+                                : GetErrorMessage(errorCode);
+
+                            throw CreateExceptionForErrorStartingProcess(nativeErrorMessage, errorCode, startInfo.FileName, startInfo.WorkingDirectory);
                     }
                 }
 

diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs
@@ -619,11 +619,11 @@ ref processInfo      // pointer to PROCESS_INFORMATION
 
                     if (!retVal)
                     {
-                        if (errorCode == Interop.Errors.ERROR_BAD_EXE_FORMAT || errorCode == Interop.Errors.ERROR_EXE_MACHINE_TYPE_MISMATCH)
-                        {
-                            throw new Win32Exception(errorCode, SR.InvalidApplication);
-                        }
-                        throw new Win32Exception(errorCode);
+                        string nativeErrorMessage = errorCode == Interop.Errors.ERROR_BAD_EXE_FORMAT || errorCode == Interop.Errors.ERROR_EXE_MACHINE_TYPE_MISMATCH
+                            ? SR.InvalidApplication
+                            : GetErrorMessage(errorCode);
+
+                        throw CreateExceptionForErrorStartingProcess(nativeErrorMessage, errorCode, startInfo.FileName, workingDirectory);
                     }
                 }
                 finally
@@ -884,5 +884,7 @@ private static string GetEnvironmentVariablesBlock(IDictionary<string, string> s
 
             return result.ToString();
         }
+
+        private static string GetErrorMessage(int error) => Interop.Kernel32.GetMessage(error);
     }
 }
diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs
@@ -1686,6 +1686,13 @@ private void CheckDisposed()
             }
         }
 
+        private static Win32Exception CreateExceptionForErrorStartingProcess(string errorMessage, int errorCode, string fileName, string? workingDirectory)
+        {
+            string directoryForException = string.IsNullOrEmpty(workingDirectory) ? Directory.GetCurrentDirectory() : workingDirectory;
+            string msg = SR.Format(SR.ErrorStartingProcess, fileName, directoryForException, errorMessage);
+            return new Win32Exception(errorCode, msg);
+        }
+
         /// <summary>
         /// This enum defines the operation mode for redirected process stream.
         /// We don't support switching between synchronous mode and asynchronous mode.

diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs
@@ -220,6 +220,8 @@ public void TestStartWithBadWorkingDirectory()
 
                 Win32Exception e = Assert.Throws<Win32Exception>(() => Process.Start(psi));
                 Assert.NotEqual(0, e.NativeErrorCode);
+                Assert.Contains(program, e.Message);
+                Assert.Contains(workingDirectory, e.Message);
             }
             else
             {
@@ -232,7 +234,9 @@ public void TestStartWithBadWorkingDirectory()
         public void ProcessStart_UseShellExecute_OnWindows_OpenMissingFile_Throws()
         {
             string fileToOpen = Path.Combine(Environment.CurrentDirectory, "_no_such_file.TXT");
-            Assert.Throws<Win32Exception>(() => Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName = fileToOpen }));
+            AssertExtensions.ThrowsContains<Win32Exception>(
+                () => Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName = fileToOpen }),
+                fileToOpen);
         }
 
         [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.HasWindowsShell))]
@@ -1398,6 +1402,7 @@ public void TestStartWithMissingFile(bool fullPath)
 
             Win32Exception e = Assert.Throws<Win32Exception>(() => Process.Start(path));
             Assert.NotEqual(0, e.NativeErrorCode);
+            Assert.Contains(path, e.Message);
         }
 
         [Fact]