From a3ade99fcf9d95a8e9c40388d283b69a379f0377 Mon Sep 17 00:00:00 2001 From: Tim Brooks Date: Tue, 11 Jul 2017 09:54:29 -0500 Subject: [PATCH] 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. --- .../bytes/BytesReferenceStreamInput.java | 20 ++++++++--------- .../common/bytes/BytesArrayTests.java | 22 ++++++++++++++----- .../bytes/CompositeBytesReferenceTests.java | 6 +++++ .../bytes/PagedBytesReferenceTests.java | 6 +++++ .../netty4/ByteBufBytesReferenceTests.java | 6 +++++ .../bytes/AbstractBytesReferenceTestCase.java | 6 +++-- 6 files changed, 48 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/bytes/BytesReferenceStreamInput.java b/core/src/main/java/org/elasticsearch/common/bytes/BytesReferenceStreamInput.java index fff6392f238..f7f1cdc6501 100644 --- a/core/src/main/java/org/elasticsearch/common/bytes/BytesReferenceStreamInput.java +++ b/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; } diff --git a/core/src/test/java/org/elasticsearch/common/bytes/BytesArrayTests.java b/core/src/test/java/org/elasticsearch/common/bytes/BytesArrayTests.java index fff030200b7..16d69f829a8 100644 --- a/core/src/test/java/org/elasticsearch/common/bytes/BytesArrayTests.java +++ b/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); - for (int i = 0; i < length; i++) { + final BytesStreamOutput out = new BytesStreamOutput(length + offset); + for (int i = 0; i < length + offset; i++) { out.writeByte((byte) random().nextInt(1 << 8)); } - assertEquals(length, out.size()); - BytesArray ref = new BytesArray(out.bytes().toBytesRef()); + assertEquals(length + offset, out.size()); + 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()); } } diff --git a/core/src/test/java/org/elasticsearch/common/bytes/CompositeBytesReferenceTests.java b/core/src/test/java/org/elasticsearch/common/bytes/CompositeBytesReferenceTests.java index aec957aba68..05d7b8dea6a 100644 --- a/core/src/test/java/org/elasticsearch/common/bytes/CompositeBytesReferenceTests.java +++ b/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 referenceList = newRefList(length); BytesReference ref = new CompositeBytesReference(referenceList.toArray(new BytesReference[0])); diff --git a/core/src/test/java/org/elasticsearch/common/bytes/PagedBytesReferenceTests.java b/core/src/test/java/org/elasticsearch/common/bytes/PagedBytesReferenceTests.java index 6ae2b3cf943..ea592b12e3b 100644 --- a/core/src/test/java/org/elasticsearch/common/bytes/PagedBytesReferenceTests.java +++ b/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++) { diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/transport/netty4/ByteBufBytesReferenceTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/transport/netty4/ByteBufBytesReferenceTests.java index bce875e8516..afe6bbbc90f 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/transport/netty4/ByteBufBytesReferenceTests.java +++ b/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)); diff --git a/test/framework/src/main/java/org/elasticsearch/common/bytes/AbstractBytesReferenceTestCase.java b/test/framework/src/main/java/org/elasticsearch/common/bytes/AbstractBytesReferenceTestCase.java index c6551543473..f1c6bd412a5 100644 --- a/test/framework/src/main/java/org/elasticsearch/common/bytes/AbstractBytesReferenceTestCase.java +++ b/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++) {