GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix for PowerShell SSH remoting with recent Win32-OpenSSH change. by PaulHigin · Pull Request #2538 · PowerShell/PowerShell

Conversation

Paul Higinbotham (PaulHigin) commented Oct 26, 2016

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dzmitry-lahoda reacted with thumbs up emoji

All reactions

👍 1 reaction


          Fix for Win32-OpenSSH break when hosting PowerShell in server mode as…

b8c993b

… an SSH subsystem.

Microsoft Contribution License Agreements (msftclas) commented Oct 26, 2016

Copy link

Copy Markdown

Hi Paul Higinbotham (@PaulHigin), I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Paul Higinbotham). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label

Oct 26, 2016

Paul Higinbotham (PaulHigin) added WG-Remoting PSRP issues with any transport layer and removed cla-not-required labels

Oct 26, 2016

Paul Higinbotham (PaulHigin) added this to the 6.0.0-alpha.12 milestone

Oct 26, 2016

Jason Shirk (lzybkr) reviewed

Oct 26, 2016

View reviewed changes

Comment thread

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs Outdated

		_parent = p;
		_ui = (ConsoleHostUserInterface)p.UI;
		_ver = ver;
		_ui = (p != null) ? (ConsoleHostUserInterface)p.UI : null;

Jason Shirk (lzybkr) Oct 26, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For better future-proofing, maybe we should provide a dummy ConsoleHostUserInterface implementation that does nothing. This would also avoid many of the other changes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Refactored to use null PSHostUserInterface implementation

1c86eae

Paul Higinbotham (PaulHigin) commented Oct 27, 2016

Copy link

Copy Markdown

Contributor Author

Updated PR based on comments.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mike Richmond (mirichmo) self-assigned this

Oct 27, 2016

Mike Richmond (mirichmo) suggested changes

Oct 27, 2016

View reviewed changes

Comment thread

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

		_ui.WriteToConsole("Waiting - type enter to continue:", false);
		_ui.ReadLine();

		((ConsoleHostUserInterface)_hostUI).WriteToConsole("Waiting - type enter to continue:", false);

Mike Richmond (mirichmo) Oct 27, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about when _hostUI is a NullHostUserInterface?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Paul Higinbotham (PaulHigin) Oct 27, 2016

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs Outdated

		try
		{
		args = new string[0];
		s_theConsoleHost = ConsoleHost.CreateSingletonInstance(configuration);

Mike Richmond (mirichmo) Oct 27, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be within the try-finally on line 222 since the finally conditionally disposes it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Paul Higinbotham (PaulHigin) Oct 27, 2016

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs

		{
		TelemetryAPI.ReportExitTelemetry(s_theConsoleHost);
		s_theConsoleHost.Dispose();
		if (s_theConsoleHost != null)

Mike Richmond (mirichmo) Oct 27, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check seems a bit unnecessary. ConsoleHost.CreateSingletonInstance asserts (in debug mode) that it is non-null

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Paul Higinbotham (PaulHigin) Oct 27, 2016

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) Oct 28, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Paul Higinbotham (PaulHigin) Oct 28, 2016

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) Oct 28, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Console.Out.Write shouldn't cause any problems.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Paul Higinbotham (PaulHigin) Oct 28, 2016

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Implemented code review comments

19e07d8

Mike Richmond (mirichmo) approved these changes

Nov 1, 2016

View reviewed changes

Mike Richmond (mirichmo) merged commit 24f3b75 into PowerShell:master

Nov 1, 2016

Paul Higinbotham (PaulHigin) deleted the W32OpenSSHFix branch

November 17, 2016 19:14

Steve Lee (SteveL-MSFT) mentioned this pull request

Nov 21, 2016

PowerShell Remoting over SSH fails Windows client #2466

Closed

Ilya (iSazonov) mentioned this pull request

Oct 1, 2018

Remove clone args #7910

Merged

6 of 11 tasks

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Fix for PowerShell SSH remoting with recent Win32-OpenSSH change. (Po…

9300cca

…werShell#2538)

* Fix for Win32-OpenSSH break when hosting PowerShell in server mode as an SSH subsystem.

* Refactored to use null PSHostUserInterface implementation

* Implemented code review comments

Reviewers

2 more reviewers

Jason Shirk (lzybkr)

lzybkr left review comments

Mike Richmond (mirichmo)

mirichmo approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Mike Richmond (mirichmo)

Labels

WG-Remoting

PSRP issues with any transport layer

Projects

None yet

Milestone

6.0.0-alpha.12

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.