-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
ssh: hangs if remote side closes connection #5877
Labels
Comments
Session.wait says: default: // This handles keepalives and matches // OpenSSH's behaviour. if msg.WantReply { s.writePacket(marshal(msgChannelFailure, channelRequestFailureMsg{ PeersId: s.remoteId, })) } unless a process is wait()ed on, keepalive messages will queue up in the session's message channel. When the channel is full, the ClientConn.mainLoop will block (in the stack trace, this is in client.go:324) The bug here is that session channel servicing is done in session.Wait(), and If session.Wait is not called, the |
Session.wait says: default: // This handles keepalives and matches // OpenSSH's behaviour. if msg.WantReply { s.writePacket(marshal(msgChannelFailure, channelRequestFailureMsg{ PeersId: s.remoteId, })) } unless a process is wait()ed on, keepalive messages will queue up in the session's message channel. When the channel is full, the ClientConn.mainLoop will block (in the stack trace, this is in client.go:324) Strictly speaking, the bug is in the docs for not mentioning this, but overall the SSH package is fragile: incorrect use of a single channel can block the entire connection. |
I have some ideas (and maybe even some time), for some rewrites. What do you propose for the short term? * handle the keepalive in ClientConn.mainLoop? (that would require hardcoding the keepalive string?) * have a separate channel that copies from clientChan.msg to a Session.exit chan, for the sole purpose of discarding the keepalives. It's a little ugly though. * update the doc of Wait |
My desire (although time is always a problem), is to work through the layers again, from transport up to session. Transport, handshaking and close all need solid tests before considering any other major surgery. I think for the moment, handling keepalives in the mainLoop would suffice. From memory there is logic for other global message types there. |
I was thinking of writing the channel system on top of something like type transport interface { ReadPacket() []byte, error WritePacket([]byte) error } and make it so client and server use the same code (the RFC suggests that this should be possible). This could be easily unittested without the need to set up a secure connection. |
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
The text was updated successfully, but these errors were encountered: