Fix hang in SftpClient.UploadFile upon error (#1643)

* Fix deadlock in SftpClient.UploadFile upon error

* Make RequestWrite deterministic wrt. exception handling

* add regression test; fix race

* x

Author: Rob-Hague

src/Renci.SshNet/Sftp/Responses/SftpStatusResponse.cs

@@ -12,7 +12,7 @@ public SftpStatusResponse(uint protocolVersion)
         {
         }
 
-        public StatusCodes StatusCode { get; private set; }
+        public StatusCodes StatusCode { get; set; }
 
         public string ErrorMessage { get; private set; }
 
@@ -39,5 +39,12 @@ protected override void LoadData()
                 Language = ReadString(Ascii);
             }
         }
+
+        protected override void SaveData()
+        {
+            base.SaveData();
+
+            Write((uint)StatusCode);
+        }
     }
 }

src/Renci.SshNet/Sftp/SftpSession.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Globalization;
 using System.Text;
 using System.Threading;
@@ -914,6 +915,8 @@ public void RequestWrite(byte[] handle,
                                  AutoResetEvent wait,
                                  Action<SftpStatusResponse> writeCompleted = null)
         {
+            Debug.Assert((wait is null) != (writeCompleted is null), "Should have one parameter or the other.");
+
             SshException exception = null;
 
             var request = new SftpWriteRequest(ProtocolVersion,
@@ -925,22 +928,27 @@ public void RequestWrite(byte[] handle,
                                                length,
                                                response =>
                                                {
-                                                   writeCompleted?.Invoke(response);
-
-                                                   exception = GetSftpException(response);
-                                                   wait?.SetIgnoringObjectDisposed();
+                                                   if (writeCompleted is not null)
+                                                   {
+                                                       writeCompleted.Invoke(response);
+                                                   }
+                                                   else
+                                                   {
+                                                       exception = GetSftpException(response);
+                                                       wait.SetIgnoringObjectDisposed();
+                                                   }
                                                });
 
             SendRequest(request);
 
             if (wait is not null)
             {
                 WaitOnHandle(wait, OperationTimeout);
-            }
 
-            if (exception is not null)
-            {
-                throw exception;
+                if (exception is not null)
+                {
+                    throw exception;
+                }
             }
         }
 
@@ -2272,7 +2280,7 @@ public uint CalculateOptimalWriteLength(uint bufferSize, byte[] handle)
             return Math.Min(bufferSize, maximumPacketSize) - lengthOfNonDataProtocolFields;
         }
 
-        private static SshException GetSftpException(SftpStatusResponse response)
+        internal static SshException GetSftpException(SftpStatusResponse response)
         {
 #pragma warning disable IDE0010 // Add missing cases
             switch (response.StatusCode)

src/Renci.SshNet/SftpClient.cs

@@ -1,11 +1,13 @@
 #nullable enable
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using System.IO;
 using System.Net;
 using System.Runtime.CompilerServices;
+using System.Runtime.ExceptionServices;
 using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
@@ -2456,56 +2458,82 @@ private void InternalUploadFile(Stream input, string path, Flags flags, SftpUplo
             // create buffer of optimal length
             var buffer = new byte[_sftpSession.CalculateOptimalWriteLength(_bufferSize, handle)];
 
-            var bytesRead = input.Read(buffer, 0, buffer.Length);
+            int bytesRead;
             var expectedResponses = 0;
-            var responseReceivedWaitHandle = new AutoResetEvent(initialState: false);
 
-            do
+            // We will send out all the write requests without waiting for each response.
+            // Afterwards, we may wait on this handle until all responses are received
+            // or an error has occured.
+            using var mres = new ManualResetEventSlim(initialState: false);
+
+            ExceptionDispatchInfo? exception = null;
+
+            while ((bytesRead = input.Read(buffer, 0, buffer.Length)) != 0)
             {
-                // Cancel upload
                 if (asyncResult is not null && asyncResult.IsUploadCanceled)
                 {
                     break;
                 }
 
-                if (bytesRead > 0)
+                exception?.Throw();
+
+                var writtenBytes = offset + (ulong)bytesRead;
+
+                _ = Interlocked.Increment(ref expectedResponses);
+                mres.Reset();
+
+                _sftpSession.RequestWrite(handle, offset, buffer, offset: 0, bytesRead, wait: null, s =>
                 {
-                    var writtenBytes = offset + (ulong)bytesRead;
+                    var setHandle = false;
+
+                    try
+                    {
+                        if (Sftp.SftpSession.GetSftpException(s) is Exception ex)
+                        {
+                            exception = ExceptionDispatchInfo.Capture(ex);
+                        }
 
-                    _sftpSession.RequestWrite(handle, offset, buffer, offset: 0, bytesRead, wait: null, s =>
+                        if (exception is not null)
                         {
-                            if (s.StatusCode == StatusCodes.Ok)
-                            {
-                                _ = Interlocked.Decrement(ref expectedResponses);
-                                _ = responseReceivedWaitHandle.Set();
+                            setHandle = true;
+                            return;
+                        }
 
-                                asyncResult?.Update(writtenBytes);
+                        Debug.Assert(s.StatusCode == StatusCodes.Ok);
 
-                                // Call callback to report number of bytes written
-                                if (uploadCallback is not null)
-                                {
-                                    // Execute callback on different thread
-                                    ThreadAbstraction.ExecuteThread(() => uploadCallback(writtenBytes));
-                                }
-                            }
-                        });
+                        asyncResult?.Update(writtenBytes);
+
+                        // Call callback to report number of bytes written
+                        if (uploadCallback is not null)
+                        {
+                            // Execute callback on different thread
+                            ThreadAbstraction.ExecuteThread(() => uploadCallback(writtenBytes));
+                        }
+                    }
+                    finally
+                    {
+                        if (Interlocked.Decrement(ref expectedResponses) == 0 || setHandle)
+                        {
+                            mres.Set();
+                        }
+                    }
+                });
 
-                    _ = Interlocked.Increment(ref expectedResponses);
+                offset += (ulong)bytesRead;
+            }
 
-                    offset += (ulong)bytesRead;
+            // Make sure the read of exception cannot be executed ahead of
+            // the read of expectedResponses so that we do not miss an
+            // exception.
 
-                    bytesRead = input.Read(buffer, 0, buffer.Length);
-                }
-                else if (expectedResponses > 0)
-                {
-                    // Wait for expectedResponses to change
-                    _sftpSession.WaitOnHandle(responseReceivedWaitHandle, _operationTimeout);
-                }
+            if (Volatile.Read(ref expectedResponses) != 0)
+            {
+                _sftpSession.WaitOnHandle(mres.WaitHandle, _operationTimeout);
             }
-            while (expectedResponses > 0 || bytesRead > 0);
+
+            exception?.Throw();
 
             _sftpSession.RequestClose(handle);
-            responseReceivedWaitHandle.Dispose();
         }
 
         private async Task InternalUploadFileAsync(Stream input, string path, CancellationToken cancellationToken)