Skip to content

fix(channel): unregister session channels on close so remote numbers can be reused - #239

Closed
GlassOnTin wants to merge 1 commit into
connectbot:mainfrom
GlassOnTin:fix/channel-deregister-on-close
Closed

fix(channel): unregister session channels on close so remote numbers can be reused#239
GlassOnTin wants to merge 1 commit into
connectbot:mainfrom
GlassOnTin:fix/channel-deregister-on-close

Conversation

@GlassOnTin

Copy link
Copy Markdown
Contributor

Fixes #238.

The bug

SessionChannel never unregisters itself. Forwarding channels are removed via unregisterForwardingChannel, and an agent channel is removed on its open-failure path, but a session channel stays in SshChannelRegistry for the life of the connection — keeping its entry in localRecipientByRemoteRecipient.

Once the server reuses that remote number for the next channel — legal after both sides have sent CHANNEL_CLOSE (RFC 4254 §5.3) — bindRemoteRecipient rejects it:

IllegalStateException: Remote channel 1 is already registered

…and because that surfaces inside the packet loop, the whole connection goes down rather than just the new channel. Against Apache MINA sshd the second openSession() just returns null instead, which is why the issue lists two symptoms.

The fix

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 the connection-wide disconnectAll() stays safe.

The part worth your eyes

checkAllChannelsClosed() was inferring "this connection carried a session" from a closed session still sitting in the registry — i.e. from the leak itself:

if (allClosed && established.any { it.kind == SshChannelRegistry.Kind.SESSION })

Once channels are properly released that signal disappears, the client stops sending its clean disconnect, and disconnectedFlow never emits. Your existing integration test disconnectedFlow emits when server closes channel caught this immediately — it passed on a clean tree and failed with my first version of the fix.

So the fact is now recorded explicitly:

if (allClosed && hadSessionChannel)

One behavioural nuance to flag rather than bury: the old condition required a session entry to still be present, the new one asks whether a session ever existed. These differ in one case — a session opens and closes, then only forwarding channels remain and later close. Previously that would not have triggered the clean disconnect; now it does. That seemed closer to the intent, but it is your call and I'm happy to scope it to hadSessionChannel && established.any { … } if you'd rather preserve the exact prior behaviour.

Testing

  • Two regression tests added to SessionChannelTest (receive-CLOSE and disconnect paths). Both verified to fail without the fix — I reverted the one-line change and re-ran to confirm they actually gate the bug rather than passing vacuously.
  • Full :sshlib:test green locally, including the Testcontainers integration tests.
  • Also verified end-to-end from the downstream side: the original reproduction (an Android client running repeated exec over one long-lived connection against real OpenSSH) no longer collides.

Notes

  • AgentChannel looks like it has the same shape — no unregister on its close path, only on open-failure. I've left it alone since I have no way to exercise agent forwarding here; happy to extend the fix if you'd like it in the same PR.
  • spotlessApply rewrites a large part of the tree on a clean checkout here (some tool-version drift), so I've kept the diff to only the three files I touched.

…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.
@GlassOnTin

Copy link
Copy Markdown
Contributor Author

Superseded by the fix in 0.4.1 — notifyChannelClosed() deregisters the entry, and SshClientConfig.autoDisconnectOnLastChannelClose provides the escape hatch a multiplexing client needs. Verified 12/12 open/close cycles on one connection against MINA with the flag off; details on #238. Closing as redundant.

@GlassOnTin GlassOnTin closed this Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant