Skip to content

Fix for SSH remoting when SSH client abruptly terminates - #4123

Merged
Mike Richmond (mirichmo) merged 4 commits into
PowerShell:masterfrom
PaulHigin:SSHErrorHang
Jun 29, 2017
Merged

Fix for SSH remoting when SSH client abruptly terminates#4123
Mike Richmond (mirichmo) merged 4 commits into
PowerShell:masterfrom
PaulHigin:SSHErrorHang

Conversation

@PaulHigin

Copy link
Copy Markdown
Contributor

This PR is for Issue #4122

If the SSH client process that PowerShell is using for the SSH transport terminates abruptly the StreamReader will return null instead of closing the pipe for a normal process exit.

The current error stream reading code ignores null StreamReader values resulting in a hang where the remote session never ends.

Fix is to throw an error when this occurs.

<data name="InvalidRoleCapabilityFileExtension" xml:space="preserve">
<value>The provided role capability file {0} does not have the required .psrc extension.</value>
</data>
<data name="SSHTerminated" >

@iSazonov Ilya (iSazonov) Jun 28, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe SSHAbruptlyTerminated ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, will change.

if (string.IsNullOrEmpty(error) ||
if (error == null)
{
return error;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did you decide to return null here and throw in the calling function? Why not just throw here and make the ReadError() function always return non-null strings?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No special reason except that I think of ReadError() helper method as a wrapper to StreamReader. But I agree that it would be cleaner to just throw in ReadError()

@mirichmo

Copy link
Copy Markdown
Member

Ilya (@iSazonov) Do you have any additional comments or concerns?

@iSazonov

Copy link
Copy Markdown
Collaborator

LGTM.

@mirichmo
Mike Richmond (mirichmo) merged commit a2922d6 into PowerShell:master Jun 29, 2017
@PaulHigin
Paul Higinbotham (PaulHigin) deleted the SSHErrorHang branch June 29, 2017 18:16
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
…4123)

* Fix for SSH remoting when SSH client abruptly terminates

* Put error message in localizable string resource

* Renamed error string per code review request

* Removed extra error==null check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Issue-Bug Issue has been identified as a bug in the product WG-Remoting PSRP issues with any transport layer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants