Repository navigation
Ignore stale SSH callbacks after reconnect - #14
Merged
Merged
Conversation
After a pipe restart, callbacks from the old client can arrive after the new client has been stored. An old close callback could clear _client, leaving the new connection open with no way to close it. Old channel data could also reach the new session and corrupt its protocol stream. Check client and channel identity before changing pipe state or forwarding data. Capture the client in the channel-open callback as well: the new client can exist before its channel opens, so checking for an empty channel slot is not enough. Close channels opened by an old client.
Contributor
|
I will look at this. I think it is conceptually correct for the issue it attempts to fix, but I also suspect it opens up for a new weakness related to EOF. |
_channel was only set in on_open, so the stale callback checks dropped every callback from a channel that was still opening. libssh can process the open confirmation, EOF and close in one poll. The EOF then reaches the pipe before on_open. If the close is processed before the open is driven, the channel is finalized without on_open or on_close. Thus the dropped EOF was the only signal, and the pipe reported neither a connection nor an error. Set _channel when the channel is created, and track separately whether it has opened. Every channel callback is now checked against this one channel. The channel-open callback no longer needs to capture its client. An EOF before on_open fails the pipe again, and also drops the channel, so that its late on_open no longer reports the pipe as connected after the error. Also ignore host key and exit callbacks from replaced clients and channels. Add tests for restart(), for channels that the server closes right after it confirms them, and for a write while the channel is still opening.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After a pipe restart callbacks from the old client can arrive after the new client has been stored. An old close callback could clear _client, leaving the new connection open with no way to close it. Old channel data could also reach the new session and corrupt its protocol stream.
So it failed like this:
Client A is active.
restart() closes A asynchronously and stores client B.
A's close callback arrives and clears the reference to B.
The next restart cannot close B because that reference is gone.
Check client and channel identity before changing pipe state or forwarding data. Capture the client in the channel-open callback as well: the new client can exist before its channel opens, so checking for an empty channel slot is not enough. Close channels opened by an old client.