From 5ef2f6ce69b67fd8cc298c8681cb5be506ee06f6 Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Thu, 8 Oct 2026 08:45:39 +0000 Subject: [PATCH] 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 | 5 + ...Download_ChannelClosedDuringFileContent.cs | 76 +++++++++++ .../Common/ScpClientTranscriptTestBase.cs | 124 +++++++++++++++++ .../Common/ScpServerTranscript.cs | 129 ++++++++++++++++++ .../Common/ScpTranscriptResult.cs | 39 ++++++ 5 files changed, 373 insertions(+) create mode 100644 test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_ChannelClosedDuringFileContent.cs create mode 100644 test/Renci.SshNet.Tests/Common/ScpClientTranscriptTestBase.cs create mode 100644 test/Renci.SshNet.Tests/Common/ScpServerTranscript.cs create mode 100644 test/Renci.SshNet.Tests/Common/ScpTranscriptResult.cs diff --git a/src/Renci.SshNet/ScpClient.cs b/src/Renci.SshNet/ScpClient.cs index 275e000a..20704502 100644 --- a/src/Renci.SshNet/ScpClient.cs +++ b/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); diff --git a/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_ChannelClosedDuringFileContent.cs b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_ChannelClosedDuringFileContent.cs new file mode 100644 index 00000000..d8453310 --- /dev/null +++ b/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 +{ + /// + /// Verifies that a download fails, rather than spinning forever, when the channel is closed + /// before all of the announced file content has been received. + /// + [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 download) + { + var result = transcript.Run(download, closeChannel: true, timeout: TimeSpan.FromSeconds(5)); + + Assert.IsTrue(result.Completed); + Assert.IsInstanceOfType(result.Exception); + } + } +} diff --git a/test/Renci.SshNet.Tests/Common/ScpClientTranscriptTestBase.cs b/test/Renci.SshNet.Tests/Common/ScpClientTranscriptTestBase.cs new file mode 100644 index 00000000..bcf83de9 --- /dev/null +++ b/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 +{ + /// + /// Base class for tests that replay a scripted server transcript (see ) + /// into a download, with an isolated destination directory. + /// + /// + /// The destination is <root>/a/b/dest below a unique temporary root, so that anything + /// written above it, into a/b or a, is detected by . + /// + public abstract class ScpClientTranscriptTestBase + { + /// + /// Gets the temporary root directory, which is deleted after each test. + /// + protected string Root { get; private set; } + + /// + /// Gets the destination directory, <root>/a/b/dest. + /// + protected string Destination { get; private set; } + + /// + /// Gets the parent of the destination directory, <root>/a/b. + /// + 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); + } + } + + /// + /// Returns the same path with the \\?\ prefix on Windows, which turns off Win32 path + /// normalization, so that entries with names such as file. or CON (which some + /// tests create on purpose) can be read and deleted. + /// + protected static string LiteralPath(string path) + { + return Path.DirectorySeparatorChar == '\\' ? @"\\?\" + path : path; + } + + /// + /// Lists every file system entry below , as paths relative to it using + /// '/' as separator, in no particular order. + /// + protected static string[] ListEntries(string directory) + { + return Directory.GetFileSystemEntries(directory, "*", SearchOption.AllDirectories) + .Select(p => p.Substring(directory.Length + 1).Replace(Path.DirectorySeparatorChar, '/')) + .ToArray(); + } + + /// + /// Runs a recursive download of into the destination directory, or + /// into if specified (for example, the destination spelled + /// differently). + /// + protected ScpTranscriptResult DownloadDirectory(ScpServerTranscript transcript, string destinationPath = null) + { + return transcript.Run( + client => client.Download("/remote", new DirectoryInfo(destinationPath ?? Destination)), + closeChannel: true); + } + + /// + /// Asserts that nothing exists below the root other than the directories leading to the + /// destination, the contents of the destination, and (a path + /// relative to the root, using '/' as separator), if specified. + /// + /// An entry outside of the destination that the test created itself. + /// A description of the scenario, included in the failure message. + 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)); + } + + /// + /// Deletes everything in the destination directory. + /// + protected void ClearDestination() + { + foreach (var entry in Directory.GetFileSystemEntries(LiteralPath(Destination))) + { + if (Directory.Exists(entry)) + { + Directory.Delete(entry, recursive: true); + } + else + { + File.Delete(entry); + } + } + } + } +} diff --git a/test/Renci.SshNet.Tests/Common/ScpServerTranscript.cs b/test/Renci.SshNet.Tests/Common/ScpServerTranscript.cs new file mode 100644 index 00000000..3fdd4498 --- /dev/null +++ b/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 +{ + /// + /// Builds the byte stream an SCP server sends to the client and replays it into a real + /// through mocked session and channel objects. + /// + public sealed class ScpServerTranscript + { + private readonly List _bytes = new List(); + + /// + /// Appends a control line, terminated by a line feed. + /// + public ScpServerTranscript Line(string line) + { + return Raw(line + "\n"); + } + + /// + /// Appends encoded as UTF-8, which is the default + /// . + /// + public ScpServerTranscript Raw(string data) + { + return Raw(Encoding.UTF8.GetBytes(data)); + } + + public ScpServerTranscript Raw(byte[] data) + { + _bytes.AddRange(data); + return this; + } + + /// + /// Appends a C (file) record, the file content, and the status byte that follows it. + /// + 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]); + } + + /// + /// Replays the transcript into a connected and runs + /// on it. + /// + /// The client operation to run. + /// + /// to signal end of stream after the transcript, as when the + /// server closes the channel; otherwise a read past the end blocks. + /// + /// How long to wait for to finish. + public ScpTranscriptResult Run(Action operation, bool closeChannel = false, TimeSpan? timeout = null) + { + var connectionInfo = new ConnectionInfo("host", 22, "user", new PasswordAuthenticationMethod("user", "pwd")); + var serviceFactoryMock = new Mock(MockBehavior.Strict); + var socketFactoryMock = new Mock(MockBehavior.Strict); + var sessionMock = new Mock(MockBehavior.Strict); + var channelMock = new Mock(MockBehavior.Strict); + var remotePathTransformationMock = new Mock(MockBehavior.Strict); + var sentData = new List(); + var execRequests = new List(); + + 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())).Returns(path => path); + _ = channelMock.Setup(p => p.SendExecRequest(It.IsAny())) + .Callback(execRequests.Add) + .Returns(true); + _ = channelMock.Setup(p => p.SendData(It.IsAny())).Callback(sentData.Add); + _ = channelMock.Setup(p => p.SendData(It.IsAny(), It.IsAny(), It.IsAny())) + .Callback((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); + } + } +} diff --git a/test/Renci.SshNet.Tests/Common/ScpTranscriptResult.cs b/test/Renci.SshNet.Tests/Common/ScpTranscriptResult.cs new file mode 100644 index 00000000..425c994f --- /dev/null +++ b/test/Renci.SshNet.Tests/Common/ScpTranscriptResult.cs @@ -0,0 +1,39 @@ +using System; +using System.Collections.Generic; + +namespace Renci.SshNet.Tests.Common +{ + /// + /// The outcome of . + /// + public sealed class ScpTranscriptResult + { + public ScpTranscriptResult(bool completed, Exception exception, IReadOnlyList sentData, IReadOnlyList execRequests) + { + Completed = completed; + Exception = exception; + SentData = sentData; + ExecRequests = execRequests; + } + + /// + /// Gets a value indicating whether the operation finished within the timeout. + /// + public bool Completed { get; } + + /// + /// Gets the exception thrown by the operation, if any. + /// + public Exception Exception { get; } + + /// + /// Gets the data the client sent on the channel, one entry per send. + /// + public IReadOnlyList SentData { get; } + + /// + /// Gets the commands the client sent in exec requests. + /// + public IReadOnlyList ExecRequests { get; } + } +}