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.

Sign up for GitHub

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

Jump to bottom

Fixes for ShellStream.Expect, ShellStream.Read, ShellStream.Write, and NullReferenceExceptions. #458

Closed

keithallenjackson wants to merge 5 commits into sshnet:develop from keithallenjackson:Fixes

keithallenjackson commented Aug 12, 2018 •

edited

Adds tests (reproducing the issues) and code fixes for issues #454, #453, #432, #424, #180, #444, #301, #302.

Keith Jackson added 2 commits

August 12, 2018 15:39


          Added test for ShellStream.Expect(Regex, TimeSpan) that reproduces is…

041806a

…sue sshnet#454


          Fixed issue sshnet#454, ShellStream.Expect(Regex, TimeSpan) returning…

39ba6bc

… "too much" string.

keithallenjackson changed the title ~~Fixes~~ Fix for ShellStream.Expect(Regex, TimeSpan) returning string past the matched buffer.


          Addresses issue sshnet#453.

e3540cb

+ ShellStream.Expect no longer throws NullReferenceException when channel unexpectedly disconnects.
+ Adds Disconnect event to ShellStream.
+ Adds Disposed property to ShellStream to read internal disposed state.

keithallenjackson changed the title ~~Fix for ShellStream.Expect(Regex, TimeSpan) returning string past the matched buffer.~~ Fixes for ShellStream.Expect and NullReferenceExceptions.

This was referenced Aug 18, 2018

add disconnect and close events #432

Closed

We has can public _isDisposed? #424

Closed

VAllens commented Aug 18, 2018

This is great, thank you.


          Fix for issues sshnet#180 sshnet#444 sshnet#301 sshnet#302

2830ba9

keithallenjackson force-pushed the Fixes branch from b216f41 to 2830ba9 Compare

August 30, 2018 06:44

keithallenjackson changed the title ~~Fixes for ShellStream.Expect and NullReferenceExceptions.~~ Fixes for ShellStream.Expect, ShellStream.Read, ShelStream.Write, and NullReferenceExceptions.

keithallenjackson changed the title ~~Fixes for ShellStream.Expect, ShellStream.Read, ShelStream.Write, and NullReferenceExceptions.~~ Fixes for ShellStream.Expect, ShellStream.Read, ShellStream.Write, and NullReferenceExceptions.

drieseng requested changes

View reviewed changes

src/Renci.SshNet/ShellStream.cs Outdated Show resolved Hide resolved

src/Renci.SshNet/ShellStream.cs

+                      /// <summary>
+                      /// Whether the instance is disposed.
+                      /// </summary>
+                      public bool Disposed

Member

drieseng Aug 30, 2018

Do not expose this.

Author

keithallenjackson Aug 31, 2018

While I'm not a fan of exposing the "Disposed" state of an object (the User API should have complete control of that), this was added to check for a disposed state since an unexpected channel closure could cause the ShellStream to Dispose without explicit user API calls. Should I change the event handler for _channel disconect to not Dispose the ShellStream? Essentially, an abrupt channel closure puts the ShellStream in a 'half-used'/'half-disposed' state. Let me know your thoughts.

src/Renci.SshNet/ShellStream.cs Outdated Show resolved Hide resolved

src/Renci.SshNet/ShellStream.cs Outdated Show resolved Hide resolved

src/Renci.SshNet/ShellStream.cs Outdated

@@ @@ -550,6 +575,9 @@ public string Expect(Regex regex, TimeSpan timeout) @@
                       {
                           var text = string.Empty;
+                          //  Flush the write buffer in case expecting output from previous write.
+                          if (!_isDisposed) Flush();

Member

drieseng Aug 30, 2018

Not something to change as part of this PR, but shouldn't we throw an ObjectDisposedException here (and consider the same for a few other methods) when the channel is closed or the ShellStream is disposed?

Author

keithallenjackson Aug 31, 2018

This goes back to the "half-used"/"half-Disposed" issue when an abrupt channel closure causes the ShellStream to abruptly be in a Disposed state without explicit user interaction. Under normal API usage, I agree 100% - Disposed state should be private, calling methods on a Disposed object should throw an ObjectDisposedException, and all of this behavior is appropriate because it is up to the user to set the 'disposed' state alone. Essentially, the user caused the Disposal, so therefore should know better than to use an object he/she disposed.

In this object, that behavior is not present. Maybe I should make a new private variable that specifies if the ShellStream is in a 'half-open' state? If so, how should the Write methods behave under this state? I'm genuinely curious about your thoughts. Thanks!

src/Renci.SshNet/ShellStream.cs Outdated Show resolved Hide resolved

src/Renci.SshNet/ShellStream.cs Outdated Show resolved Hide resolved


          Changes requested for PL sshnet#458. All except Disposed issued (stil…

eb248d2

…l talking it through)

keithallenjackson commented

View reviewed changes

Author

keithallenjackson left a comment

All that's left to discuss is the behavior of Dispose and abrupt channel closure.

Author

keithallenjackson commented Aug 31, 2018

How about this:

States

Normal, in use: Performs normal operations with standard Timeouts (as already written)
Channel Closed, Not Disposed: Throws either an SshConnectionException or ChannelClosedException when attempting to use Read, Write, or Expect
Disposed: Throws ObjectDisposedException

Additions

Closed Event: Only occurs when the Channel closes and the instance is NOT Disposed/Disposing
Closed EventArgs: Has properties containing the remaining input and output buffers. That way if/when an abrupt channel closure occurs, the User may see what was not sent and any remaining data within the input.

keithallenjackson mentioned this pull request

Real time command output #464

Open

Member

drieseng commented Sep 3, 2018

Quick and incomplete feedback:

We should not dispose the ShellStream when the channel is closed.
When disposed, we should throw an ObjectDisposedException in all methods (except for Dispose() of course).
When the channel is closed, we should throw an exception in the Write methods. The Read and Expect methods should return / operate on buffered data, and only throw an exception when the channel is closed and the buffer is empty at the time of the invocation.

lipmas added a commit to bastionzero/SSH.NET that referenced this pull request


          Emit ErrorOccurred events when on ShellStream channel close/session d…

8fa9b69

…isconnected

Temporary fix until this is fixed upstream:

sshnet#424 (comment)
sshnet#458

Author

keithallenjackson commented Oct 2, 2021

Got busy with work and completely forgot about this. This is so out of date, there is no reason to keep it around. I'm closing.

keithallenjackson closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment