8000 SSH Remoting should handle multi-line error messages from SSH client by PaulHigin · Pull Request #3612 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

SSH Remoting should handle multi-line error messages from SSH client #3612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 22, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1453,11 +1453,6 @@ internal override void CreateAsync()
out _stdOutReader,
out _stdErrReader);

_sshProcess.Exited += (sender, args) =>
{
CloseConnection();
};

// Start error reader thread.
StartErrorThread(_stdErrReader);

Expand Down Expand Up @@ -1515,38 +1510,77 @@ private void ProcessErrorThread(object state)

while (true)
{
string error = reader.ReadLine();
string error = ReadError(reader);
if (string.IsNullOrEmpty(error))
{
// Ignore blank error messages.
continue;
}
if (error.IndexOf("WARNING:", StringComparison.OrdinalIgnoreCase) > -1)
{
// Handle as interactive warning message.
Console.WriteLine(error);
}
else
{
// Any SSH client error results in a broken session.
PSRemotingTransportException psrte = new PSRemotingTransportException(
PSRemotingErrorId.IPCServerProcessReportedError,
RemotingErrorIdStrings.IPCServerProcessReportedError,
string.IsNullOrEmpty(error) ?
RemotingErrorIdStrings.SSHClientEndNoErrorMessage
: StringUtil.Format(RemotingErrorIdStrings.SSHClientEndWithErrorMessage, error));
RaiseErrorHandler(new TransportErrorOccuredEventArgs(psrte, TransportMethodEnum.CloseShellOperationEx));
CloseConnection();
}

// Any SSH client error results in a broken session.
PSRemotingTransportException psrte = new PSRemotingTransportException(
PSRemotingErrorId.IPCServerProcessReportedError,
RemotingErrorIdStrings.IPCServerProcessReportedError,
StringUtil.Format(RemotingErrorIdStrings.SSHClientEndWithErrorMessage, error));
HandleSSHError(psrte);
}
}
catch (ObjectDisposedException) { }
catch (Exception e)
{
string errorMsg = (e.Message != null) ? e.Message : string.Empty;
_tracer.WriteMessage("SSHClientSessionTransportManager", "ProcessErrorThread", Guid.Empty,
"Transport manager error thread ended with error: {0}", errorMsg);

PSRemotingTransportException psrte = new PSRemotingTransportException(
StringUtil.Format(RemotingErrorIdStrings.SSHClientEndWithErrorMessage, errorMsg),
e);
HandleSSHError(psrte);
}
}

private void HandleSSHError(PSRemotingTransportException psrte)
{
RaiseErrorHandler(new TransportError 10000 OccuredEventArgs(psrte, TransportMethodEnum.CloseShellOperationEx));
CloseConnection();
}

private static string ReadError(StreamReader reader)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be feasible to write a negative test that covers this scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this time we don't have any functional tests for SSH based remoting because we don't currently have a way to automatically set it up. I thought I had an open issue about this (create an Enable-SSHRemoting cmdlet that downloads and sets up SSH for different platforms) but I don't see it. I'll add one but I do see that I created an RFC for it (https://github.com/PowerShell/PowerShell-RFC/blob/master/1-Draft/RFC0012-Enable-SSH-Remoting.md)

In any case adding negative tests (tests with malformed error stream) would require test hooks and mock ups for the transport. I am not sure it would be worthwhile at this point given that we don't yet have functional tests.

{
// Blocking read from StdError stream
string error = reader.ReadLine();

if (string.IsNullOrEmpty(error) ||
error.IndexOf("WARNING:", StringComparison.OrdinalIgnoreCase) > -1)
{
// Handle as interactive warning message
Console.WriteLine(error);
return string.Empty;
}

// SSH may return a multi-line error message
System.Text.StringBuilder sb = new Text.StringBuilder(error);
var running = true;
while (running)
{
try
{
var task = reader.ReadLineAsync();
if (task.Wait(1000) && (task.Result != null))
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious about the reasoning behind the choice for a 1 second timeout over an await or a straight wait(). Did you encounter stalls in the stream reader when working on the fix or scenarios where the error lines were written with a long time gap between them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. The reason is that the error stream reader is based on a pipe stream which is open ended and never ends. So the read operation blocks until data is available. Previously I assumed that a single stream text line constituted a single error message and when received was converted into a transport error message and the connection terminated. But SSH will return multiple line error messages (which normally just appear in the console) and I need to capture them as well as I can before terminating the connection. There is no way to know how many error lines will be streamed and so it is impossible not to hang the application by blocking on the read. My solution is to use the line async read with a timeout. I chose 1 second through ad hoc experimentation.

Copy link
Member

Choose a reason for hiding this comment

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

@PaulHigin thanks for the clarification! Could you please add summarize this and add some comments to this line of code? I'm sure that will be very helpful to people reading the code 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

{
sb.Append(Environment.NewLine);
sb.Append(task.Result);
}
else
{
running = false;
}
}
catch (Exception)
{
running = false;
}
}

return sb.ToString();
}

private void StartReaderThread(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1615,9 +1615,6 @@ All WinRM sessions connected to Windows PowerShell session configurations, such
<data name="SSHClientEndWithErrorMessage" xml:space="preserve">
<value>The SSH client session has ended with error message: {0}</value>
</data>
<data name="SSHClientEndNoErrorMessage" xml:space="preserve">
<value>The SSH client session has ended with no error message.</value>
</data>
<data name="MissingRequiredSSHParameter" xml:space="preserve">
<value>The provided SSHConnection hashtable is missing the required ComputerName or HostName parameter.</value>
</data>
Expand Down
0