Close pipe client handles after creating the child ssh process (#26491)

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

@@ -2211,11 +2211,11 @@ namespace System.Management.Automation.Runspaces
                     CommandTypes.Application,
                     SearchResolutionOptions.None,
                     CommandOrigin.Internal,
-                    context) as ApplicationInfo;
+                    context);
 
-                if (cmdInfo != null)
+                if (cmdInfo is ApplicationInfo appInfo)
                 {
-                    filePath = cmdInfo.Path;
+                    filePath = appInfo.Path;
                 }
             }
             else
@@ -2273,13 +2273,13 @@ namespace System.Management.Automation.Runspaces
             //     Subsystem powershell /usr/local/bin/pwsh -SSHServerMode -NoLogo -NoProfile
 
             // codeql[cs/microsoft/command-line-injection-shell-execution] - This is expected Poweshell behavior where user inputted paths are supported for the context of this method. The user assumes trust for the file path specified, so any file executed in the runspace would be in the user's local system/process or a system they have access to in which case restricted remoting security guidelines should be used.
-            System.Diagnostics.ProcessStartInfo startInfo = new System.Diagnostics.ProcessStartInfo(filePath);
+            ProcessStartInfo startInfo = new(filePath);
 
             // pass "-i identity_file" command line argument to ssh if KeyFilePath is set
             // if KeyFilePath is not set, then ssh will use IdentityFile / IdentityAgent from ssh_config if defined else none by default
             if (!string.IsNullOrEmpty(this.KeyFilePath))
             {
-                if (!System.IO.File.Exists(this.KeyFilePath))
+                if (!File.Exists(this.KeyFilePath))
                 {
                     throw new FileNotFoundException(
                         StringUtil.Format(RemotingErrorIdStrings.KeyFileNotFound, this.KeyFilePath));
@@ -2326,7 +2326,7 @@ namespace System.Management.Automation.Runspaces
             // note that ssh expects IPv6 addresses to not be enclosed in square brackets so trim them if present
             startInfo.ArgumentList.Add(string.Create(CultureInfo.InvariantCulture, $@"-s {this.ComputerName.TrimStart('[').TrimEnd(']')} {this.Subsystem}"));
 
-            startInfo.WorkingDirectory = System.IO.Path.GetDirectoryName(filePath);
+            startInfo.WorkingDirectory = Path.GetDirectoryName(filePath);
             startInfo.CreateNoWindow = true;
             startInfo.UseShellExecute = false;
 
@@ -2580,7 +2580,7 @@ namespace System.Management.Automation.Runspaces
             // Allocate the unmanaged array to hold each string pointer.
             // It needs to have an extra element to null terminate the array.
             arrPtr = (byte**)Marshal.AllocHGlobal(sizeof(IntPtr) * arrLength);
-            System.Diagnostics.Debug.Assert(arrPtr != null, "Invalid array ptr");
+            Debug.Assert(arrPtr != null, "Invalid array ptr");
 
             // Zero the memory so that if any of the individual string allocations fails,
             // we can loop through the array to free any that succeeded.
@@ -2597,7 +2597,7 @@ namespace System.Management.Automation.Runspaces
                 byte[] byteArr = System.Text.Encoding.UTF8.GetBytes(arr[i]);
 
                 arrPtr[i] = (byte*)Marshal.AllocHGlobal(byteArr.Length + 1); // +1 for null termination
-                System.Diagnostics.Debug.Assert(arrPtr[i] != null, "Invalid array ptr");
+                Debug.Assert(arrPtr[i] != null, "Invalid array ptr");
 
                 Marshal.Copy(byteArr, 0, (IntPtr)arrPtr[i], byteArr.Length); // copy over the data from the managed byte array
                 arrPtr[i][byteArr.Length] = (byte)'\0'; // null terminate
@@ -2641,13 +2641,13 @@ namespace System.Management.Automation.Runspaces
         /// P-Invoking native APIs.
         /// </summary>
         private static int StartSSHProcessImpl(
-            System.Diagnostics.ProcessStartInfo startInfo,
+            ProcessStartInfo startInfo,
             out StreamWriter stdInWriterVar,
             out StreamReader stdOutReaderVar,
             out StreamReader stdErrReaderVar)
         {
             Exception ex = null;
-            System.Diagnostics.Process sshProcess = null;
+            Process sshProcess = null;
             //
             // These std pipe handles are bound to managed Reader/Writer objects and returned to the transport
             // manager object, which uses them for PSRP communication.  The lifetime of these handles are then
@@ -2668,7 +2668,7 @@ namespace System.Management.Automation.Runspaces
             catch (InvalidOperationException e) { ex = e; }
             catch (ArgumentException e) { ex = e; }
             catch (FileNotFoundException e) { ex = e; }
-            catch (System.ComponentModel.Win32Exception e) { ex = e; }
+            catch (Win32Exception e) { ex = e; }
 
             if ((ex != null) ||
                 (sshProcess == null) ||
@@ -2693,9 +2693,9 @@ namespace System.Management.Automation.Runspaces
             {
                 if (stdInWriterVar != null) { stdInWriterVar.Dispose(); } else { stdInPipeServer.Dispose(); }
 
-                if (stdOutReaderVar != null) { stdInWriterVar.Dispose(); } else { stdOutPipeServer.Dispose(); }
+                if (stdOutReaderVar != null) { stdOutReaderVar.Dispose(); } else { stdOutPipeServer.Dispose(); }
 
-                if (stdErrReaderVar != null) { stdInWriterVar.Dispose(); } else { stdErrPipeServer.Dispose(); }
+                if (stdErrReaderVar != null) { stdErrReaderVar.Dispose(); } else { stdErrPipeServer.Dispose(); }
 
                 throw;
             }
@@ -2705,7 +2705,7 @@ namespace System.Management.Automation.Runspaces
 
         private static void KillSSHProcessImpl(int pid)
         {
-            using (var sshProcess = System.Diagnostics.Process.GetProcessById(pid))
+            using (var sshProcess = Process.GetProcessById(pid))
             {
                 if ((sshProcess != null) && (sshProcess.Handle != IntPtr.Zero) && !sshProcess.HasExited)
                 {
@@ -2736,7 +2736,7 @@ namespace System.Management.Automation.Runspaces
             SafeFileHandle stdInPipeClient = null;
             SafeFileHandle stdOutPipeClient = null;
             SafeFileHandle stdErrPipeClient = null;
-            string randomName = System.IO.Path.GetFileNameWithoutExtension(System.IO.Path.GetRandomFileName());
+            string randomName = Path.GetFileNameWithoutExtension(Path.GetRandomFileName());
 
             try
             {
@@ -2829,16 +2829,14 @@ namespace System.Management.Automation.Runspaces
             catch (Exception)
             {
                 stdInPipeServer?.Dispose();
-                stdInPipeClient?.Dispose();
                 stdOutPipeServer?.Dispose();
-                stdOutPipeClient?.Dispose();
                 stdErrPipeServer?.Dispose();
-                stdErrPipeClient?.Dispose();
 
                 throw;
             }
             finally
             {
+                lpStartupInfo.Dispose();
                 lpProcessInformation.Dispose();
             }
         }

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs

@@ -1733,7 +1733,7 @@ namespace System.Management.Automation.Remoting.Client
                     bool sshTerminated = false;
                     try
                     {
-                        using (var sshProcess = System.Diagnostics.Process.GetProcessById(_sshProcessId))
+                        using (var sshProcess = Process.GetProcessById(_sshProcessId))
                         {
                             sshTerminated = sshProcess == null || sshProcess.Handle == IntPtr.Zero || sshProcess.HasExited;
                         }
@@ -1847,7 +1847,7 @@ namespace System.Management.Automation.Remoting.Client
                         // Messages in error stream from ssh are unreliable, and may just be warnings or
                         // banner text.
                         // So just report the messages but don't act on them.
-                        System.Console.WriteLine(error);
+                        Console.WriteLine(error);
                     }
                     catch (IOException)
                     { }
@@ -1907,10 +1907,10 @@ namespace System.Management.Automation.Remoting.Client
                         break;
                     }
 
-                    if (data.StartsWith(System.Management.Automation.Remoting.Server.FormattedErrorTextWriter.ErrorPrefix, StringComparison.OrdinalIgnoreCase))
+                    if (data.StartsWith(OutOfProcessTextWriter.ErrorPrefix, StringComparison.OrdinalIgnoreCase))
                     {
                         // Error message from the server.
-                        string errorData = data.Substring(System.Management.Automation.Remoting.Server.FormattedErrorTextWriter.ErrorPrefix.Length);
+                        string errorData = data.Substring(OutOfProcessTextWriter.ErrorPrefix.Length);
                         HandleErrorDataReceived(errorData);
                     }
                     else

test/powershell/engine/Remoting/SSHRemotingCmdlets.Tests.ps1

@@ -70,3 +70,24 @@ Describe "SSHConnection parameter hashtable type conversions" -Tags 'Feature', '
         $err.FullyQualifiedErrorId | Should -Match 'PSSessionOpenFailed'
     }
 }
+
+Describe "No hangs when host doesn't exist" -Tags "CI" {
+    $testCases = @(
+        @{
+            Name = 'Verifies no hang for New-PSSession with non-existing host name'
+            ScriptBlock = { New-PSSession -HostName "test-notexist" -UserName "test" -ErrorAction Stop }
+            FullyQualifiedErrorId = 'PSSessionOpenFailed'
+        },
+        @{
+            Name = 'Verifies no hang for Invoke-Command with non-existing host name'
+            ScriptBlock = { Invoke-Command -HostName "test-notexist" -UserName "test" -ScriptBlock { 1 } -ErrorAction Stop }
+            FullyQualifiedErrorId = 'PSSessionStateBroken'
+        }
+    )
+
+    It "<Name>" -TestCases $testCases {
+        param ($ScriptBlock, $FullyQualifiedErrorId)
+
+        $ScriptBlock | Should -Throw -ErrorId $FullyQualifiedErrorId -ExceptionType 'System.Management.Automation.Remoting.PSRemotingTransportException'
+    }
+}