x/crypto/ssh: semantics around running session.Wait() after calling session.Run(), when EOF messages are sent

[ianvernon]

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

$ go version
1.12.5

Does this issue reproduce with the latest release?

I am using a vendored version of the library with commit e84da0312774c21d64ee2317962ef669b27ffb41

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

go env Output

$ go env
go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/vagrant/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/vagrant/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/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-build896755704=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I will work on providing an exact reproducer of this via a small go program, but do not have this on hand immediately. I can do this if you like. My question is more about how to properly use the SSH library, as I feel like I am misunderstanding how it works.

I am wondering about the semantics behind running session.Wait() after calling session.Run().

It appears that session.Run() calls session.Start() followed by session.Wait().

I've found that if I try calling session.Wait() again after calling session.Run(), my code will hang infinitely. Is this because there is nothing to wait upon after session.Run() terminates, since the remote command has exited, so session.Wait() is waiting for something, while nothing is being sent?

I also am confused about how sessions are closed. I observe when running with debugMux set to true locally, that my session gets sent a channelEOFMsg after I have called session.Run(), but before I call session.Close(). When I try to close the session, I get the error failed to close session: EOF. I would expect that after calling Run, that I'd be able to close the session gracefully. Who is sending these EOF's? Is that the expected behavior for running a command on a session, in that the session will get an EOF when the command is done running?

Output from running with debugMux=true:

--------------> session.Run() <----------------
2019/06/05 08:17:03 send global(2): ssh.channelOpenMsg{ChanType:"session", PeersID:0x2, PeersWindow:0x200000, MaxPacketSize:0x8000, TypeSpecificData:[]uint8(nil)}
2019/06/05 08:17:03 decoding(2): 91 &ssh.channelOpenConfirmMsg{PeersID:0x2, MyID:0x0, MyWindow:0x0, MaxPacketSize:0x8000, TypeSpecificData:[]uint8{}} - 17 bytes
2019/06/05 08:17:03 send(2): ssh.channelRequestMsg{PeersID:0x0, Request:"exec", WantReply:true, RequestSpecificData:[]uint8{0x0, 0x0, 0x0, 0x56, 0x6b, 0x75, 0x62, 0x65, 0x63, 0x74, 0x6c, 0x20, 0x67, 0x65, 0x74, 0x20, 0x70, 0x6f, 0x64, 0x73, 0x20, 0x2d, 0x2d, 0x61, 0x6c, 0x6c, 0x2d, 0x6e, 0x61, 0x6d, 0x65, 0x73, 0x70, 0x61, 0x63, 0x65, 0x73, 0x20, 0x2d, 0x6f, 0x20, 0x6a, 0x73, 0x6f, 0x6e, 0x70, 0x61, 0x74, 0x68, 0x3d, 0x27, 0x7b, 0x2e, 0x69, 0x74, 0x65, 0x6d, 0x73, 0x5b, 0x2a, 0x5d, 0x2e, 0x6d, 0x65, 0x74, 0x61, 0x64, 0x61, 0x74, 0x61, 0x2e, 0x64, 0x65, 0x6c, 0x65, 0x74, 0x69, 0x6f, 0x6e, 0x54, 0x69, 0x6d, 0x65, 0x73, 0x74, 0x61, 0x6d, 0x70, 0x7d, 0x27}}
2019/06/05 08:17:03 decoding(2): 93 &ssh.windowAdjustMsg{PeersID:0x2, AdditionalBytes:0x200000} - 9 bytes
2019/06/05 08:17:03 decoding(2): 99 &ssh.channelRequestSuccessMsg{PeersID:0x2} - 5 bytes

-------------> where is this EOF coming from? <---------------
2019/06/05 08:17:03 send(2): ssh.channelEOFMsg{PeersID:0x0}

-----------> close session here <-----------------
2019/06/05 08:17:03 decoding(2): 98 &ssh.channelRequestMsg{PeersID:0x2, Request:"exit-status", WantReply:false, RequestSpecificData:[]uint8{0x0, 0x0, 0x0, 0x0}} - 25 bytes
2019/06/05 08:17:03 decoding(2): 96 &ssh.channelEOFMsg{PeersID:0x2} - 5 bytes
2019/06/05 08:17:03 decoding(2): 97 &ssh.channelCloseMsg{PeersID:0x2} - 5 bytes
2019/06/05 08:17:03 send(2): ssh.channelCloseMsg{PeersID:0x0}
2019/06/05 08:17:03 send(2): ssh.channelCloseMsg{PeersID:0x0}

-------------> log of error from closing session <----------------
failed to close session: EOF

What did you expect to see?

I would expect to not get an EOF after calling Run() on a session, and for Wait() to be idempotent / to return immediately if there is nothing to wait upon. I would also expect that EOF would not get sent by Run(), and that I would not get an error closing a session when nothing else has tried to close it.

What did you see instead?

See above.

I will work on a small reproducer locally, but I hope that the above is enough context / information for me to get some answers, as I feel like this is moreso me not understanding how the library is supposed to be used.

[dmitshur]

/cc @hanwen @FiloSottile

[hanwen]

session.Run is syntactic sugar for Start + Wait. Don't call Wait if you use Run.

re. EOF, EOF signifies the end of the writing to a channel (stdout/stderr), so you can send it only once.

I am open to review changes to be more intelligent here, but the behavior per se is not a bug.

[ianvernon]

@hanwen thanks for the reply!

Regarding your comments about EOF - it looks like calling Run sends EOF, so we do not need to close the session after we have called Run, and Run has returned, because Run implicitly closes the session as well?

I don't think there is a bug here either, it was just a bit hard to understand the idempotency of session operations. Perhaps I can open up a PR for some godoc changes describing this?

[wiml]

I ran into a similar problem, and I agree it might be best fixed just by adding some information to the documentation. In particular I had to look at the sources to see which functions were primitive (Start, Shell, and Wait) and which were conveniences which could be implemented in terms of other public API (Run, Output, CombinedOutput).

On the other hand, even though calling Wait() twice is a programmer error, the behavior of hanging seems suboptimal. What about a change like this:

• In the Session struct, make exitStatus a receive-only channel
• In newSession(), only assign the receive half of the channel to exitStatus, and give the send half to the waiting goroutine.
• after sending the exit status to the channel, close the channel.
• in Wait(), check whether we received on a closed channel, and either panic or throw an error describing the situation (I prefer panic here, since it's a programmer error; but in any case, it had better not just return the default nil value!)

[hanwen]

Yes, please send a change

your suggestion to change things sound promising. Can you try if you can make this work?

The doc change should be separte from the functional change.

[wiml]

Hmm, I'd love to send a PR or two, but it looks like that'd require my employer to sign the Google CLA, and while that's in principle possible, it's lot of work for four lines of code. Unless/until we work through that red tape for other reasons, I'm going to have to pass on sending any actual changes. :(

[ianvernon]

I’ll try to take a shot at this !