added real bounds checking to ByteString.Slice (#6709)

* added real bounds checking to `ByteString.Slice`

Fixed a very weird bit of code that could result in byte buffers constantly allocating despite being of illegal size.

* Fix zero length slice

* Fix index out of bound bug

---------

Co-authored-by: Gregorius Soedharmo <[email protected]>

Author: Aaronontheweb

src/core/Akka.Streams.Tests/IO/InputStreamSinkSpec.cs

@@ -168,10 +168,11 @@ await this.AssertAllStagesStoppedAsync(() => {
 
                 while (!bytes.IsEmpty)
                 {
-                    var expected = bytes.Slice(0, 3);
-                    bytes = bytes.Slice(3);
+                    var max = Math.Min(bytes.Count, 3);
+                    var expected = bytes.Slice(0, max);
+                    bytes = bytes.Slice(max);
 
-                    var result = ReadN(inputStream, 3);
+                    var result = ReadN(inputStream, max);
                     result.Item1.Should().Be(expected.Count);
                     result.Item2.Should().BeEquivalentTo(expected);
                 }

src/core/Akka.Tests/Util/ByteStringSpec.cs

@@ -8,6 +8,7 @@
 using System.Linq;
 using System.Text;
 using Akka.IO;
+using FluentAssertions;
 using FsCheck;
 using Xunit;
 
@@ -44,8 +45,10 @@ public void A_ByteString_must_have_correct_size_when_concatenating()
         [Fact]
         public void A_ByteString_must_have_correct_size_when_slicing_from_index()
         {
-            Prop.ForAll((ByteString a, ByteString b) => (a + b).Slice(b.Count).Count == a.Count)
-                .QuickCheckThrowOnFailure();
+            var a = ByteString.FromBytes(new byte[]{ 1, 2, 3, 4, 5, 6, 7, 8, 9} );
+            var b = ByteString.FromBytes(new byte[] { 10, 11, 12, 13, 14, 15, 16, 17, 18 });
+
+            (a + b).Slice(b.Count).Count.Should().Be(a.Count);
         }
 
         [Fact]
@@ -57,8 +60,10 @@ public void A_ByteString_must_be_sequential_when_slicing_from_start()
         [Fact]
         public void A_ByteString_must_be_sequential_when_slicing_from_index()
         {
-            Prop.ForAll((ByteString a, ByteString b) => (a + b).Slice(a.Count).SequenceEqual(b))
-                .QuickCheckThrowOnFailure();
+            var a = ByteString.FromBytes(new byte[]{ 1, 2, 3, 4, 5, 6, 7, 8, 9} );
+            var b = ByteString.FromBytes(new byte[] { 10, 11, 12, 13, 14, 15, 16, 17, 18 });
+
+            (a + b).Slice(a.Count).Should().BeEquivalentTo(b);
         }
 
         [Fact]
@@ -74,14 +79,12 @@ public void A_ByteString_must_be_equal_to_the_original_when_compacting()
         [Fact]
         public void A_ByteString_must_be_equal_to_the_original_when_recombining()
         {
-            Prop.ForAll((ByteString xs, int until) =>
-            {
-                var tmp1 = xs.Slice(0, until);
-                var tmp2 = xs.Slice(until);
-                var tmp11 = tmp1.Slice(0, until);
-                var tmp12 = tmp1.Slice(until);
-                return (tmp11 + tmp12 + tmp2).SequenceEqual(xs);
-            }).QuickCheckThrowOnFailure();
+            var xs = ByteString.FromBytes(new byte[]{ 1, 2, 3, 4, 5, 6, 7, 8, 9} );
+            var tmp1 = xs.Slice(0, xs.Count / 2);
+            var tmp2 = xs.Slice(xs.Count / 2);
+            var tmp11 = tmp1.Slice(0, tmp1.Count / 2);
+            var tmp12 = tmp1.Slice(tmp1.Count / 2);
+            (tmp11 + tmp12 + tmp2).Should().BeEquivalentTo(xs);
         }
 
         [Fact]

src/core/Akka/Util/ByteString.cs

@@ -332,21 +332,22 @@ public ByteString Compact()
         /// </summary>
         /// <param name="index">index inside current <see cref="ByteString"/>, from which slicing should start</param>
         /// <param name="count">Number of bytes to fit into new <see cref="ByteString"/>.</param>
-        /// <returns></returns>
+        /// <exception cref="ArgumentOutOfRangeException">If index or count result in an invalid <see cref="ByteString"/>.</exception>
         public ByteString Slice(int index, int count)
         {
-            //TODO: this is really stupid, but previous impl didn't throw if arguments 
-            //      were out of range. We either have to round them to valid bounds or 
-            //      (future version, provide negative-arg slicing like i.e. Python).
-            if (index < 0) index = 0;
-            if (index >= _count) index = Math.Max(0, _count - 1);
-            if (count > _count - index) count = _count - index;
-            if (count <= 0) return Empty;
-
-            if (index == 0 && count == _count) return this;
+            if(index < 0)
+                throw new ArgumentOutOfRangeException(nameof(index), "Index must be positive number");
+            if (count < 0)
+                throw new ArgumentOutOfRangeException(nameof(count), "Count must be positive number");
+            if (count == 0) return Empty;
+            if(index > _count)
+                throw new ArgumentOutOfRangeException(nameof(index), "Index is outside of the bounds of the ByteString");
+            if(index + count > _count)
+                throw new ArgumentOutOfRangeException(nameof(count), "Index + count is outside of the bounds of the ByteString");
 
-            int j;
-            var i = GetBufferFittingIndex(index, out j);
+            if (index == 0 && count == _count) return this;
+
+            var i = GetBufferFittingIndex(index, out var j);
             var init = _buffers[i];
 
             var copied = Math.Min(init.Count - j, count);
@@ -503,7 +504,7 @@ public byte[] ToArray()
                 return Array.Empty<byte>();
 
             var copy = new byte[_count];
-            this.CopyTo(copy, 0, _count);
+            CopyTo(copy, 0, _count);
             return copy;
         }