From c1c9744033f7d39552c5b4bd62da56e45aac8646 Mon Sep 17 00:00:00 2001 From: Robert Hague Date: Thu, 20 Feb 2025 10:57:45 +0100 Subject: [PATCH 1/2] Add a Stream buffer validation helper --- src/Renci.SshNet/Common/ChannelInputStream.cs | 15 +-- src/Renci.SshNet/Common/PipeStream.cs | 10 ++ src/Renci.SshNet/Common/ThrowHelper.cs | 34 +++++++ src/Renci.SshNet/Sftp/SftpFileStream.cs | 98 ++++--------------- src/Renci.SshNet/ShellStream.cs | 10 ++ 5 files changed, 77 insertions(+), 90 deletions(-) diff --git a/src/Renci.SshNet/Common/ChannelInputStream.cs b/src/Renci.SshNet/Common/ChannelInputStream.cs index db0ace5d3..a5dd0b417 100644 --- a/src/Renci.SshNet/Common/ChannelInputStream.cs +++ b/src/Renci.SshNet/Common/ChannelInputStream.cs @@ -101,17 +101,10 @@ public override int Read(byte[] buffer, int offset, int count) /// offset or count is negative. public override void Write(byte[] buffer, int offset, int count) { - ThrowHelper.ThrowIfNull(buffer); - - if (offset + count > buffer.Length) - { - throw new ArgumentException("The sum of offset and count is greater than the buffer length."); - } - - if (offset < 0 || count < 0) - { - throw new ArgumentOutOfRangeException(nameof(offset), "offset or count is negative."); - } +#if !NET + ThrowHelper. +#endif + ValidateBufferArguments(buffer, offset, count); ThrowHelper.ThrowObjectDisposedIf(_isDisposed, this); diff --git a/src/Renci.SshNet/Common/PipeStream.cs b/src/Renci.SshNet/Common/PipeStream.cs index a2d09fefb..75224f827 100644 --- a/src/Renci.SshNet/Common/PipeStream.cs +++ b/src/Renci.SshNet/Common/PipeStream.cs @@ -64,6 +64,11 @@ public override void SetLength(long value) /// public override int Read(byte[] buffer, int offset, int count) { +#if !NET + ThrowHelper. +#endif + ValidateBufferArguments(buffer, offset, count); + lock (_sync) { while (_head == _tail && !_disposed) @@ -88,6 +93,11 @@ public override int Read(byte[] buffer, int offset, int count) /// public override void Write(byte[] buffer, int offset, int count) { +#if !NET + ThrowHelper. +#endif + ValidateBufferArguments(buffer, offset, count); + lock (_sync) { ThrowHelper.ThrowObjectDisposedIf(_disposed, this); diff --git a/src/Renci.SshNet/Common/ThrowHelper.cs b/src/Renci.SshNet/Common/ThrowHelper.cs index 398a2c120..cfde5fd33 100644 --- a/src/Renci.SshNet/Common/ThrowHelper.cs +++ b/src/Renci.SshNet/Common/ThrowHelper.cs @@ -79,5 +79,39 @@ static void Throw(string? argument, string? paramName) } #endif } + +#if !NET + // A rough copy of + // https://github.com/dotnet/runtime/blob/1d1bf92fcf43aa6981804dc53c5174445069c9e4/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs#L960C13-L974C10 + // for lower targets. + public static void ValidateBufferArguments(byte[] buffer, int offset, int count) + { + ThrowIfNull(buffer); + + if (offset < 0) + { + Throw(); + + [DoesNotReturn] + static void Throw() + { + throw new ArgumentOutOfRangeException(nameof(offset), "Non-negative number required."); + } + } + + if ((uint)count > buffer.Length - offset) + { + Throw(); + + [DoesNotReturn] + static void Throw() + { + throw new ArgumentOutOfRangeException( + nameof(count), + "Offset and length were out of bounds for the array or count is greater than the number of elements from index to the end of the source collection."); + } + } + } +#endif } } diff --git a/src/Renci.SshNet/Sftp/SftpFileStream.cs b/src/Renci.SshNet/Sftp/SftpFileStream.cs index 06f44f98d..53826a0f0 100644 --- a/src/Renci.SshNet/Sftp/SftpFileStream.cs +++ b/src/Renci.SshNet/Sftp/SftpFileStream.cs @@ -511,28 +511,12 @@ public override Task FlushAsync(CancellationToken cancellationToken) /// public override int Read(byte[] buffer, int offset, int count) { - var readLen = 0; - - ThrowHelper.ThrowIfNull(buffer); - -#if NET - ArgumentOutOfRangeException.ThrowIfNegative(offset); - ArgumentOutOfRangeException.ThrowIfNegative(count); -#else - if (offset < 0) - { - throw new ArgumentOutOfRangeException(nameof(offset)); - } - - if (count < 0) - { - throw new ArgumentOutOfRangeException(nameof(count)); - } +#if !NET + ThrowHelper. #endif - if ((buffer.Length - offset) < count) - { - throw new ArgumentException("Invalid array range."); - } + ValidateBufferArguments(buffer, offset, count); + + var readLen = 0; // Lock down the file stream while we do this. lock (_lock) @@ -653,28 +637,14 @@ public override int Read(byte[] buffer, int offset, int count) /// A that represents the asynchronous read operation. public override async Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) { - var readLen = 0; - - ThrowHelper.ThrowIfNull(buffer); +#if !NET + ThrowHelper. +#endif + ValidateBufferArguments(buffer, offset, count); -#if NET - ArgumentOutOfRangeException.ThrowIfNegative(offset); - ArgumentOutOfRangeException.ThrowIfNegative(count); -#else - if (offset < 0) - { - throw new ArgumentOutOfRangeException(nameof(offset)); - } + cancellationToken.ThrowIfCancellationRequested(); - if (count < 0) - { - throw new ArgumentOutOfRangeException(nameof(count)); - } -#endif - if ((buffer.Length - offset) < count) - { - throw new ArgumentException("Invalid array range."); - } + var readLen = 0; CheckSessionIsOpen(); @@ -1005,26 +975,10 @@ public override void SetLength(long value) /// Methods were called after the stream was closed. public override void Write(byte[] buffer, int offset, int count) { - ThrowHelper.ThrowIfNull(buffer); - -#if NET - ArgumentOutOfRangeException.ThrowIfNegative(offset); - ArgumentOutOfRangeException.ThrowIfNegative(count); -#else - if (offset < 0) - { - throw new ArgumentOutOfRangeException(nameof(offset)); - } - - if (count < 0) - { - throw new ArgumentOutOfRangeException(nameof(count)); - } +#if !NET + ThrowHelper. #endif - if ((buffer.Length - offset) < count) - { - throw new ArgumentException("Invalid array range."); - } + ValidateBufferArguments(buffer, offset, count); // Lock down the file stream while we do this. lock (_lock) @@ -1105,26 +1059,12 @@ public override void Write(byte[] buffer, int offset, int count) /// Methods were called after the stream was closed. public override async Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) { - ThrowHelper.ThrowIfNull(buffer); - -#if NET - ArgumentOutOfRangeException.ThrowIfNegative(offset); - ArgumentOutOfRangeException.ThrowIfNegative(count); -#else - if (offset < 0) - { - throw new ArgumentOutOfRangeException(nameof(offset)); - } - - if (count < 0) - { - throw new ArgumentOutOfRangeException(nameof(count)); - } +#if !NET + ThrowHelper. #endif - if ((buffer.Length - offset) < count) - { - throw new ArgumentException("Invalid array range."); - } + ValidateBufferArguments(buffer, offset, count); + + cancellationToken.ThrowIfCancellationRequested(); CheckSessionIsOpen(); diff --git a/src/Renci.SshNet/ShellStream.cs b/src/Renci.SshNet/ShellStream.cs index 55bbec864..c1a3c2dea 100644 --- a/src/Renci.SshNet/ShellStream.cs +++ b/src/Renci.SshNet/ShellStream.cs @@ -789,6 +789,11 @@ public string Read() /// public override int Read(byte[] buffer, int offset, int count) { +#if !NET + ThrowHelper. +#endif + ValidateBufferArguments(buffer, offset, count); + lock (_sync) { while (_readHead == _readTail && !_disposed) @@ -835,6 +840,11 @@ public void Write(string? text) /// public override void Write(byte[] buffer, int offset, int count) { +#if !NET + ThrowHelper. +#endif + ValidateBufferArguments(buffer, offset, count); + ThrowHelper.ThrowObjectDisposedIf(_disposed, this); while (count > 0) From 37efd9567e199e363d3566939bb5a6318e915fcd Mon Sep 17 00:00:00 2001 From: Robert Hague Date: Sun, 2 Mar 2025 19:55:54 +0100 Subject: [PATCH 2/2] add ThrowHelper.ThrowIfNegative --- src/Renci.SshNet/Common/PacketDump.cs | 6 +---- src/Renci.SshNet/Common/ThrowHelper.cs | 26 ++++++++++++------- src/Renci.SshNet/Sftp/SftpFileStream.cs | 9 +------ .../Classes/Common/PacketDumpTest.cs | 3 --- 4 files changed, 19 insertions(+), 25 deletions(-) diff --git a/src/Renci.SshNet/Common/PacketDump.cs b/src/Renci.SshNet/Common/PacketDump.cs index 608a56f74..ec3b58587 100644 --- a/src/Renci.SshNet/Common/PacketDump.cs +++ b/src/Renci.SshNet/Common/PacketDump.cs @@ -15,11 +15,7 @@ public static string Create(List data, int indentLevel) public static string Create(byte[] data, int indentLevel) { ThrowHelper.ThrowIfNull(data); - - if (indentLevel < 0) - { - throw new ArgumentOutOfRangeException(nameof(indentLevel), "Cannot be less than zero."); - } + ThrowHelper.ThrowIfNegative(indentLevel); const int lineWidth = 16; diff --git a/src/Renci.SshNet/Common/ThrowHelper.cs b/src/Renci.SshNet/Common/ThrowHelper.cs index cfde5fd33..be6b13b4b 100644 --- a/src/Renci.SshNet/Common/ThrowHelper.cs +++ b/src/Renci.SshNet/Common/ThrowHelper.cs @@ -87,31 +87,39 @@ static void Throw(string? argument, string? paramName) public static void ValidateBufferArguments(byte[] buffer, int offset, int count) { ThrowIfNull(buffer); + ThrowIfNegative(offset); - if (offset < 0) + if ((uint)count > buffer.Length - offset) { Throw(); [DoesNotReturn] static void Throw() { - throw new ArgumentOutOfRangeException(nameof(offset), "Non-negative number required."); + throw new ArgumentOutOfRangeException( + nameof(count), + "Offset and length were out of bounds for the array or count is greater than the number of elements from index to the end of the source collection."); } } + } +#endif - if ((uint)count > buffer.Length - offset) + public static void ThrowIfNegative(long value, [CallerArgumentExpression(nameof(value))] string? paramName = null) + { +#if NET + ArgumentOutOfRangeException.ThrowIfNegative(value, paramName); +#else + if (value < 0) { - Throw(); + Throw(value, paramName); [DoesNotReturn] - static void Throw() + static void Throw(long value, string? paramName) { - throw new ArgumentOutOfRangeException( - nameof(count), - "Offset and length were out of bounds for the array or count is greater than the number of elements from index to the end of the source collection."); + throw new ArgumentOutOfRangeException(paramName, value, "Value must be non-negative."); } } - } #endif + } } } diff --git a/src/Renci.SshNet/Sftp/SftpFileStream.cs b/src/Renci.SshNet/Sftp/SftpFileStream.cs index 53826a0f0..2965f86e7 100644 --- a/src/Renci.SshNet/Sftp/SftpFileStream.cs +++ b/src/Renci.SshNet/Sftp/SftpFileStream.cs @@ -922,14 +922,7 @@ public override long Seek(long offset, SeekOrigin origin) /// public override void SetLength(long value) { -#if NET - ArgumentOutOfRangeException.ThrowIfNegative(value); -#else - if (value < 0) - { - throw new ArgumentOutOfRangeException(nameof(value)); - } -#endif + ThrowHelper.ThrowIfNegative(value); // Lock down the file stream while we do this. lock (_lock) diff --git a/test/Renci.SshNet.Tests/Classes/Common/PacketDumpTest.cs b/test/Renci.SshNet.Tests/Classes/Common/PacketDumpTest.cs index be6ad48e8..ebc7f7f20 100644 --- a/test/Renci.SshNet.Tests/Classes/Common/PacketDumpTest.cs +++ b/test/Renci.SshNet.Tests/Classes/Common/PacketDumpTest.cs @@ -3,7 +3,6 @@ using Microsoft.VisualStudio.TestTools.UnitTesting; using Renci.SshNet.Common; -using Renci.SshNet.Tests.Common; namespace Renci.SshNet.Tests.Classes.Common { @@ -41,8 +40,6 @@ public void Create_ByteArrayAndIndentLevel_IndentLevelLessThanZero() { Assert.IsNull(ex.InnerException); - ArgumentExceptionAssert.MessageEquals("Cannot be less than zero.", ex); - Assert.AreEqual("indentLevel", ex.ParamName); } }