From ad56cc9bdf7cd317b9da8d80adc92db0a7bbb17f Mon Sep 17 00:00:00 2001 From: drieseng Date: Sun, 21 Aug 2016 18:14:03 +0200 Subject: [PATCH] Take into account the offset in SftpFileStream.Write(byte[] buffer, int offset, int count) when not writing to the buffer. Fixes issue #70. --- .../Sftp/Requests/SftpWriteRequestTest.cs | 25 ++- ...tGreatherThanTwoTimesTheWriteBufferSize.cs | 187 ++++++++++++++++++ .../Renci.SshNet.Tests.csproj | 1 + src/Renci.SshNet/Sftp/ISftpSession.cs | 10 +- .../Sftp/Requests/SftpWriteRequest.cs | 46 ++++- src/Renci.SshNet/Sftp/SftpFileStream.cs | 24 +-- src/Renci.SshNet/Sftp/SftpSession.cs | 14 +- src/Renci.SshNet/SftpClient.cs | 4 +- 8 files changed, 271 insertions(+), 40 deletions(-) create mode 100644 src/Renci.SshNet.Tests/Classes/Sftp/SftpFileStreamTest_Write_SessionOpen_CountGreatherThanTwoTimesTheWriteBufferSize.cs diff --git a/src/Renci.SshNet.Tests/Classes/Sftp/Requests/SftpWriteRequestTest.cs b/src/Renci.SshNet.Tests/Classes/Sftp/Requests/SftpWriteRequestTest.cs index 68531c9b..7e5233dd 100644 --- a/src/Renci.SshNet.Tests/Classes/Sftp/Requests/SftpWriteRequestTest.cs +++ b/src/Renci.SshNet.Tests/Classes/Sftp/Requests/SftpWriteRequestTest.cs @@ -15,8 +15,9 @@ namespace Renci.SshNet.Tests.Classes.Sftp.Requests private uint _protocolVersion; private uint _requestId; private byte[] _handle; - private ulong _offset; + private ulong _serverFileOffset; private byte[] _data; + private int _offset; private int _length; [TestInitialize] @@ -28,22 +29,25 @@ namespace Renci.SshNet.Tests.Classes.Sftp.Requests _requestId = (uint)random.Next(0, int.MaxValue); _handle = new byte[random.Next(1, 10)]; random.NextBytes(_handle); - _offset = (ulong) random.Next(0, int.MaxValue); - _data = new byte[random.Next(5, 10)]; + _serverFileOffset = (ulong) random.Next(0, int.MaxValue); + _data = new byte[random.Next(10, 15)]; random.NextBytes(_data); - _length = random.Next(1, 4); + _offset = random.Next(0, 4); + _length = random.Next(5, 10); } [TestMethod] public void Constructor() { - var request = new SftpWriteRequest(_protocolVersion, _requestId, _handle, _offset, _data, _length, null); + var request = new SftpWriteRequest(_protocolVersion, _requestId, _handle, _serverFileOffset, _data, _offset, _length, null); Assert.AreSame(_data, request.Data); Assert.AreSame(_handle, request.Handle); Assert.AreEqual(_length, request.Length); + Assert.AreEqual(_offset, request.Offset); Assert.AreEqual(_protocolVersion, request.ProtocolVersion); Assert.AreEqual(_requestId, request.RequestId); + Assert.AreEqual(_serverFileOffset, request.ServerFileOffset); Assert.AreEqual(SftpMessageTypes.Write, request.SftpMessageType); } @@ -58,8 +62,9 @@ namespace Renci.SshNet.Tests.Classes.Sftp.Requests _protocolVersion, _requestId, _handle, - _offset, + _serverFileOffset, _data, + _offset, _length, statusAction); @@ -72,7 +77,7 @@ namespace Renci.SshNet.Tests.Classes.Sftp.Requests [TestMethod] public void GetBytes() { - var request = new SftpWriteRequest(_protocolVersion, _requestId, _handle, _offset, _data, _length, null); + var request = new SftpWriteRequest(_protocolVersion, _requestId, _handle, _serverFileOffset, _data, _offset, _length, null); var bytes = request.GetBytes(); @@ -82,7 +87,7 @@ namespace Renci.SshNet.Tests.Classes.Sftp.Requests expectedBytesLength += 4; // RequestId expectedBytesLength += 4; // Handle length expectedBytesLength += _handle.Length; // Handle - expectedBytesLength += 8; // Offset + expectedBytesLength += 8; // ServerFileOffset expectedBytesLength += 4; // Data length expectedBytesLength += _length; // Data @@ -99,12 +104,12 @@ namespace Renci.SshNet.Tests.Classes.Sftp.Requests sshDataStream.Read(actualHandle, 0, actualHandle.Length); Assert.IsTrue(_handle.SequenceEqual(actualHandle)); - Assert.AreEqual(_offset, sshDataStream.ReadUInt64()); + Assert.AreEqual(_serverFileOffset, sshDataStream.ReadUInt64()); Assert.AreEqual((uint) _length, sshDataStream.ReadUInt32()); var actualData = new byte[_length]; sshDataStream.Read(actualData, 0, actualData.Length); - Assert.IsTrue(_data.Take(_length).SequenceEqual(actualData)); + Assert.IsTrue(_data.Take(_offset, _length).SequenceEqual(actualData)); Assert.IsTrue(sshDataStream.IsEndOfData); } diff --git a/src/Renci.SshNet.Tests/Classes/Sftp/SftpFileStreamTest_Write_SessionOpen_CountGreatherThanTwoTimesTheWriteBufferSize.cs b/src/Renci.SshNet.Tests/Classes/Sftp/SftpFileStreamTest_Write_SessionOpen_CountGreatherThanTwoTimesTheWriteBufferSize.cs new file mode 100644 index 00000000..5e8b5327 --- /dev/null +++ b/src/Renci.SshNet.Tests/Classes/Sftp/SftpFileStreamTest_Write_SessionOpen_CountGreatherThanTwoTimesTheWriteBufferSize.cs @@ -0,0 +1,187 @@ +using System; +using System.Globalization; +using System.IO; +using System.Threading; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using Renci.SshNet.Sftp; +using Renci.SshNet.Sftp.Responses; + +namespace Renci.SshNet.Tests.Classes.Sftp +{ + [TestClass] + public class SftpFileStreamTest_Write_SessionOpen_CountGreatherThanTwoTimesTheWriteBufferSize + { + private Mock _sftpSessionMock; + private string _path; + private SftpFileStream _sftpFileStream; + private byte[] _handle; + private SftpFileAttributes _fileAttributes; + private uint _bufferSize; + private uint _readBufferSize; + private uint _writeBufferSize; + private byte[] _data; + private int _count; + private int _offset; + private MockSequence _sequence; + private Random _random; + private uint _expectedWrittenByteCount; + private int _expectedBufferedByteCount; + private byte[] _expectedBufferedBytes; + + [TestInitialize] + public void Setup() + { + Arrange(); + Act(); + } + + [TestCleanup] + public void TearDown() + { + if (_sftpSessionMock != null) + { + // allow Dispose to complete successfully + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.IsOpen) + .Returns(true); + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.RequestWrite(_handle, _expectedWrittenByteCount, It.IsAny(), 0, _expectedBufferedByteCount, It.IsAny(), null)); + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.RequestClose(_handle)); + } + } + + protected void Arrange() + { + _random = new Random(); + _path = _random.Next().ToString(CultureInfo.InvariantCulture); + _handle = new[] {(byte) _random.Next(byte.MinValue, byte.MaxValue)}; + _fileAttributes = SftpFileAttributes.Empty; + _bufferSize = (uint) _random.Next(1, 1000); + _readBufferSize = (uint) _random.Next(0, 1000); + _writeBufferSize = (uint) _random.Next(500, 1000); + _data = new byte[(_writeBufferSize * 2) + 15]; + _random.NextBytes(_data); + _offset = _random.Next(1, 5); + // to get multiple SSH_FXP_WRITE messages (and verify the offset is updated correctly), we make sure + // the number of bytes to write is at least two times the write buffer size; we write a few extra bytes to + // ensure the buffer is not empty after the writes so we can verify whether Length, Dispose and Flush + // flush the buffer + _count = ((int) _writeBufferSize*2) + _random.Next(1, 5); + + _expectedWrittenByteCount = (2 * _writeBufferSize); + _expectedBufferedByteCount = (int)(_count - _expectedWrittenByteCount); + _expectedBufferedBytes = _data.Take(_offset + (int)_expectedWrittenByteCount, _expectedBufferedByteCount); + + _sftpSessionMock = new Mock(MockBehavior.Strict); + + _sequence = new MockSequence(); + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.RequestOpen(_path, Flags.Write | Flags.Truncate, true)) + .Returns(_handle); + _sftpSessionMock.InSequence(_sequence).Setup(p => p.RequestFStat(_handle)).Returns(_fileAttributes); + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.CalculateOptimalReadLength(_bufferSize)) + .Returns(_readBufferSize); + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.CalculateOptimalWriteLength(_bufferSize, _handle)) + .Returns(_writeBufferSize); + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.IsOpen) + .Returns(true); + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.RequestWrite(_handle, 0, _data, _offset, (int) _writeBufferSize, It.IsAny(), null)); + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.RequestWrite(_handle, _writeBufferSize, _data, _offset + (int) _writeBufferSize, (int)_writeBufferSize, It.IsAny(), null)); + + _sftpFileStream = new SftpFileStream(_sftpSessionMock.Object, _path, FileMode.Create, FileAccess.Write, (int) _bufferSize); + } + + protected void Act() + { + _sftpFileStream.Write(_data, _offset, _count); + } + + [TestMethod] + public void RequestWriteOnSftpSessionShouldBeInvokedTwice() + { + _sftpSessionMock.Verify(p => p.RequestWrite(_handle, 0, _data, _offset, (int)_writeBufferSize, It.IsAny(), null), Times.Once); + _sftpSessionMock.Verify(p => p.RequestWrite(_handle, _writeBufferSize, _data, _offset + (int)_writeBufferSize, (int)_writeBufferSize, It.IsAny(), null), Times.Once); + } + + [TestMethod] + public void PositionShouldBeNumberOfBytesWrittenToFileAndNUmberOfBytesInBuffer() + { + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.IsOpen) + .Returns(true); + + Assert.AreEqual(_count, _sftpFileStream.Position); + } + + [TestMethod] + public void LengthShouldFlushBufferAndReturnSizeOfFile() + { + var lengthFileAttributes = new SftpFileAttributes(DateTime.Now, DateTime.Now, _random.Next(), _random.Next(), + _random.Next(), (uint) _random.Next(0, int.MaxValue), null); + byte[] actualFlushedData = null; + + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.IsOpen) + .Returns(true); + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.RequestWrite(_handle, _expectedWrittenByteCount, It.IsAny(), 0, _expectedBufferedByteCount, It.IsAny(), null)) + .Callback>((handle, serverFileOffset, data, offset, length, wait, writeCompleted) => actualFlushedData = data.Take(offset, length)); + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.RequestFStat(_handle)) + .Returns(lengthFileAttributes); + + Assert.AreEqual(lengthFileAttributes.Size, _sftpFileStream.Length); + Assert.IsTrue(actualFlushedData.IsEqualTo(_expectedBufferedBytes)); + + _sftpSessionMock.Verify(p => p.RequestWrite(_handle, _expectedWrittenByteCount, It.IsAny(), 0, _expectedBufferedByteCount, It.IsAny(), null), Times.Once); + } + + [TestMethod] + public void DisposeShouldFlushBufferAndCloseRequest() + { + byte[] actualFlushedData = null; + + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.IsOpen) + .Returns(true); + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.RequestWrite(_handle, _expectedWrittenByteCount, It.IsAny(), 0, _expectedBufferedByteCount, It.IsAny(), null)) + .Callback>((handle, serverFileOffset, data, offset, length, wait, writeCompleted) => actualFlushedData = data.Take(offset, length)); + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.RequestClose(_handle)); + + _sftpFileStream.Dispose(); + + Assert.IsTrue(actualFlushedData.IsEqualTo(_expectedBufferedBytes)); + + _sftpSessionMock.Verify(p => p.RequestWrite(_handle, _expectedWrittenByteCount, It.IsAny(), 0, _expectedBufferedByteCount, It.IsAny(), null), Times.Once); + _sftpSessionMock.Verify(p => p.RequestClose(_handle), Times.Once); + } + + [TestMethod] + public void FlushShouldFlushBuffer() + { + byte[] actualFlushedData = null; + + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.IsOpen) + .Returns(true); + _sftpSessionMock.InSequence(_sequence) + .Setup(p => p.RequestWrite(_handle, _expectedWrittenByteCount, It.IsAny(), 0, _expectedBufferedByteCount, It.IsAny(), null)) + .Callback>((handle, serverFileOffset, data, offset, length, wait, writeCompleted) => actualFlushedData = data.Take(offset, length)); + + _sftpFileStream.Flush(); + + Assert.IsTrue(actualFlushedData.IsEqualTo(_expectedBufferedBytes)); + + _sftpSessionMock.Verify(p => p.RequestWrite(_handle, _expectedWrittenByteCount, It.IsAny(), 0, _expectedBufferedByteCount, It.IsAny(), null), Times.Once); + } + } +} diff --git a/src/Renci.SshNet.Tests/Renci.SshNet.Tests.csproj b/src/Renci.SshNet.Tests/Renci.SshNet.Tests.csproj index 85893840..1174fdbb 100644 --- a/src/Renci.SshNet.Tests/Renci.SshNet.Tests.csproj +++ b/src/Renci.SshNet.Tests/Renci.SshNet.Tests.csproj @@ -406,6 +406,7 @@ + diff --git a/src/Renci.SshNet/Sftp/ISftpSession.cs b/src/Renci.SshNet/Sftp/ISftpSession.cs index d0f25636..56190a9d 100644 --- a/src/Renci.SshNet/Sftp/ISftpSession.cs +++ b/src/Renci.SshNet/Sftp/ISftpSession.cs @@ -154,14 +154,16 @@ namespace Renci.SshNet.Sftp /// Performs SSH_FXP_WRITE request. /// /// The handle. - /// The offset. - /// The data to send. - /// The number of bytes of to send. + /// The the zero-based offset (in bytes) relative to the beginning of the file that the write must start at. + /// The buffer holding the data to write. + /// the zero-based offset in at which to begin taking bytes to write. + /// The length (in bytes) of the data to write. /// The wait event handle if needed. /// The callback to invoke when the write has completed. void RequestWrite(byte[] handle, - ulong offset, + ulong serverOffset, byte[] data, + int offset, int length, AutoResetEvent wait, Action writeCompleted = null); diff --git a/src/Renci.SshNet/Sftp/Requests/SftpWriteRequest.cs b/src/Renci.SshNet/Sftp/Requests/SftpWriteRequest.cs index 4b37a505..6e8ca316 100644 --- a/src/Renci.SshNet/Sftp/Requests/SftpWriteRequest.cs +++ b/src/Renci.SshNet/Sftp/Requests/SftpWriteRequest.cs @@ -12,10 +12,39 @@ namespace Renci.SshNet.Sftp.Requests public byte[] Handle { get; private set; } - public ulong Offset { get; private set; } + /// + /// Gets the zero-based offset (in bytes) relative to the beginning of the file that the write + /// must start at. + /// + /// + /// The zero-based offset (in bytes) relative to the beginning of the file that the write must + /// start at. + /// + public ulong ServerFileOffset { get; private set; } + /// + /// Gets the buffer holding the data to write. + /// + /// + /// The buffer holding the data to write. + /// public byte[] Data { get; private set; } + /// + /// Gets the zero-based offset in at which to begin taking bytes to + /// write. + /// + /// + /// The zero-based offset in at which to begin taking bytes to write. + /// + public int Offset { get; private set; } + + /// + /// Gets the length (in bytes) of the data to write. + /// + /// + /// The length (in bytes) of the data to write. + /// public int Length { get; private set; } protected override int BufferCapacity @@ -25,7 +54,7 @@ namespace Renci.SshNet.Sftp.Requests var capacity = base.BufferCapacity; capacity += 4; // Handle length capacity += Handle.Length; // Handle - capacity += 8; // Offset length + capacity += 8; // ServerFileOffset length capacity += 4; // Data length capacity += Length; // Data return capacity; @@ -35,15 +64,17 @@ namespace Renci.SshNet.Sftp.Requests public SftpWriteRequest(uint protocolVersion, uint requestId, byte[] handle, - ulong offset, + ulong serverFileOffset, byte[] data, + int offset, int length, Action statusAction) : base(protocolVersion, requestId, statusAction) { Handle = handle; - Offset = offset; + ServerFileOffset = serverFileOffset; Data = data; + Offset = offset; Length = length; } @@ -51,8 +82,9 @@ namespace Renci.SshNet.Sftp.Requests { base.LoadData(); Handle = ReadBinary(); - Offset = ReadUInt64(); + ServerFileOffset = ReadUInt64(); Data = ReadBinary(); + Offset = 0; Length = Data.Length; } @@ -60,8 +92,8 @@ namespace Renci.SshNet.Sftp.Requests { base.SaveData(); WriteBinaryString(Handle); - Write(Offset); - WriteBinary(Data, 0, Length); + Write(ServerFileOffset); + WriteBinary(Data, Offset, Length); } } } diff --git a/src/Renci.SshNet/Sftp/SftpFileStream.cs b/src/Renci.SshNet/Sftp/SftpFileStream.cs index cf2401c9..b392688e 100644 --- a/src/Renci.SshNet/Sftp/SftpFileStream.cs +++ b/src/Renci.SshNet/Sftp/SftpFileStream.cs @@ -271,6 +271,10 @@ namespace Renci.SshNet.Sftp _attributes = _session.RequestFStat(_handle); + // instead of using the specified buffer size as is, we use it to calculate a buffer size + // that ensures we always receive or send the max. number of bytes in a single SSH_FXP_READ + // or SSH_FXP_WRITE message + _readBufferSize = (int)session.CalculateOptimalReadLength((uint)bufferSize); _readBuffer = new byte[_readBufferSize]; _writeBufferSize = (int)session.CalculateOptimalWriteLength((uint)bufferSize, _handle); @@ -633,15 +637,13 @@ namespace Renci.SshNet.Sftp var tempLen = _writeBufferSize - _bufferPosition; if (tempLen <= 0) { - using (var wait = new AutoResetEvent(false)) - { - _session.RequestWrite(_handle, _serverFilePosition, _writeBuffer, _bufferPosition, wait); - _serverFilePosition += (ulong) _bufferPosition; - } - - _bufferPosition = 0; + // flush write buffer, and mark it empty + FlushWriteBuffer(); + // we can now write or buffer the full buffer size tempLen = _writeBufferSize; } + + // limit the number of bytes to write to the actual number of bytes requested if (tempLen > count) { tempLen = count; @@ -652,7 +654,7 @@ namespace Renci.SshNet.Sftp { using (var wait = new AutoResetEvent(false)) { - _session.RequestWrite(_handle, _serverFilePosition, buffer, tempLen, wait); + _session.RequestWrite(_handle, _serverFilePosition, buffer, offset, tempLen, wait); _serverFilePosition += (ulong) tempLen; } } @@ -675,7 +677,7 @@ namespace Renci.SshNet.Sftp { using (var wait = new AutoResetEvent(false)) { - _session.RequestWrite(_handle, _serverFilePosition, _writeBuffer, _bufferPosition, wait); + _session.RequestWrite(_handle, _serverFilePosition, _writeBuffer, 0, _bufferPosition, wait); _serverFilePosition += (ulong) _bufferPosition; } @@ -706,7 +708,7 @@ namespace Renci.SshNet.Sftp { using (var wait = new AutoResetEvent(false)) { - _session.RequestWrite(_handle, _serverFilePosition, _writeBuffer, _bufferPosition, wait); + _session.RequestWrite(_handle, _serverFilePosition, _writeBuffer, 0, _bufferPosition, wait); _serverFilePosition += (ulong) _bufferPosition; } @@ -789,7 +791,7 @@ namespace Renci.SshNet.Sftp { using (var wait = new AutoResetEvent(false)) { - _session.RequestWrite(_handle, _serverFilePosition, _writeBuffer, _bufferPosition, wait); + _session.RequestWrite(_handle, _serverFilePosition, _writeBuffer, 0, _bufferPosition, wait); _serverFilePosition += (ulong) _bufferPosition; } diff --git a/src/Renci.SshNet/Sftp/SftpSession.cs b/src/Renci.SshNet/Sftp/SftpSession.cs index 5a1265b4..c66ff71c 100644 --- a/src/Renci.SshNet/Sftp/SftpSession.cs +++ b/src/Renci.SshNet/Sftp/SftpSession.cs @@ -356,22 +356,24 @@ namespace Renci.SshNet.Sftp /// Performs SSH_FXP_WRITE request. /// /// The handle. - /// The offset. - /// The data to send. - /// The number of bytes of to send. + /// The the zero-based offset (in bytes) relative to the beginning of the file that the write must start at. + /// The buffer holding the data to write. + /// the zero-based offset in at which to begin taking bytes to write. + /// The length (in bytes) of the data to write. /// The wait event handle if needed. /// The callback to invoke when the write has completed. public void RequestWrite(byte[] handle, - ulong offset, + ulong serverOffset, byte[] data, + int offset, int length, AutoResetEvent wait, Action writeCompleted = null) { SshException exception = null; - var request = new SftpWriteRequest(ProtocolVersion, NextRequestId, handle, offset, data, length, - response => + var request = new SftpWriteRequest(ProtocolVersion, NextRequestId, handle, serverOffset, data, offset, + length, response => { if (writeCompleted != null) { diff --git a/src/Renci.SshNet/SftpClient.cs b/src/Renci.SshNet/SftpClient.cs index f09bdbb3..336cefc2 100644 --- a/src/Renci.SshNet/SftpClient.cs +++ b/src/Renci.SshNet/SftpClient.cs @@ -1136,7 +1136,7 @@ namespace Renci.SshNet /// Creates or overwrites the specified file. /// /// The path and name of the file to create. - /// The number of bytes buffered for reads and writes to the file. + /// The maximum number of bytes buffered for reads and writes to the file. /// /// A that provides read/write access to the file specified in path. /// @@ -2077,7 +2077,7 @@ namespace Renci.SshNet { var writtenBytes = offset + (ulong) bytesRead; - _sftpSession.RequestWrite(handle, offset, buffer, bytesRead, null, s => + _sftpSession.RequestWrite(handle, offset, buffer, 0, bytesRead, null, s => { if (s.StatusCode == StatusCodes.Ok) {