Don't dispose channel when completing SshCommand (#1596)

The new(-ish) implementation of SshCommand has a race condition for short-lived
commands where SSH_MSG_CHANNEL_CLOSE may be processed on the message loop thread
before SSH_MSG_CHANNEL_SUCCESS is waited upon on the Execute (main) thread. This
manifests in an ArgumentNull/NullReference exception on the wait handle because
the channel has already been closed and disposed.

Fix this by only delaying the channel dispose until the command dispose.

Author: Rob-Hague

src/Renci.SshNet/SshCommand.cs

@@ -21,7 +21,7 @@ public class SshCommand : IDisposable
         private readonly ISession _session;
         private readonly Encoding _encoding;
 
-        private IChannelSession? _channel;
+        private IChannelSession _channel;
         private TaskCompletionSource<object>? _tcs;
         private CancellationTokenSource? _cts;
         private CancellationTokenRegistration _tokenRegistration;
@@ -142,14 +142,14 @@ public int? ExitStatus
         /// </example>
         public Stream CreateInputStream()
         {
-            if (_channel == null)
+            if (!_channel.IsOpen)
             {
-                throw new InvalidOperationException($"The input stream can be used only after calling BeginExecute and before calling EndExecute.");
+                throw new InvalidOperationException("The input stream can be used only during execution.");
             }
 
             if (_inputStream != null)
             {
-                throw new InvalidOperationException($"The input stream already exists.");
+                throw new InvalidOperationException("The input stream already exists.");
             }
 
             _inputStream = new ChannelInputStream(_channel);
@@ -226,6 +226,7 @@ internal SshCommand(ISession session, string commandText, Encoding encoding)
             ExtendedOutputStream = new PipeStream();
             _session.Disconnected += Session_Disconnected;
             _session.ErrorOccured += Session_ErrorOccured;
+            _channel = _session.CreateChannelSession();
         }
 
         /// <summary>
@@ -257,6 +258,8 @@ public Task ExecuteAsync(CancellationToken cancellationToken = default)
                     throw new InvalidOperationException("Asynchronous operation is already in progress.");
                 }
 
+                UnsubscribeFromChannelEvents(dispose: true);
+
                 OutputStream.Dispose();
                 ExtendedOutputStream.Dispose();
 
@@ -265,6 +268,7 @@ public Task ExecuteAsync(CancellationToken cancellationToken = default)
                 // so we just need to reinitialise them for subsequent executions.
                 OutputStream = new PipeStream();
                 ExtendedOutputStream = new PipeStream();
+                _channel = _session.CreateChannelSession();
             }
 
             _exitStatus = default;
@@ -282,7 +286,6 @@ public Task ExecuteAsync(CancellationToken cancellationToken = default)
             _tcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
             _userToken = cancellationToken;
 
-            _channel = _session.CreateChannelSession();
             _channel.DataReceived += Channel_DataReceived;
             _channel.ExtendedDataReceived += Channel_ExtendedDataReceived;
             _channel.RequestReceived += Channel_RequestReceived;
@@ -542,7 +545,10 @@ private void SetAsyncComplete(bool setResult = true)
                 }
             }
 
-            UnsubscribeFromEventsAndDisposeChannel();
+            // We don't dispose the channel here to avoid a race condition
+            // where SSH_MSG_CHANNEL_CLOSE arrives before _channel starts
+            // waiting for a response in _channel.SendExecRequest().
+            UnsubscribeFromChannelEvents(dispose: false);
 
             OutputStream.Dispose();
             ExtendedOutputStream.Dispose();
@@ -568,7 +574,7 @@ private void Channel_RequestReceived(object? sender, ChannelRequestEventArgs e)
 
                 Debug.Assert(!exitSignalInfo.WantReply, "exit-signal is want_reply := false by definition.");
             }
-            else if (e.Info.WantReply && _channel?.RemoteChannelNumber is uint remoteChannelNumber)
+            else if (e.Info.WantReply && sender is IChannel { RemoteChannelNumber: uint remoteChannelNumber })
             {
                 var replyMessage = new ChannelFailureMessage(remoteChannelNumber);
                 _session.SendMessage(replyMessage);
@@ -591,29 +597,24 @@ private void Channel_DataReceived(object? sender, ChannelDataEventArgs e)
         }
 
         /// <summary>
-        /// Unsubscribes the current <see cref="SshCommand"/> from channel events, and disposes
-        /// the <see cref="_channel"/>.
+        /// Unsubscribes the current <see cref="SshCommand"/> from channel events, and optionally,
+        /// disposes <see cref="_channel"/>.
         /// </summary>
-        private void UnsubscribeFromEventsAndDisposeChannel()
+        private void UnsubscribeFromChannelEvents(bool dispose)
         {
             var channel = _channel;
 
-            if (channel is null)
-            {
-                return;
-            }
-
-            _channel = null;
-
             // unsubscribe from events as we do not want to be signaled should these get fired
             // during the dispose of the channel
             channel.DataReceived -= Channel_DataReceived;
             channel.ExtendedDataReceived -= Channel_ExtendedDataReceived;
             channel.RequestReceived -= Channel_RequestReceived;
             channel.Closed -= Channel_Closed;
 
-            // actually dispose the channel
-            channel.Dispose();
+            if (dispose)
+            {
+                channel.Dispose();
+            }
         }
 
         /// <summary>
@@ -645,7 +646,7 @@ protected virtual void Dispose(bool disposing)
 
                 // unsubscribe from channel events to ensure other objects that we're going to dispose
                 // are not accessed while disposing
-                UnsubscribeFromEventsAndDisposeChannel();
+                UnsubscribeFromChannelEvents(dispose: true);
 
                 _inputStream?.Dispose();
                 _inputStream = null;

test/Renci.SshNet.Tests/Classes/SshCommandTest_EndExecute.cs

@@ -30,7 +30,7 @@ protected override void OnInit()
 
         private void Arrange()
         {
-            _sessionMock = new Mock<ISession>(MockBehavior.Strict);
+            _sessionMock = new Mock<ISession>();
             _commandText = new Random().Next().ToString(CultureInfo.InvariantCulture);
             _encoding = Encoding.UTF8;
             _asyncResult = null;

test/Renci.SshNet.Tests/Classes/SshCommandTest_EndExecute_AsyncResultIsNull.cs

@@ -81,12 +81,6 @@ private void Act()
             _actual = _sshCommand.EndExecute(_asyncResult);
         }
 
-        [TestMethod]
-        public void ChannelSessionShouldBeDisposedOnce()
-        {
-            _channelSessionMock.Verify(p => p.Dispose(), Times.Once);
-        }
-
         [TestMethod]
         public void EndExecuteShouldReturnAllDataReceivedInSpecifiedEncoding()
         {