Fix for PowerShell SSH remoting with recent Win32-OpenSSH change. #2538
Conversation
… an SSH subsystem.
|
Hi @PaulHigin, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
PaulHigin
added
WG-Remoting
| _parent = p; | ||
| _ui = (ConsoleHostUserInterface)p.UI; | ||
| _ver = ver; | ||
| _ui = (p != null) ? (ConsoleHostUserInterface)p.UI : null; |
There was a problem hiding this comment.
For better future-proofing, maybe we should provide a dummy ConsoleHostUserInterface implementation that does nothing. This would also avoid many of the other changes.
|
Updated PR based on comments. |
| _ui.WriteToConsole("Waiting - type enter to continue:", false); | ||
| _ui.ReadLine(); | ||
| ((ConsoleHostUserInterface)_hostUI).WriteToConsole("Waiting - type enter to continue:", false); |
There was a problem hiding this comment.
What about when _hostUI is a NullHostUserInterface?
There was a problem hiding this comment.
Then this will fail. Note that this is for debugging interactive console only and should never be called without ConsoleHostUserInterface.
In reply to: 85439307 [](ancestors = 85439307)
| try | ||
| { | ||
| args = new string[0]; | ||
| s_theConsoleHost = ConsoleHost.CreateSingletonInstance(configuration); |
There was a problem hiding this comment.
Shouldn't this be within the try-finally on line 222 since the finally conditionally disposes it?
There was a problem hiding this comment.
No, because otherwise CommandLineParameterParser will never be called and we can't find out if we are running server mode where s_theConsoleHost is not needed. Also I am saving the HostException so I can re-throw if it turns out we are not running in server mode.
In reply to: 85440668 [](ancestors = 85440668)
| { | ||
| TelemetryAPI.ReportExitTelemetry(s_theConsoleHost); | ||
| s_theConsoleHost.Dispose(); | ||
| if (s_theConsoleHost != null) |
There was a problem hiding this comment.
This check seems a bit unnecessary. ConsoleHost.CreateSingletonInstance asserts (in debug mode) that it is non-null
There was a problem hiding this comment.
This is the exact condition I am allowing. The s_theConsoleHost is null because the process is created without a console, but that is Ok if we are running in server mode. If we are not running in server mode then the HostException is re-thrown since interactive PowerShell requires s_theConsoleHost.
I liked my original changes over this second iteration because it was cleaner. The command line is parsed and if we run in server mode we don't even bother creating the s_theConsoleHost. But that version was rejected.
In reply to: 85441248 [](ancestors = 85441248)
There was a problem hiding this comment.
I didn't realize your changes were all in the command line parser.
Maybe a better fix is to just use the Console apis directly.
I guess in server mode you should never see an error, but you'd want the error reported if there was one, right?
There was a problem hiding this comment.
I presume System.Console APIs won't work if there is no process console. However, I can use the ConsoleHost class tracer to write errors. But I don't feel that reporting a command line parse error in this case is very important.
There was a problem hiding this comment.
Console.Out.Write shouldn't cause any problems.
There was a problem hiding this comment.
This always confuses me. Isn't this the console std out? If so then again I presume it is not available when no process console is available. In any case it seems that trace output would be better for debugging a rare case of command line errors in server mode.
4 participants
A recent change in Win32-OpenSSH caused PowerShell SSH remoting to break when targeting a Windows machine (Windows->Windows or Linux->Windows). The breaking change in Win32-OpenSSH is that when it creates a subsystem process it no longer creates a console for that process, since a console is not needed.
However, PowerShell currently requires a process console even when it is running in server mode when a console is not really needed.
The fix is to update PowerShell so that it does not require a process console when it is running in server mode. It includes removing unused fields in the CommandLineParameterParser class and only failing on start up with a console error when running in console mode.