Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,16 @@ class SshConnection(
private val _disconnectedFlow = MutableSharedFlow<Throwable?>(extraBufferCapacity = 1)
val disconnectedFlow: SharedFlow<Throwable?> = _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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) }
}
}
Loading