From fe1af3a4189650f6a61d30bfcb431e61bb141f41 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sun, 5 Jul 2026 20:20:39 +0200 Subject: [PATCH] ssh: make channel.close idempotent channel.close closes the internal Go channels (msg, incomingRequests); calling it twice panics with "close of closed channel". Both current callers run on the mux loop and are serialized, so the panic is not reachable today. Harden close by guarding it with sync.Once, so that the connection does not become fragile to any future bug that reaches close twice. Change-Id: I5d8d948367538ea5561724033c4ee4daa668e221 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/797060 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Filippo Valsorda Reviewed-by: David Chase Reviewed-by: Junyang Shao --- ssh/channel.go | 28 +++++++++++++++++----------- ssh/mux_test.go | 12 ++++++++++++ 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/ssh/channel.go b/ssh/channel.go index afc9aef..a65c79d 100644 --- a/ssh/channel.go +++ b/ssh/channel.go @@ -216,6 +216,10 @@ type channel struct { // packetPool has a buffer for each extended channel ID to // save allocations during writes. packetPool map[uint32][]byte + + // closeOnce guards close so it is idempotent: closing the internal Go + // channels (msg, incomingRequests) more than once would panic. + closeOnce sync.Once } // writePacket sends a packet. If the packet is a channel close, it updates @@ -393,17 +397,19 @@ func (c *channel) ReadExtended(data []byte, extended uint32) (n int, err error) } func (c *channel) close() { - c.pending.eof() - c.extPending.eof() - close(c.msg) - close(c.incomingRequests) - c.writeMu.Lock() - // This is not necessary for a normal channel teardown, but if - // there was another error, it is. - c.sentClose = true - c.writeMu.Unlock() - // Unblock writers. - c.remoteWin.close() + c.closeOnce.Do(func() { + c.pending.eof() + c.extPending.eof() + close(c.msg) + close(c.incomingRequests) + c.writeMu.Lock() + // This is not necessary for a normal channel teardown, but if + // there was another error, it is. + c.sentClose = true + c.writeMu.Unlock() + // Unblock writers. + c.remoteWin.close() + }) } // responseMessageReceived is called when a success or failure message is diff --git a/ssh/mux_test.go b/ssh/mux_test.go index a7dff34..1370091 100644 --- a/ssh/mux_test.go +++ b/ssh/mux_test.go @@ -1213,6 +1213,18 @@ func TestChannelUnexpectedResponsesDiscarded(t *testing.T) { } } +func TestChannelCloseIdempotent(t *testing.T) { + a, b := memPipe() + defer a.Close() + defer b.Close() + + m := newMux(a) + ch := m.newChannel("session", channelOutbound, nil) + + ch.close() + ch.close() // must be a no-op, not a panic +} + func TestChannelConcurrentRequests(t *testing.T) { writer, reader, mux := channelPair(t) defer writer.Close()