Follow-up to #126192: address review comments (doc nit, ShellExecuteHelper refactor, field keyword, test, trimming, waitStateHolder fix)

src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Unix.cs

@@ -59,12 +59,15 @@ protected override bool ReleaseHandle()
         // On Unix, we don't use process descriptors yet, so we can't get PID.
         private static int GetProcessIdCore() => throw new PlatformNotSupportedException();
 
+        private delegate SafeProcessHandle StartWithShellExecuteDelegate(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle, SafeFileHandle? stdoutHandle, SafeFileHandle? stderrHandle, out ProcessWaitState.Holder? waitStateHolder);
+        private static StartWithShellExecuteDelegate? s_startWithShellExecute;
+
         private static SafeProcessHandle StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle, SafeFileHandle? stdoutHandle, SafeFileHandle? stderrHandle)
         {
             SafeProcessHandle startedProcess = StartCore(startInfo, stdinHandle, stdoutHandle, stderrHandle, out ProcessWaitState.Holder? waitStateHolder);
 
             // For standalone SafeProcessHandle.Start, we dispose the wait state holder immediately.
-            // The DangerousAddRef on the SafeWaitHandle (Unix) keeps the OS handle alive.
+            // The DangerousAddRef on the SafeWaitHandle (Unix) keeps the handle alive.
             waitStateHolder?.Dispose();
 
             return startedProcess;
@@ -81,6 +84,11 @@ internal static SafeProcessHandle StartCore(ProcessStartInfo startInfo, SafeFile
 
             ProcessUtils.EnsureInitialized();
 
+            if (startInfo.UseShellExecute)
+            {
+                return s_startWithShellExecute!(startInfo, stdinHandle, stdoutHandle, stderrHandle, out waitStateHolder);
+            }
+
             string? filename;
             string[] argv;
 
@@ -105,63 +113,82 @@ internal static SafeProcessHandle StartCore(ProcessStartInfo startInfo, SafeFile
                 || (stdoutHandle is not null && Interop.Sys.IsATty(stdoutHandle))
                 || (stderrHandle is not null && Interop.Sys.IsATty(stderrHandle));
 
-            if (startInfo.UseShellExecute)
+            filename = ProcessUtils.ResolvePath(startInfo.FileName);
+            argv = ProcessUtils.ParseArgv(startInfo);
+            if (Directory.Exists(filename))
             {
-                string verb = startInfo.Verb;
-                if (verb != string.Empty &&
-                    !string.Equals(verb, "open", StringComparison.OrdinalIgnoreCase))
-                {
-                    throw new Win32Exception(Interop.Errors.ERROR_NO_ASSOCIATION);
-                }
+                throw new Win32Exception(SR.DirectoryNotValidAsInput);
+            }
 
-                // 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).
-                filename = ProcessUtils.ResolveExecutableForShellExecute(startInfo.FileName, cwd);
-                if (filename != null)
-                {
-                    argv = ProcessUtils.ParseArgv(startInfo);
-
-                    SafeProcessHandle processHandle = ForkAndExecProcess(
-                        startInfo, filename, argv, env, cwd,
-                        setCredentials, userId, groupId, groups,
-                        stdinHandle, stdoutHandle, stderrHandle, usesTerminal,
-                        out waitStateHolder,
-                        throwOnNoExec: false); // return invalid handle instead of throwing on ENOEXEC
-
-                    if (!processHandle.IsInvalid)
-                    {
-                        return processHandle;
-                    }
-                }
+            return ForkAndExecProcess(
+                startInfo, filename, argv, env, cwd,
+                setCredentials, userId, groupId, groups,
+                stdinHandle, stdoutHandle, stderrHandle, usesTerminal,
+                out waitStateHolder);
+        }
 
-                // use default program to open file/url
-                filename = Process.GetPathToOpenFile();
-                argv = ProcessUtils.ParseArgv(startInfo, filename, ignoreArguments: true);
+        private static SafeProcessHandle StartWithShellExecute(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle, SafeFileHandle? stdoutHandle, SafeFileHandle? stderrHandle, out ProcessWaitState.Holder? waitStateHolder)
+        {
+            IDictionary<string, string?> env = startInfo.Environment;
+            string? cwd = !string.IsNullOrWhiteSpace(startInfo.WorkingDirectory) ? startInfo.WorkingDirectory : null;
 
-                return ForkAndExecProcess(
-                    startInfo, filename, argv, env, cwd,
-                    setCredentials, userId, groupId, groups,
-                    stdinHandle, stdoutHandle, stderrHandle, usesTerminal,
-                    out waitStateHolder);
+            bool setCredentials = !string.IsNullOrEmpty(startInfo.UserName);
+            uint userId = 0;
+            uint groupId = 0;
+            uint[]? groups = null;
+            if (setCredentials)
+            {
+                (userId, groupId, groups) = ProcessUtils.GetUserAndGroupIds(startInfo);
             }
-            else
+
+            bool usesTerminal = (stdinHandle is not null && Interop.Sys.IsATty(stdinHandle))
+                || (stdoutHandle is not null && Interop.Sys.IsATty(stdoutHandle))
+                || (stderrHandle is not null && Interop.Sys.IsATty(stderrHandle));
+
+            string verb = startInfo.Verb;
+            if (verb != string.Empty &&
+                !string.Equals(verb, "open", StringComparison.OrdinalIgnoreCase))
             {
-                filename = ProcessUtils.ResolvePath(startInfo.FileName);
-                argv = ProcessUtils.ParseArgv(startInfo);
-                if (Directory.Exists(filename))
-                {
-                    throw new Win32Exception(SR.DirectoryNotValidAsInput);
-                }
+                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).
+            string? filename = ProcessUtils.ResolveExecutableForShellExecute(startInfo.FileName, cwd);
+            if (filename != null)
+            {
+                string[] argv = ProcessUtils.ParseArgv(startInfo);
 
-                return ForkAndExecProcess(
+                SafeProcessHandle processHandle = ForkAndExecProcess(
                     startInfo, filename, argv, env, cwd,
                     setCredentials, userId, groupId, groups,
                     stdinHandle, stdoutHandle, stderrHandle, usesTerminal,
-                    out waitStateHolder);
+                    out waitStateHolder,
+                    throwOnNoExec: false); // return invalid handle instead of throwing on ENOEXEC
+
+                if (!processHandle.IsInvalid)
+                {
+                    return processHandle;
+                }
+
+                // ENOEXEC: the process was not started on this path; dispose the holder and try the fallback.
+                waitStateHolder?.Dispose();
             }
+
+            // use default program to open file/url
+            filename = Process.GetPathToOpenFile();
+            string[] openFileArgv = ProcessUtils.ParseArgv(startInfo, filename, ignoreArguments: true);
+
+            SafeProcessHandle result = ForkAndExecProcess(
+                startInfo, filename, openFileArgv, env, cwd,
+                setCredentials, userId, groupId, groups,
+                stdinHandle, stdoutHandle, stderrHandle, usesTerminal,
+                out waitStateHolder);
+
+            return result;
         }
 
         private static SafeProcessHandle ForkAndExecProcess(

src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs

@@ -6,6 +6,7 @@
 using System.Runtime.InteropServices;
 using System.Security;
 using System.Text;
+using System.Threading;
 
 namespace Microsoft.Win32.SafeHandles
 {
@@ -16,86 +17,13 @@ protected override bool ReleaseHandle()
             return Interop.Kernel32.CloseHandle(handle);
         }
 
-        internal static SafeProcessHandle StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle, SafeFileHandle? stdoutHandle, SafeFileHandle? stderrHandle)
-        {
-            return startInfo.UseShellExecute
-                ? StartWithShellExecuteEx(startInfo)
-                : StartWithCreateProcess(startInfo, stdinHandle, stdoutHandle, stderrHandle);
-        }
+        private static Func<ProcessStartInfo, SafeProcessHandle>? s_startWithShellExecute;
 
-        private static unsafe SafeProcessHandle StartWithShellExecuteEx(ProcessStartInfo startInfo)
+        internal static unsafe SafeProcessHandle StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle, SafeFileHandle? stdoutHandle, SafeFileHandle? stderrHandle)
         {
-            if (!string.IsNullOrEmpty(startInfo.UserName) || startInfo.Password != null)
-                throw new InvalidOperationException(SR.CantStartAsUser);
-
-            if (startInfo.StandardInputEncoding != null)
-                throw new InvalidOperationException(SR.StandardInputEncodingNotAllowed);
-
-            if (startInfo.StandardErrorEncoding != null)
-                throw new InvalidOperationException(SR.StandardErrorEncodingNotAllowed);
-
-            if (startInfo.StandardOutputEncoding != null)
-                throw new InvalidOperationException(SR.StandardOutputEncodingNotAllowed);
-
-            if (startInfo._environmentVariables != null)
-                throw new InvalidOperationException(SR.CantUseEnvVars);
-
-            string arguments = startInfo.BuildArguments();
-
-            fixed (char* fileName = startInfo.FileName.Length > 0 ? startInfo.FileName : null)
-            fixed (char* verb = startInfo.Verb.Length > 0 ? startInfo.Verb : null)
-            fixed (char* parameters = arguments.Length > 0 ? arguments : null)
-            fixed (char* directory = startInfo.WorkingDirectory.Length > 0 ? startInfo.WorkingDirectory : null)
-            {
-                Interop.Shell32.SHELLEXECUTEINFO shellExecuteInfo = new Interop.Shell32.SHELLEXECUTEINFO()
-                {
-                    cbSize = (uint)sizeof(Interop.Shell32.SHELLEXECUTEINFO),
-                    lpFile = fileName,
-                    lpVerb = verb,
-                    lpParameters = parameters,
-                    lpDirectory = directory,
-                    fMask = Interop.Shell32.SEE_MASK_NOCLOSEPROCESS | Interop.Shell32.SEE_MASK_FLAG_DDEWAIT
-                };
-
-                if (startInfo.ErrorDialog)
-                    shellExecuteInfo.hwnd = startInfo.ErrorDialogParentHandle;
-                else
-                    shellExecuteInfo.fMask |= Interop.Shell32.SEE_MASK_FLAG_NO_UI;
+            if (startInfo.UseShellExecute)
+                return s_startWithShellExecute!(startInfo);
 
-                shellExecuteInfo.nShow = ProcessUtils.GetShowWindowFromWindowStyle(startInfo.WindowStyle);
-                ShellExecuteHelper executeHelper = new ShellExecuteHelper(&shellExecuteInfo);
-                if (!executeHelper.ShellExecuteOnSTAThread())
-                {
-                    int errorCode = executeHelper.ErrorCode;
-                    if (errorCode == 0)
-                    {
-                        errorCode = ShellExecuteHelper.GetShellError(shellExecuteInfo.hInstApp);
-                    }
-
-                    switch (errorCode)
-                    {
-                        case Interop.Errors.ERROR_CALL_NOT_IMPLEMENTED:
-                            // This happens on Windows Nano
-                            throw new PlatformNotSupportedException(SR.UseShellExecuteNotSupported);
-                        default:
-                            string nativeErrorMessage = errorCode == Interop.Errors.ERROR_BAD_EXE_FORMAT || errorCode == Interop.Errors.ERROR_EXE_MACHINE_TYPE_MISMATCH
-                                ? SR.InvalidApplication
-                                : Interop.Kernel32.GetMessage(errorCode);
-
-                            throw ProcessUtils.CreateExceptionForErrorStartingProcess(nativeErrorMessage, errorCode, startInfo.FileName, startInfo.WorkingDirectory);
-                    }
-                }
-
-                // From https://learn.microsoft.com/windows/win32/api/shellapi/ns-shellapi-shellexecuteinfow:
-                // "In some cases, such as when execution is satisfied through a DDE conversation, no handle will be returned."
-                // Process.Start will return false if the handle is invalid.
-                return new SafeProcessHandle(shellExecuteInfo.hProcess);
-            }
-        }
-
-        /// <summary>Starts the process using the supplied start info.</summary>
-        private static unsafe SafeProcessHandle StartWithCreateProcess(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle, SafeFileHandle? stdoutHandle, SafeFileHandle? stderrHandle)
-        {
             // See knowledge base article Q190351 for an explanation of the following code.  Noteworthy tricky points:
             //    * The handles are duplicated as inheritable before they are passed to CreateProcess so
             //      that the child process can use them
@@ -281,6 +209,126 @@ ref processInfo      // pointer to PROCESS_INFORMATION
             return procSH;
         }
 
+        private static unsafe SafeProcessHandle StartWithShellExecute(ProcessStartInfo startInfo)
+        {
+            if (!string.IsNullOrEmpty(startInfo.UserName) || startInfo.Password != null)
+                throw new InvalidOperationException(SR.CantStartAsUser);
+
+            if (startInfo.StandardInputEncoding != null)
+                throw new InvalidOperationException(SR.StandardInputEncodingNotAllowed);
+
+            if (startInfo.StandardErrorEncoding != null)
+                throw new InvalidOperationException(SR.StandardErrorEncodingNotAllowed);
+
+            if (startInfo.StandardOutputEncoding != null)
+                throw new InvalidOperationException(SR.StandardOutputEncodingNotAllowed);
+
+            if (startInfo._environmentVariables != null)
+                throw new InvalidOperationException(SR.CantUseEnvVars);
+
+            string arguments = startInfo.BuildArguments();
+
+            fixed (char* fileName = startInfo.FileName.Length > 0 ? startInfo.FileName : null)
+            fixed (char* verb = startInfo.Verb.Length > 0 ? startInfo.Verb : null)
+            fixed (char* parameters = arguments.Length > 0 ? arguments : null)
+            fixed (char* directory = startInfo.WorkingDirectory.Length > 0 ? startInfo.WorkingDirectory : null)
+            {
+                Interop.Shell32.SHELLEXECUTEINFO shellExecuteInfo = new Interop.Shell32.SHELLEXECUTEINFO()
+                {
+                    cbSize = (uint)sizeof(Interop.Shell32.SHELLEXECUTEINFO),
+                    lpFile = fileName,
+                    lpVerb = verb,
+                    lpParameters = parameters,
+                    lpDirectory = directory,
+                    fMask = Interop.Shell32.SEE_MASK_NOCLOSEPROCESS | Interop.Shell32.SEE_MASK_FLAG_DDEWAIT
+                };
+
+                if (startInfo.ErrorDialog)
+                    shellExecuteInfo.hwnd = startInfo.ErrorDialogParentHandle;
+                else
+                    shellExecuteInfo.fMask |= Interop.Shell32.SEE_MASK_FLAG_NO_UI;
+
+                shellExecuteInfo.nShow = ProcessUtils.GetShowWindowFromWindowStyle(startInfo.WindowStyle);
+
+                bool succeeded = false;
+                int lastError = 0;
+                nuint executeInfoAddress = (nuint)(&shellExecuteInfo); // cast to nuint to allow delegate capture; safe because Join() keeps this stack frame alive for the thread's lifetime
+
+                void ShellExecuteFunction()
+                {
+                    try
+                    {
+                        if (!(succeeded = Interop.Shell32.ShellExecuteExW((Interop.Shell32.SHELLEXECUTEINFO*)executeInfoAddress)))
+                            lastError = Marshal.GetLastWin32Error();
+                    }
+                    catch (EntryPointNotFoundException)
+                    {
+                        lastError = Interop.Errors.ERROR_CALL_NOT_IMPLEMENTED;
+                    }
+                }
+
+                // ShellExecute() requires STA in order to work correctly.
+                if (Thread.CurrentThread.GetApartmentState() != ApartmentState.STA)
+                {
+                    Thread executionThread = new Thread(ShellExecuteFunction)
+                    {
+                        IsBackground = true,
+                        Name = ".NET Process STA"
+                    };
+                    executionThread.SetApartmentState(ApartmentState.STA);
+                    executionThread.Start();
+                    executionThread.Join();
+                }
+                else
+                {
+                    ShellExecuteFunction();
+                }
+
+                if (!succeeded)
+                {
+                    int errorCode = lastError;
+                    if (errorCode == 0)
+                    {
+                        errorCode = GetShellError(shellExecuteInfo.hInstApp);
+                    }
+
+                    switch (errorCode)
+                    {
+                        case Interop.Errors.ERROR_CALL_NOT_IMPLEMENTED:
+                            // This happens on Windows Nano
+                            throw new PlatformNotSupportedException(SR.UseShellExecuteNotSupported);
+                        default:
+                            string nativeErrorMessage = errorCode == Interop.Errors.ERROR_BAD_EXE_FORMAT || errorCode == Interop.Errors.ERROR_EXE_MACHINE_TYPE_MISMATCH
+                                ? SR.InvalidApplication
+                                : Interop.Kernel32.GetMessage(errorCode);
+
+                            throw ProcessUtils.CreateExceptionForErrorStartingProcess(nativeErrorMessage, errorCode, startInfo.FileName, startInfo.WorkingDirectory);
+                    }
+                }
+
+                // From https://learn.microsoft.com/windows/win32/api/shellapi/ns-shellapi-shellexecuteinfow:
+                // "In some cases, such as when execution is satisfied through a DDE conversation, no handle will be returned."
+                // Process.Start will return false if the handle is invalid.
+                return new SafeProcessHandle(shellExecuteInfo.hProcess);
+
+                static int GetShellError(IntPtr error) =>
+                    (long)error switch
+                    {
+                        Interop.Shell32.SE_ERR_FNF => Interop.Errors.ERROR_FILE_NOT_FOUND,
+                        Interop.Shell32.SE_ERR_PNF => Interop.Errors.ERROR_PATH_NOT_FOUND,
+                        Interop.Shell32.SE_ERR_ACCESSDENIED => Interop.Errors.ERROR_ACCESS_DENIED,
+                        Interop.Shell32.SE_ERR_OOM => Interop.Errors.ERROR_NOT_ENOUGH_MEMORY,
+                        Interop.Shell32.SE_ERR_DDEFAIL or
+                        Interop.Shell32.SE_ERR_DDEBUSY or
+                        Interop.Shell32.SE_ERR_DDETIMEOUT => Interop.Errors.ERROR_DDE_FAIL,
+                        Interop.Shell32.SE_ERR_SHARE => Interop.Errors.ERROR_SHARING_VIOLATION,
+                        Interop.Shell32.SE_ERR_NOASSOC => Interop.Errors.ERROR_NO_ASSOCIATION,
+                        Interop.Shell32.SE_ERR_DLLNOTFOUND => Interop.Errors.ERROR_DLL_NOT_FOUND,
+                        _ => (int)(long)error,
+                    };
+            }
+        }
+
         private int GetProcessIdCore() => Interop.Kernel32.GetProcessId(this);
     }
 }