Skip to content
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

Closed
hanwen opened this issue Jul 12, 2013 · 13 comments
Closed

ssh: hangs if remote side closes connection #5877

hanwen opened this issue Jul 12, 2013 · 13 comments

Comments

@hanwen
Copy link
Contributor

hanwen commented Jul 12, 2013

if I let SSHd close my go.crypto/ssh client connection ( 
https://golang.org/issue/5875),  none of my SSH I/O exits.

Sending sigquit yields the following trace  (edited to remove confidential data)

this is with

Version: 1f2453f1ed54

goroutine 44 [chan receive]:
go.crypto/ssh/ssh.(*ClientConn).sendGlobalRequest(0x2106e4e70, 0x31da00, 0x2107749f0,
0x0, 0x0, ...)
    go.crypto/ssh/ssh/client.go:418 +0x101
go.crypto/ssh/ssh.(*ClientConn).ListenTCP(0x2106e4e70, 0x2107749c0, 0x21076d670, 0xf,
0x2107749c0, ...)
    go.crypto/ssh/ssh/tcpip.go:46 +0xc7
go.crypto/ssh/ssh.(*ClientConn).Listen(0x2106e4e70, 0x372a40, 0x3, 0x21076d670, 0xf, ...)
    go.crypto/ssh/ssh/tcpip.go:24 +0x96

goroutine 11 [chan send]:
go.crypto/ssh/ssh.(*ClientConn).mainLoop(0x2106e4e70)
    go.crypto/ssh/ssh/client.go:324 +0xf2a
created by go.crypto/ssh/ssh.clientWithAddress
    go.crypto/ssh/ssh/client.go:56 +0x21e

goroutine 12 [semacquire]:
sync.runtime_Semacquire(0x2106accd8)
    /home/hanwen/vc/go/src/pkg/runtime/zsema_darwin_amd64.c:165 +0x30
sync.(*Cond).Wait(0x2107379c0)
    /home/hanwen/vc/go/src/pkg/sync/cond.go:74 +0x97
go.crypto/ssh/ssh.(*buffer).Read(0x21074a100, 0x21074e000, 0x8000, 0x8000, 0x0, ...)
    go.crypto/ssh/ssh/buffer.go:93 +0x23e
go.crypto/ssh/ssh.(*chanReader).Read(0x210734db0, 0x21074e000, 0x8000, 0x8000, 0x8000,
...)
    go.crypto/ssh/ssh/channel.go:558 +0x4e
io.Copy(0x688698, 0x2106ac010, 0x68fc38, 0x210734db0, 0x0, ...)
    /home/hanwen/vc/go/src/pkg/io/io.go:348 +0x1c8
go.crypto/ssh/ssh.func·007(0x24110, 0x25ca60)
    go.crypto/ssh/ssh/session.go:512 +0x5e
go.crypto/ssh/ssh.func·004(0x210734dd0)
    go.crypto/ssh/ssh/session.go:378 +0x2c
created by go.crypto/ssh/ssh.(*Session).start
    go.crypto/ssh/ssh/session.go:379 +0x1bd

goroutine 13 [semacquire]:
sync.runtime_Semacquire(0x210788610)
    /home/hanwen/vc/go/src/pkg/runtime/zsema_darwin_amd64.c:165 +0x30
sync.(*Cond).Wait(0x210737980)
    /home/hanwen/vc/go/src/pkg/sync/cond.go:74 +0x97
go.crypto/ssh/ssh.(*buffer).Read(0x21074a0a0, 0x21078f000, 0x1000, 0x1000, 0x0, ...)
    go.crypto/ssh/ssh/buffer.go:93 +0x23e
go.crypto/ssh/ssh.(*chanReader).Read(0x210734da0, 0x21078f000, 0x1000, 0x1000, 0x1d, ...)
    go.crypto/ssh/ssh/channel.go:558 +0x4e

goroutine 16 [chan receive]:
go.crypto/ssh/ssh.(*tcpListener).Accept(0x21074afa0, 0x68ffb8, 0x21078d360, 0x0, 0x0)
    go.crypto/ssh/ssh/tcpip.go:135 +0x34

goroutine 23 [chan receive]:
go.crypto/ssh/ssh.(*tcpListener).Accept(0x210778440, 0x68ffb8, 0x210774630, 0x0, 0x0)
    go.crypto/ssh/ssh/tcpip.go:135 +0x34

goroutine 32 [chan receive]:
go.crypto/ssh/ssh.(*tcpListener).Accept(0x21076cee0, 0x68ffb8, 0x2107745a0, 0x0, 0x0)
    go.crypto/ssh/ssh/tcpip.go:135 +0x34
@hanwen
Copy link
Contributor Author

hanwen commented Jul 16, 2013

Comment 1:

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

@hanwen
Copy link
Contributor Author

hanwen commented Jul 16, 2013

Comment 2:

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.

@davecheney
Copy link
Contributor

Comment 3:

Yes, this is my mistake. I was so enamoured with channels I overused them in this
package. I think a large rewrite is in order.

@hanwen
Copy link
Contributor Author

hanwen commented Jul 16, 2013

Comment 4:

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

@davecheney
Copy link
Contributor

Comment 5:

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.

@hanwen
Copy link
Contributor Author

hanwen commented Jul 16, 2013

Comment 6:

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.

@rsc
Copy link
Contributor

rsc commented Oct 18, 2013

Comment 7:

Labels changed: added priority-later, removed priority-triage.

Status changed to Accepted.

@hanwen
Copy link
Contributor Author

hanwen commented Nov 11, 2013

Comment 8:

this has been fixed in gosshnew

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 9:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 10:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 11:

Labels changed: added repo-crypto.

@gopherbot
Copy link
Contributor

Comment 12 by peter.waller:

Didn't gosshnew become go.crypto/ssh? and if so, can this issue be closed?

@agl
Copy link
Contributor

agl commented May 10, 2014

Comment 13:

gosshnew has been merged and, based on #8, the fix came with it.

Status changed to Fixed.

@mikioh mikioh changed the title go.crypto/ssh: hangs if remote side closes connection ssh: hangs if remote side closes connection Jan 8, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants