-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Changes from all commits
0bba356
722b5f6
c789d8e
d5da165
c8b4ba1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1453,11 +1453,6 @@ internal override void CreateAsync() | |
out _stdOutReader, | ||
out _stdErrReader); | ||
|
||
_sshProcess.Exited += (sender, args) => | ||
{ | ||
CloseConnection(); | ||
}; | ||
|
||
// Start error reader thread. | ||
StartErrorThread(_stdErrReader); | ||
|
||
|
@@ -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) | ||
{ | ||
// 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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||
|
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.
Would it be feasible to write a negative test that covers this scenario?
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.
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.