Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #805: Prevent CHANNEL_CLOSE to be sent between Channel.isOpen and… #813

Merged
merged 2 commits into from
Sep 17, 2022

Conversation

kegelh
Copy link
Contributor

@kegelh kegelh commented Sep 15, 2022

… a Transport.write call

Otherwise, a disconnect with a "packet referred to nonexistent channel" message can occur.

This particularly happens when the transport.Reader thread passes an eof from the server to the ChannelInputStream, the reading library-user thread returns, and closes the channel at the same time as the transport.Reader thread receives the subsequent CHANNEL_CLOSE from the server.

The issue was reproducible and debug output showed that after a CHANNEL_CLOSE from the server, sshj sporadically sent the CHANNEL_CLOSE from a Channel.close call first and afterwards the CHANNEL_EOF from AbstractChannel.gotClose.

This commit ensures,that the CHANNEL_EOF is not sent anymore if Channel.close sent out CHANNEL_CLOSE first.

The Transport.write call in DataBuffer.flush could produce a similar issue with a data packet, so it was placed within the openCloseLock, too.

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2022

Codecov Report

Merging #813 (f9b0a60) into master (2551f8e) will decrease coverage by 0.08%.
The diff coverage is 73.68%.

@@             Coverage Diff              @@
##             master     #813      +/-   ##
============================================
- Coverage     64.84%   64.75%   -0.09%     
+ Complexity     1466     1463       -3     
============================================
  Files           210      210              
  Lines          8524     8537      +13     
  Branches        779      781       +2     
============================================
+ Hits           5527     5528       +1     
- Misses         2592     2600       +8     
- Partials        405      409       +4     
Impacted Files Coverage Δ
...z/sshj/connection/channel/ChannelOutputStream.java 76.19% <61.53%> (-1.74%) ⬇️
...hmizz/sshj/connection/channel/AbstractChannel.java 74.69% <100.00%> (+0.94%) ⬆️
...net/schmizz/sshj/transport/TransportException.java 33.33% <0.00%> (-22.23%) ⬇️
...ain/java/net/schmizz/sshj/common/SSHException.java 61.29% <0.00%> (-12.91%) ⬇️
...va/net/schmizz/sshj/connection/ConnectionImpl.java 59.34% <0.00%> (-0.82%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kegelh kegelh force-pushed the eof_issue branch 2 times, most recently from a2a6388 to ff30c9b Compare September 16, 2022 13:08
…isOpen and a Transport.write call

Otherwise, a disconnect with a "packet referred to nonexistent channel" message can occur.

This particularly happens when the transport.Reader thread passes an eof from the server to the ChannelInputStream, the reading library-user thread returns, and closes the channel at the same time as the transport.Reader thread receives the subsequent CHANNEL_CLOSE from the server.
@hierynomus
Copy link
Owner

Thanks for the find on how this happened!

@hierynomus hierynomus merged commit d5d6096 into hierynomus:master Sep 17, 2022
@kegelh kegelh deleted the eof_issue branch September 19, 2022 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants