x/crypto/ssh: Client.Listen doc needs clarification on avoiding hangs #77035
Description
Go version
go version go1.24.0 linux/amd64
Output of go env in your module/workspace:
AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='gccgo'
GO111MODULE='on'
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/home/will/.cache/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/home/will/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1020578408=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/home/will/xcrypto/go.mod'
GOMODCACHE='/home/will/go/pkg/mod'
GONOPROXY='<redacted>.extrahop.com/*'
GONOSUMDB='<redacted>.extrahop.com/*'
GOOS='linux'
GOPATH='/home/will/go'
GOPRIVATE='<redacted>.extrahop.com/*'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/will/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.24.0'
GOWORK=''
PKG_CONFIG='pkg-config'What did you do?
The doc for Client.Listen (implied to apply to Client.ListenTCP and Client.ListenUnix) says: "The listener must be serviced, or the SSH connection may hang." It's not clearly stated that, to avoid leaking goroutines or blocking other listeners, merely calling Close on the listener isn't sufficient servicing, and rather you need to continue calling Accept until either Accept returns an error or Close returns.
The problem is that forwardList.forward holds the forwardList's mutex while sending on the listener's channel, and the listener's Close calls forwardList.remove, which tries to take that same mutex before closing that same channel. So if the listener's channel is full (from not calling Accept), so that forwardList.forward blocks while holding the mutex, then Close blocks too (trying to take that mutex in forwardList.remove). Calling Accept to receive on the channel and unblock forwardList.forward is the only way to resolve this.
I added the following test to ssh/client_test.go:
type channelForwardMsg2 struct {
Addr string
Rport uint32
}
func TestListenerCloseWithoutAccept(t *testing.T) {
c1, c2, err := netPipe()
if err != nil {
t.Fatalf("netPipe: %v", err)
}
defer c1.Close()
defer c2.Close()
serverConnChan := make(chan *ServerConn, 1)
var serverIncomingReqs <-chan *Request
go func() {
conf := &ServerConfig{NoClientAuth: true}
conf.AddHostKey(testSigners["rsa"])
conn, _, reqs, err := NewServerConn(c2, conf)
if err != nil {
t.Fatalf("NewServerConn: %v", err)
}
serverIncomingReqs = reqs
serverConnChan <- conn
}()
clientConn, chans, reqs, err := NewClientConn(c1, "", &ClientConfig{
HostKeyCallback: InsecureIgnoreHostKey(),
})
if err != nil {
t.Fatalf("NewClientConn: %v", err)
}
client := NewClient(clientConn, chans, reqs)
serverConn := <-serverConnChan
go func() {
// server accepts the upcoming tcpip-forward request
req := <-serverIncomingReqs
if req.Type != "tcpip-forward" {
t.Fatalf("expected tcpip-forward request, got %q", req.Type)
}
var msg channelForwardMsg2
if err := Unmarshal(req.Payload, &msg); err != nil {
t.Fatalf("Unmarshal: %v", err)
}
err = req.Reply(true, nil)
if err != nil {
t.Fatalf("reply failed: %v", err)
}
DiscardRequests(serverIncomingReqs)
}()
// client sends tcpip-forward request
sshListener, err := client.Listen("tcp", "127.0.0.1:54321")
if err != nil {
t.Fatal(err)
}
// The client's tcpListener's golang channel has a capacity of 1, so send
// two SSH channel open requests to fill it and cause forwardList.forward()
// to block. (The OpenChannel calls block until serverConn is closed, and
// then return an error.)
go func() {
serverConn.OpenChannel(
"forwarded-tcpip", Marshal(channelOpenDirectMsg{
raddr: "127.0.0.1",
rport: 54321,
laddr: "127.0.0.2",
lport: 54321,
}))
}()
go func() {
serverConn.OpenChannel(
"forwarded-tcpip", Marshal(channelOpenDirectMsg{
raddr: "127.0.0.1",
rport: 54321,
laddr: "127.0.0.2",
lport: 65432,
}))
}()
// sleep to give the client's handleChannels goroutine time to process both
// SSH channel open requests and block sending on the tcpListener's golang
// channel in forwardList.forward()
time.Sleep(100 * time.Millisecond)
doneCh := make(chan struct{})
go func() {
// this blocks forever trying to take the forwardList's mutex
sshListener.Close()
close(doneCh)
}()
serverConn.Close()
client.Close()
// this blocks forever
<-doneCh
}
What did you see happen?
The test hangs forever because sshListener.Close() blocks in forwardList.remove() trying to take the lock held by forwardList.forward(). There is also another goroutine blocked in forwardList.closeAll() trying to take the same lock.
Dumping all the goroutine stack traces immediately prior to the receive on doneCh looks like:
goroutine 627 [running]:
golang.org/x/crypto/ssh.TestListenerCloseWithoutAccept(0xc0002a7500)
/home/will/xcrypto/ssh/client_test.go:460 +0x5df
testing.tRunner(0xc0002a7500, 0x799998)
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/testing/testing.go:1792 +0xf4
created by testing.(*T).Run in goroutine 1
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/testing/testing.go:1851 +0x413
goroutine 1 [chan receive]:
testing.(*T).Run(0xc000102a80, {0x77ec23?, 0xc00002db30?}, 0x799998)
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/testing/testing.go:1859 +0x431
testing.runTests.func1(0xc000102a80)
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/testing/testing.go:2279 +0x37
testing.tRunner(0xc000102a80, 0xc00002dc70)
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/testing/testing.go:1792 +0xf4
testing.runTests(0xc000267200, {0xa38560, 0xbc, 0xbc}, {0xa3c120?, 0x7?, 0xa3afa0?})
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/testing/testing.go:2277 +0x4b4
testing.(*M).Run(0xc0001403c0)
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/testing/testing.go:2142 +0x64a
main.main()
_testmain.go:433 +0x9b
goroutine 663 [sync.Mutex.Lock]:
internal/sync.runtime_SemacquireMutex(0x74bce0?, 0xb0?, 0xc000011278?)
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/runtime/sema.go:95 +0x25
internal/sync.(*Mutex).lockSlow(0xc00014f1f0)
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/internal/sync/mutex.go:149 +0x15d
internal/sync.(*Mutex).Lock(...)
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/internal/sync/mutex.go:70
sync.(*Mutex).Lock(...)
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/sync/mutex.go:46
golang.org/x/crypto/ssh.(*forwardList).remove(0xc00014f1f0, {0x772fed, 0x3}, {0xc000197460, 0xf})
/home/will/xcrypto/ssh/tcpip.go:283 +0x73
golang.org/x/crypto/ssh.(*tcpListener).Close(0xc00027b2c0)
/home/will/xcrypto/ssh/tcpip.go:359 +0xc7
golang.org/x/crypto/ssh.TestListenerCloseWithoutAccept.func5()
/home/will/xcrypto/ssh/client_test.go:454 +0x25
created by golang.org/x/crypto/ssh.TestListenerCloseWithoutAccept in goroutine 627
/home/will/xcrypto/ssh/client_test.go:453 +0x585
goroutine 635 [sync.Mutex.Lock]:
internal/sync.runtime_SemacquireMutex(0x0?, 0x0?, 0xc00038aea0?)
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/runtime/sema.go:95 +0x25
internal/sync.(*Mutex).lockSlow(0xc00014f1f0)
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/internal/sync/mutex.go:149 +0x15d
internal/sync.(*Mutex).Lock(...)
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/internal/sync/mutex.go:70
sync.(*Mutex).Lock(...)
/home/will/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/sync/mutex.go:46
golang.org/x/crypto/ssh.(*forwardList).closeAll(0xc00014f1f0)
/home/will/xcrypto/ssh/tcpip.go:296 +0x45
golang.org/x/crypto/ssh.NewClient.func1()
/home/will/xcrypto/ssh/client.go:63 +0x32
created by golang.org/x/crypto/ssh.NewClient in goroutine 627
/home/will/xcrypto/ssh/client.go:61 +0x156
goroutine 637 [chan send]:
golang.org/x/crypto/ssh.(*forwardList).forward(0xc00013eea0?, {0x772fed, 0x3}, {0xc00006cef0, 0xf}, {0x7f8748, 0xc00011b170}, {0x7fa100, 0xc00033a0c0})
/home/will/xcrypto/ssh/tcpip.go:309 +0x245
golang.org/x/crypto/ssh.(*forwardList).handleChannels(0xc00014f1f0, 0xc0004d60e0)
/home/will/xcrypto/ssh/tcpip.go:270 +0x445
created by golang.org/x/crypto/ssh.(*Client).handleForwards in goroutine 627
/home/will/xcrypto/ssh/tcpip.go:106 +0x8c
What did you expect to see?
Test doesn't hang, and/or the docs for ssh.Client make it clear that Listeners specifically must be serviced by repeatedly calling Accept until either Accept returns an error or Close returns. (Ignoring any fairness/starvation guarantees provided by sync.Mutex, I think this could mean calling Accept arbitrarily many times, even while Close is in progress.)