Fix BytesReferenceStreamInput#skip with offset (#25634)

There is a bug when a call to `BytesReferenceStreamInput` skip is made
on a `BytesReference` that has an initial offset. The offset for the
current slice is added to the current index and then subtracted from the
length. This introduces the possibility of a negative number of bytes to
skip. This happens inside a loop, which leads to an infinte loop.

This commit correctly subtracts the current slice index from the
slice.length. Additionally, the `BytesArrayTests` are modified to test
instances that include an offset.

core/src/main/java/org/elasticsearch/common/bytes/BytesReferenceStreamInput.java

@@ -32,7 +32,7 @@ import java.io.IOException;
  */
 final class BytesReferenceStreamInput extends StreamInput {
     private final BytesRefIterator iterator;
-    private int sliceOffset;
+    private int sliceIndex;
     private BytesRef slice;
     private final int length; // the total size of the stream
     private int offset; // the current position of the stream
@@ -42,7 +42,7 @@ final class BytesReferenceStreamInput extends StreamInput {
         this.slice = iterator.next();
         this.length = length;
         this.offset = 0;
-        this.sliceOffset = 0;
+        this.sliceIndex = 0;
     }
 
     @Override
@@ -51,15 +51,15 @@ final class BytesReferenceStreamInput extends StreamInput {
             throw new EOFException();
         }
         maybeNextSlice();
-        byte b = slice.bytes[slice.offset + (sliceOffset++)];
+        byte b = slice.bytes[slice.offset + (sliceIndex++)];
         offset++;
         return b;
     }
 
     private void maybeNextSlice() throws IOException {
-        while (sliceOffset == slice.length) {
+        while (sliceIndex == slice.length) {
             slice = iterator.next();
-            sliceOffset = 0;
+            sliceIndex = 0;
             if (slice == null) {
                 throw new EOFException();
             }
@@ -92,12 +92,12 @@ final class BytesReferenceStreamInput extends StreamInput {
         int destOffset = bOffset;
         while (remaining > 0) {
             maybeNextSlice();
-            final int currentLen = Math.min(remaining, slice.length - sliceOffset);
+            final int currentLen = Math.min(remaining, slice.length - sliceIndex);
             assert currentLen > 0 : "length has to be > 0 to make progress but was: " + currentLen;
-            System.arraycopy(slice.bytes, slice.offset + sliceOffset, b, destOffset, currentLen);
+            System.arraycopy(slice.bytes, slice.offset + sliceIndex, b, destOffset, currentLen);
             destOffset += currentLen;
             remaining -= currentLen;
-            sliceOffset += currentLen;
+            sliceIndex += currentLen;
             offset += currentLen;
             assert remaining >= 0 : "remaining: " + remaining;
         }
@@ -129,9 +129,9 @@ final class BytesReferenceStreamInput extends StreamInput {
         int remaining = numBytesSkipped;
         while (remaining > 0) {
             maybeNextSlice();
-            int currentLen = Math.min(remaining, slice.length - (slice.offset + sliceOffset));
+            int currentLen = Math.min(remaining, slice.length - sliceIndex);
             remaining -= currentLen;
-            sliceOffset += currentLen;
+            sliceIndex += currentLen;
             offset += currentLen;
             assert remaining >= 0 : "remaining: " + remaining;
         }

core/src/test/java/org/elasticsearch/common/bytes/BytesArrayTests.java

@@ -24,15 +24,25 @@ import org.hamcrest.Matchers;
 import java.io.IOException;
 
 public class BytesArrayTests extends AbstractBytesReferenceTestCase {
+
     @Override
     protected BytesReference newBytesReference(int length) throws IOException {
+        return newBytesReference(length, randomInt(length));
+    }
+
+    @Override
+    protected BytesReference newBytesReferenceWithOffsetOfZero(int length) throws IOException {
+        return newBytesReference(length, 0);
+    }
+
+    private BytesReference newBytesReference(int length, int offset) throws IOException {
         // we know bytes stream output always creates a paged bytes reference, we use it to create randomized content
-        final BytesStreamOutput out = new BytesStreamOutput(length);
+        final BytesStreamOutput out = new BytesStreamOutput(length + offset);
-        for (int i = 0; i < length; i++) {
+        for (int i = 0; i < length + offset; i++) {
             out.writeByte((byte) random().nextInt(1 << 8));
         }
-        assertEquals(length, out.size());
+        assertEquals(length + offset, out.size());
-        BytesArray ref = new BytesArray(out.bytes().toBytesRef());
+        BytesArray ref = new BytesArray(out.bytes().toBytesRef().bytes, offset, length);
         assertEquals(length, ref.length());
         assertTrue(ref instanceof BytesArray);
         assertThat(ref.length(), Matchers.equalTo(length));
@@ -46,14 +56,14 @@ public class BytesArrayTests extends AbstractBytesReferenceTestCase {
             BytesArray pbr = (BytesArray) newBytesReference(sizes[i]);
             byte[] array = pbr.array();
             assertNotNull(array);
-            assertEquals(sizes[i], array.length);
+            assertEquals(sizes[i], array.length - pbr.offset());
             assertSame(array, pbr.array());
         }
     }
 
     public void testArrayOffset() throws IOException {
         int length = randomInt(PAGE_SIZE * randomIntBetween(2, 5));
-        BytesArray pbr = (BytesArray) newBytesReference(length);
+        BytesArray pbr = (BytesArray) newBytesReferenceWithOffsetOfZero(length);
         assertEquals(0, pbr.offset());
     }
 }

core/src/test/java/org/elasticsearch/common/bytes/CompositeBytesReferenceTests.java

@@ -29,8 +29,14 @@ import java.util.ArrayList;
 import java.util.List;
 
 public class CompositeBytesReferenceTests extends AbstractBytesReferenceTestCase {
+
     @Override
     protected BytesReference newBytesReference(int length) throws IOException {
+        return newBytesReferenceWithOffsetOfZero(length);
+    }
+
+    @Override
+    protected BytesReference newBytesReferenceWithOffsetOfZero(int length) throws IOException {
         // we know bytes stream output always creates a paged bytes reference, we use it to create randomized content
         List<BytesReference> referenceList = newRefList(length);
         BytesReference ref = new CompositeBytesReference(referenceList.toArray(new BytesReference[0]));

core/src/test/java/org/elasticsearch/common/bytes/PagedBytesReferenceTests.java

@@ -37,7 +37,13 @@ import java.util.Arrays;
 
 public class PagedBytesReferenceTests extends AbstractBytesReferenceTestCase {
 
+    @Override
     protected BytesReference newBytesReference(int length) throws IOException {
+        return newBytesReferenceWithOffsetOfZero(length);
+    }
+
+    @Override
+    protected BytesReference newBytesReferenceWithOffsetOfZero(int length) throws IOException {
         // we know bytes stream output always creates a paged bytes reference, we use it to create randomized content
         ReleasableBytesStreamOutput out = new ReleasableBytesStreamOutput(length, bigarrays);
         for (int i = 0; i < length; i++) {

modules/transport-netty4/src/test/java/org/elasticsearch/transport/netty4/ByteBufBytesReferenceTests.java

@@ -28,8 +28,14 @@ import org.elasticsearch.common.io.stream.ReleasableBytesStreamOutput;
 import java.io.IOException;
 
 public class ByteBufBytesReferenceTests extends AbstractBytesReferenceTestCase {
+
     @Override
     protected BytesReference newBytesReference(int length) throws IOException {
+        return newBytesReferenceWithOffsetOfZero(length);
+    }
+
+    @Override
+    protected BytesReference newBytesReferenceWithOffsetOfZero(int length) throws IOException {
         ReleasableBytesStreamOutput out = new ReleasableBytesStreamOutput(length, bigarrays);
         for (int i = 0; i < length; i++) {
             out.writeByte((byte) random().nextInt(1 << 8));

test/framework/src/main/java/org/elasticsearch/common/bytes/AbstractBytesReferenceTestCase.java

@@ -433,7 +433,7 @@ public abstract class AbstractBytesReferenceTestCase extends ESTestCase {
 
     public void testSliceArrayOffset() throws IOException {
         int length = randomIntBetween(1, PAGE_SIZE * randomIntBetween(2, 5));
-        BytesReference pbr = newBytesReference(length);
+        BytesReference pbr = newBytesReferenceWithOffsetOfZero(length);
         int sliceOffset = randomIntBetween(0, pbr.length() - 1); // an offset to the end would be len 0
         int sliceLength = randomIntBetween(1, pbr.length() - sliceOffset);
         BytesReference slice = pbr.slice(sliceOffset, sliceLength);
@@ -466,7 +466,7 @@ public abstract class AbstractBytesReferenceTestCase extends ESTestCase {
 
     public void testSliceToBytesRef() throws IOException {
         int length = randomIntBetween(0, PAGE_SIZE);
-        BytesReference pbr = newBytesReference(length);
+        BytesReference pbr = newBytesReferenceWithOffsetOfZero(length);
         // get a BytesRef from a slice
         int sliceOffset = randomIntBetween(0, pbr.length());
         int sliceLength = randomIntBetween(0, pbr.length() - sliceOffset);
@@ -544,6 +544,8 @@ public abstract class AbstractBytesReferenceTestCase extends ESTestCase {
 
     protected abstract BytesReference newBytesReference(int length) throws IOException;
 
+    protected abstract BytesReference newBytesReferenceWithOffsetOfZero(int length) throws IOException;
+
     public void testCompareTo() throws IOException {
         final int iters = randomIntBetween(5, 10);
         for (int i = 0; i < iters; i++) {