Fail SCP download when the channel closes mid-file

When the channel was closed before all of the file content announced in a
C record had been received, PipeStream.Read kept returning 0 and the copy
loop spun forever. Throw an SshException in this case, matching ReadByte.

src/Renci.SshNet/ScpClient.cs

@@ -862,6 +862,11 @@ namespace Renci.SshNet
             {
                 var read = input.Read(buffer, 0, (int)Math.Min(needToRead, BufferSize));
 
+                if (read == 0 && needToRead > 0)
+                {
+                    throw new SshException("Stream has been closed.");
+                }
+
                 output.Write(buffer, 0, read);
 
                 RaiseDownloadingEvent(filename, length, length - needToRead);

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

@@ -0,0 +1,76 @@
+using System;
+using System.IO;
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+using Renci.SshNet.Common;
+using Renci.SshNet.Tests.Common;
+
+namespace Renci.SshNet.Tests.Classes
+{
+    /// <summary>
+    /// Verifies that a download fails, rather than spinning forever, when the channel is closed
+    /// before all of the announced file content has been received.
+    /// </summary>
+    [TestClass]
+    public class ScpClientTest_Download_ChannelClosedDuringFileContent : ScpClientTranscriptTestBase
+    {
+        [TestMethod]
+        public void DownloadDirectoryInfo_ShouldThrow()
+        {
+            var transcript = new ScpServerTranscript()
+                .Line("D0755 0 remote")
+                .Line("C0644 100 file")
+                .Raw("0123456789");
+
+            AssertThrows(transcript, client => client.Download("/remote", new DirectoryInfo(Destination)));
+        }
+
+        [TestMethod]
+        public void DownloadFileInfo_ShouldThrow()
+        {
+            var transcript = new ScpServerTranscript()
+                .Line("C0644 100 file")
+                .Raw("0123456789");
+
+            AssertThrows(transcript, client => client.Download("/remote/file", new FileInfo(Path.Combine(Destination, "file"))));
+        }
+
+        [TestMethod]
+        public void DownloadStream_ShouldThrow()
+        {
+            var transcript = new ScpServerTranscript()
+                .Line("C0644 100 file")
+                .Raw("0123456789");
+
+            using (var destination = new MemoryStream())
+            {
+                AssertThrows(transcript, client => client.Download("/remote/file", destination));
+            }
+        }
+
+        [TestMethod]
+        public void DownloadEmptyFile_ShouldStillSucceed()
+        {
+            var transcript = new ScpServerTranscript()
+                .File("file", string.Empty);
+
+            using (var destination = new MemoryStream())
+            {
+                var result = transcript.Run(client => client.Download("/remote/file", destination), closeChannel: true);
+
+                Assert.IsTrue(result.Completed);
+                Assert.IsNull(result.Exception, result.Exception?.ToString());
+                Assert.AreEqual(0, destination.Length);
+            }
+        }
+
+        private static void AssertThrows(ScpServerTranscript transcript, Action<ScpClient> download)
+        {
+            var result = transcript.Run(download, closeChannel: true, timeout: TimeSpan.FromSeconds(5));
+
+            Assert.IsTrue(result.Completed);
+            Assert.IsInstanceOfType<SshException>(result.Exception);
+        }
+    }
+}

test/Renci.SshNet.Tests/Common/ScpClientTranscriptTestBase.cs

@@ -0,0 +1,124 @@
+using System;
+using System.IO;
+using System.Linq;
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace Renci.SshNet.Tests.Common
+{
+    /// <summary>
+    /// Base class for tests that replay a scripted server transcript (see <see cref="ScpServerTranscript"/>)
+    /// into a download, with an isolated destination directory.
+    /// </summary>
+    /// <remarks>
+    /// The destination is <c>&lt;root&gt;/a/b/dest</c> below a unique temporary root, so that anything
+    /// written above it, into <c>a/b</c> or <c>a</c>, is detected by <see cref="AssertNothingOutsideDestination"/>.
+    /// </remarks>
+    public abstract class ScpClientTranscriptTestBase
+    {
+        /// <summary>
+        /// Gets the temporary root directory, which is deleted after each test.
+        /// </summary>
+        protected string Root { get; private set; }
+
+        /// <summary>
+        /// Gets the destination directory, <c>&lt;root&gt;/a/b/dest</c>.
+        /// </summary>
+        protected string Destination { get; private set; }
+
+        /// <summary>
+        /// Gets the parent of the destination directory, <c>&lt;root&gt;/a/b</c>.
+        /// </summary>
+        protected string DestinationParent
+        {
+            get
+            {
+                return Path.GetDirectoryName(Destination);
+            }
+        }
+
+        [TestInitialize]
+        public void CreateDestination()
+        {
+            Root = Path.Combine(Path.GetTempPath(), "sshnet-scp-" + Guid.NewGuid().ToString("N"));
+            Destination = Directory.CreateDirectory(Path.Combine(Root, "a", "b", "dest")).FullName;
+        }
+
+        [TestCleanup]
+        public void DeleteRoot()
+        {
+            if (Root != null && Directory.Exists(Root))
+            {
+                Directory.Delete(LiteralPath(Root), recursive: true);
+            }
+        }
+
+        /// <summary>
+        /// Returns the same path with the <c>\\?\</c> prefix on Windows, which turns off Win32 path
+        /// normalization, so that entries with names such as <c>file.</c> or <c>CON</c> (which some
+        /// tests create on purpose) can be read and deleted.
+        /// </summary>
+        protected static string LiteralPath(string path)
+        {
+            return Path.DirectorySeparatorChar == '\\' ? @"\\?\" + path : path;
+        }
+
+        /// <summary>
+        /// Lists every file system entry below <paramref name="directory"/>, as paths relative to it using
+        /// '/' as separator, in no particular order.
+        /// </summary>
+        protected static string[] ListEntries(string directory)
+        {
+            return Directory.GetFileSystemEntries(directory, "*", SearchOption.AllDirectories)
+                            .Select(p => p.Substring(directory.Length + 1).Replace(Path.DirectorySeparatorChar, '/'))
+                            .ToArray();
+        }
+
+        /// <summary>
+        /// Runs a recursive download of <paramref name="transcript"/> into the destination directory, or
+        /// into <paramref name="destinationPath"/> if specified (for example, the destination spelled
+        /// differently).
+        /// </summary>
+        protected ScpTranscriptResult DownloadDirectory(ScpServerTranscript transcript, string destinationPath = null)
+        {
+            return transcript.Run(
+                client => client.Download("/remote", new DirectoryInfo(destinationPath ?? Destination)),
+                closeChannel: true);
+        }
+
+        /// <summary>
+        /// Asserts that nothing exists below the root other than the directories leading to the
+        /// destination, the contents of the destination, and <paramref name="expectedOutside"/> (a path
+        /// relative to the root, using '/' as separator), if specified.
+        /// </summary>
+        /// <param name="expectedOutside">An entry outside of the destination that the test created itself.</param>
+        /// <param name="context">A description of the scenario, included in the failure message.</param>
+        protected void AssertNothingOutsideDestination(string expectedOutside = null, string context = null)
+        {
+            var outside = ListEntries(Root).Where(p => p is not "a" and not "a/b" and not "a/b/dest" &&
+                                                      !p.StartsWith("a/b/dest/", StringComparison.Ordinal) &&
+                                                      !string.Equals(p, expectedOutside, StringComparison.Ordinal))
+                                           .ToArray();
+
+            Assert.IsEmpty(outside, context + " Written outside of the destination: " + string.Join(", ", outside));
+        }
+
+        /// <summary>
+        /// Deletes everything in the destination directory.
+        /// </summary>
+        protected void ClearDestination()
+        {
+            foreach (var entry in Directory.GetFileSystemEntries(LiteralPath(Destination)))
+            {
+                if (Directory.Exists(entry))
+                {
+                    Directory.Delete(entry, recursive: true);
+                }
+                else
+                {
+                    File.Delete(entry);
+                }
+            }
+        }
+    }
+}

test/Renci.SshNet.Tests/Common/ScpServerTranscript.cs

@@ -0,0 +1,129 @@
+using System;
+using System.Collections.Generic;
+using System.Globalization;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+
+using Microsoft.Extensions.Logging.Abstractions;
+
+using Moq;
+
+using Renci.SshNet.Channels;
+using Renci.SshNet.Common;
+using Renci.SshNet.Connection;
+
+namespace Renci.SshNet.Tests.Common
+{
+    /// <summary>
+    /// Builds the byte stream an SCP server sends to the client and replays it into a real
+    /// <see cref="ScpClient"/> through mocked session and channel objects.
+    /// </summary>
+    public sealed class ScpServerTranscript
+    {
+        private readonly List<byte> _bytes = new List<byte>();
+
+        /// <summary>
+        /// Appends a control line, terminated by a line feed.
+        /// </summary>
+        public ScpServerTranscript Line(string line)
+        {
+            return Raw(line + "\n");
+        }
+
+        /// <summary>
+        /// Appends <paramref name="data"/> encoded as UTF-8, which is the default
+        /// <see cref="ConnectionInfo.Encoding"/>.
+        /// </summary>
+        public ScpServerTranscript Raw(string data)
+        {
+            return Raw(Encoding.UTF8.GetBytes(data));
+        }
+
+        public ScpServerTranscript Raw(byte[] data)
+        {
+            _bytes.AddRange(data);
+            return this;
+        }
+
+        /// <summary>
+        /// Appends a C (file) record, the file content, and the status byte that follows it.
+        /// </summary>
+        public ScpServerTranscript File(string name, string content)
+        {
+            var bytes = Encoding.UTF8.GetBytes(content);
+            var length = bytes.Length.ToString(CultureInfo.InvariantCulture);
+
+            return Line($"C0644 {length} {name}").Raw(bytes).Raw([0]);
+        }
+
+        /// <summary>
+        /// Replays the transcript into a connected <see cref="ScpClient"/> and runs
+        /// <paramref name="operation"/> on it.
+        /// </summary>
+        /// <param name="operation">The client operation to run.</param>
+        /// <param name="closeChannel">
+        /// <see langword="true"/> to signal end of stream after the transcript, as when the
+        /// server closes the channel; otherwise a read past the end blocks.
+        /// </param>
+        /// <param name="timeout">How long to wait for <paramref name="operation"/> to finish.</param>
+        public ScpTranscriptResult Run(Action<ScpClient> operation, bool closeChannel = false, TimeSpan? timeout = null)
+        {
+            var connectionInfo = new ConnectionInfo("host", 22, "user", new PasswordAuthenticationMethod("user", "pwd"));
+            var serviceFactoryMock = new Mock<IServiceFactory>(MockBehavior.Strict);
+            var socketFactoryMock = new Mock<ISocketFactory>(MockBehavior.Strict);
+            var sessionMock = new Mock<ISession>(MockBehavior.Strict);
+            var channelMock = new Mock<IChannelSession>(MockBehavior.Strict);
+            var remotePathTransformationMock = new Mock<IRemotePathTransformation>(MockBehavior.Strict);
+            var sentData = new List<byte[]>();
+            var execRequests = new List<string>();
+
+            var pipeStream = new PipeStream();
+            pipeStream.Write(_bytes.ToArray(), 0, _bytes.Count);
+            if (closeChannel)
+            {
+                pipeStream.Dispose();
+            }
+
+            _ = sessionMock.Setup(p => p.SessionLoggerFactory).Returns(NullLoggerFactory.Instance);
+            _ = serviceFactoryMock.Setup(p => p.CreateSocketFactory()).Returns(socketFactoryMock.Object);
+            _ = serviceFactoryMock.Setup(p => p.CreateSession(connectionInfo, socketFactoryMock.Object)).Returns(sessionMock.Object);
+            _ = sessionMock.Setup(p => p.Connect());
+            _ = serviceFactoryMock.Setup(p => p.CreatePipeStream()).Returns(pipeStream);
+            _ = sessionMock.Setup(p => p.CreateChannelSession()).Returns(channelMock.Object);
+            _ = channelMock.Setup(p => p.Open());
+            _ = remotePathTransformationMock.Setup(p => p.Transform(It.IsAny<string>())).Returns<string>(path => path);
+            _ = channelMock.Setup(p => p.SendExecRequest(It.IsAny<string>()))
+                           .Callback<string>(execRequests.Add)
+                           .Returns(true);
+            _ = channelMock.Setup(p => p.SendData(It.IsAny<byte[]>())).Callback<byte[]>(sentData.Add);
+            _ = channelMock.Setup(p => p.SendData(It.IsAny<byte[]>(), It.IsAny<int>(), It.IsAny<int>()))
+                           .Callback<byte[], int, int>((data, offset, count) => sentData.Add(data.Skip(offset).Take(count).ToArray()));
+            _ = channelMock.Setup(p => p.Dispose());
+
+            var scpClient = new ScpClient(connectionInfo, false, serviceFactoryMock.Object, remotePathTransformationMock.Object);
+            scpClient.Connect();
+
+            Exception exception = null;
+            var task = Task.Run(() =>
+            {
+                try
+                {
+                    operation(scpClient);
+                }
+                catch (Exception ex)
+                {
+                    exception = ex;
+                }
+            });
+
+            var completed = task.Wait(timeout ?? TimeSpan.FromSeconds(10));
+
+            pipeStream.Dispose();
+
+            _ = task.Wait(TimeSpan.FromSeconds(10));
+
+            return new ScpTranscriptResult(completed, exception, sentData, execRequests);
+        }
+    }
+}

test/Renci.SshNet.Tests/Common/ScpTranscriptResult.cs

@@ -0,0 +1,39 @@
+using System;
+using System.Collections.Generic;
+
+namespace Renci.SshNet.Tests.Common
+{
+    /// <summary>
+    /// The outcome of <see cref="ScpServerTranscript.Run"/>.
+    /// </summary>
+    public sealed class ScpTranscriptResult
+    {
+        public ScpTranscriptResult(bool completed, Exception exception, IReadOnlyList<byte[]> sentData, IReadOnlyList<string> execRequests)
+        {
+            Completed = completed;
+            Exception = exception;
+            SentData = sentData;
+            ExecRequests = execRequests;
+        }
+
+        /// <summary>
+        /// Gets a value indicating whether the operation finished within the timeout.
+        /// </summary>
+        public bool Completed { get; }
+
+        /// <summary>
+        /// Gets the exception thrown by the operation, if any.
+        /// </summary>
+        public Exception Exception { get; }
+
+        /// <summary>
+        /// Gets the data the client sent on the channel, one entry per send.
+        /// </summary>
+        public IReadOnlyList<byte[]> SentData { get; }
+
+        /// <summary>
+        /// Gets the commands the client sent in exec requests.
+        /// </summary>
+        public IReadOnlyList<string> ExecRequests { get; }
+    }
+}