From 2c384f254eab694e4dcdd58d8e553d616b4f84f3 Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Fri, 24 Jul 2026 00:38:13 +0100 Subject: [PATCH] fix(channel): unregister session channels on close so remote numbers can be reused Opening a second session channel on a connection fails once the first has closed. Against a real OpenSSH server it throws IllegalStateException: Remote channel 1 is already registered and the failure tears down the whole connection, not just the new channel; against Apache MINA sshd the second openSession() simply returns null. SessionChannel never unregistered itself. Forwarding channels are removed via unregisterForwardingChannel and an agent channel is removed on its open-failure path, but a session channel stayed in SshChannelRegistry for the life of the connection - keeping its entry in localRecipientByRemoteRecipient. Once the server reused that remote number for the next channel (legal after both sides have sent CHANNEL_CLOSE, RFC 4254 section 5.3) bindRemoteRecipient rejected it. closeResources() now releases the channel. Every path into it is terminal - receiving CLOSE, or disconnect - and unregistering an already-removed entry is a no-op, so disconnectAll() stays safe. checkAllChannelsClosed() had been inferring 'this connection carried a session' from a closed session still sitting in the registry, i.e. from the leak itself. With channels properly released that signal disappears and the client stops sending its clean disconnect, so the fact is now recorded explicitly in hadSessionChannel. Caught by the existing integration test 'disconnectedFlow emits when server closes channel'. Adds two regression tests, both verified to fail without the fix. --- .../sshlib/client/SessionChannel.kt | 6 ++++ .../connectbot/sshlib/client/SshConnection.kt | 33 +++++++++++++++++-- .../sshlib/client/SessionChannelTest.kt | 25 ++++++++++++++ 3 files changed, 62 insertions(+), 2 deletions(-) diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt index bdd01cf..02229df 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt @@ -215,6 +215,12 @@ class SessionChannel internal constructor( // Channel is gone; if the server never reported an exit, resolve // waiters with "unknown" rather than leaving them suspended. _exitInfo.complete(null) + // Release the local/remote numbers now the lifecycle is CLOSED, so the + // server may legally reuse this remote number for the next channel + // (RFC 4254 §5.3). Every path into here is terminal — receiving CLOSE, + // or disconnect — and unregistering an already-removed entry is a no-op, + // so the connection-wide teardown in disconnectAll() stays safe. + connection.unregisterSessionChannel(localChannelNumber) } internal suspend fun onDisconnected() { diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SshConnection.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SshConnection.kt index a25f0ad..f58e0cb 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SshConnection.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SshConnection.kt @@ -356,6 +356,16 @@ class SshConnection( private val _disconnectedFlow = MutableSharedFlow(extraBufferCapacity = 1) val disconnectedFlow: SharedFlow = _disconnectedFlow.asSharedFlow() + /** + * Whether a session channel has ever been established on this connection. + * + * [checkAllChannelsClosed] used to infer this from a *closed* session channel still sitting in + * the registry. Closed channels are now unregistered so the server may reuse their channel + * numbers, so the fact is recorded explicitly instead of being a side effect of the leak. + */ + @Volatile + private var hadSessionChannel = false + private var serverVersion: String? = null private var clientKexInit: ByteArray? = null private var serverKexInit: ByteArray? = null @@ -2320,6 +2330,19 @@ class SshConnection( channelRegistry.unregister(channel.localChannelNumber) } + /** + * Releases a session channel's local and remote numbers once its lifecycle has reached + * [org.connectbot.sshlib.protocol.SshChannelState.CLOSED]. + * + * Without this the registry keeps the closed channel's remote number indexed forever, so the + * next channel the server confirms with that number — legal to reuse per RFC 4254 §5.3 once + * both sides have sent CHANNEL_CLOSE — is rejected with "Remote channel N is already + * registered", taking the whole connection down with it. + */ + internal fun unregisterSessionChannel(localChannelNumber: Int) { + channelRegistry.unregister(localChannelNumber) + } + internal suspend fun sendChannelOpenConfirmationPublic( recipientChannel: Int, senderChannel: Int, @@ -2884,8 +2907,13 @@ class SshConnection( private suspend fun checkAllChannelsClosed() { val established = channelRegistry.establishedSnapshot() val allClosed = established.all { !it.isOpen } - logger.debug("checkAllChannelsClosed: established=${established.size}, allClosed=$allClosed") - if (allClosed && established.any { it.kind == SshChannelRegistry.Kind.SESSION }) { + logger.debug( + "checkAllChannelsClosed: established=${established.size}, allClosed=$allClosed, " + + "hadSessionChannel=$hadSessionChannel", + ) + // Previously this asked whether a SESSION entry was still present, which only held because + // closed channels were never unregistered. Now that they are, ask the question directly. + if (allClosed && hadSessionChannel) { logger.info("All channels closed, sending disconnect") sendDisconnect() } @@ -3076,6 +3104,7 @@ class SshConnection( val pending = channelRegistry.findPendingSession(localChannelNumber) ?: error("Pending session channel $localChannelNumber disappeared before promotion") channelRegistry.promote(pending, channel) + hadSessionChannel = true logger.debug("Session channel registered: local=$localChannelNumber, remote=$remoteChannelNumber") return channel diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/SessionChannelTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/SessionChannelTest.kt index 2bb93c6..3dd6907 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/SessionChannelTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/SessionChannelTest.kt @@ -21,6 +21,7 @@ import io.mockk.coEvery import io.mockk.coVerify import io.mockk.mockk import io.mockk.slot +import io.mockk.verify import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -258,4 +259,28 @@ class SessionChannelTest { coVerify(exactly = 1) { conn.sendChannelEof(1) } } + + @Test + fun `receiving CLOSE unregisters the channel so its remote number can be reused`() = runTest { + val (channel, connection) = createChannel() + + channel.onClose() + + // The registry indexes channels by the SERVER's channel number. Leaving a + // closed channel registered means the next channel the server confirms + // with that number - legal to reuse once both sides have sent + // CHANNEL_CLOSE (RFC 4254 section 5.3) - is rejected with "Remote channel + // N is already registered", which fails the open AND tears down the whole + // connection. + verify { connection.unregisterSessionChannel(0) } + } + + @Test + fun `disconnect unregisters the channel too`() = runTest { + val (channel, connection) = createChannel() + + channel.onDisconnected() + + verify { connection.unregisterSessionChannel(0) } + } }