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

Jump to bottom

x/crypto/ssh: newChannel leak after Reject() #35127

Open
ziyan opened this issue Oct 24, 2019 · 4 comments
Open

x/crypto/ssh: newChannel leak after Reject() #35127

ziyan opened this issue Oct 24, 2019 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@ziyan
Copy link

ziyan commented Oct 24, 2019 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.12.7 linux/amd64

Does this issue reproduce with the latest release?

Most likely.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ziyan/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ziyan/go"
GOPROXY=""
GORACE=""
GOROOT="/home/ziyan/software/go"
GOTMPDIR=""
GOTOOLDIR="/home/ziyan/software/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build414841088=/tmp/go-build -gno-record-gcc-switches"

What did you do?

conn, channels, requests, err := ssh.NewServerConn(...)
for channel := range channels {
	channel.Reject(ssh.ConnectionFailed, "rejecting channel")
}

Then keep trying to open new direct-tcpip channel on a SSH connection:

ssh -N gosshserver -L 9999:name:9999
telnet localhost 9999
telnet localhost 9999
telnet localhost 9999

What did you expect to see?

No memory leak over time.

What did you see instead?

Unconstrained memory leak.

  112.52MB 68.69% 68.69%   155.30MB 94.81%  golang.org/x/crypto/ssh.(*mux).newChannel
   23.50MB 14.35% 83.04%    23.50MB 14.35%  sync.NewCond
   15.50MB  9.46% 92.50%       36MB 21.98%  golang.org/x/crypto/ssh.newBuffer
       7MB  4.27% 96.77%        7MB  4.27%  golang.org/x/crypto/ssh.(*connectionState).readPacket
    2.50MB  1.53% 98.30%       26MB 15.87%  golang.org/x/crypto/ssh.newCond
    1.28MB  0.78% 99.08%     1.28MB  0.78%  golang.org/x/crypto/ssh.(*chanList).add
@gopherbot gopherbot added this to the Unreleased milestone Oct 24, 2019
@ziyan
Copy link
Author

ziyan commented Oct 24, 2019

I can see that newChannel() method of mux does:

m.chanList.add(ch)

But chanList.remove() is called only in two places, both of which is inside handlePacket() of channel. This probably means if client side does not send msgChannelClose or channelOpenFailureMsg, the channel object will be left in m.chanList forever.

@dmitshur
Copy link
Contributor

/cc @hanwen per owners.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 24, 2019
ziyan added a commit to ziyan/sshrejectleak that referenced this issue Oct 25, 2019
@ziyan
Reproduce golang/go#35127.
d67b50f
@ziyan
Copy link
Author

ziyan commented Oct 25, 2019

I've created a minimal repro here: https://github.com/ziyan/sshrejectleak

@hanwen
Copy link
Contributor

hanwen commented Oct 25, 2019

sounds believable. Can you whip up a change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@hanwen @ziyan @dmitshur @gopherbot