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) } + } }