Browse Source

Fix BlobCacheBufferedIndexInput large read after clone (#98970)

After a clone the delegate is positioned as if it has just filled the
buffer, but the buffer is empty. On most paths we go via `refill()`
which repositions the delegate correctly, but if we skip the buffer and
read directly we must also account for this case.
David Turner 2 years ago
parent
commit
fb95510c09

+ 5 - 0
docs/changelog/98970.yaml

@@ -0,0 +1,5 @@
+pr: 98970
+summary: Fix `BlobCacheBufferedIndexInput` large read after clone
+area: Snapshot/Restore
+type: bug
+issues: []

+ 10 - 7
x-pack/plugin/blob-cache/src/main/java/org/elasticsearch/blobcache/common/BlobCacheBufferedIndexInput.java

@@ -20,6 +20,8 @@ import java.nio.ByteOrder;
 /**
  * Copy of {@link org.apache.lucene.store.BufferedIndexInput} that contains optimizations that haven't made it to the Lucene version used
  * by Elasticsearch yet or that are only applicable to Elasticsearch.
+ * <p>
+ * Deviates from Lucene's implementation slightly to fix a bug - see [NOTE: Missing Seek] below, and #98970 for more details.
  */
 public abstract class BlobCacheBufferedIndexInput extends IndexInput implements RandomAccessInput {
 
@@ -106,13 +108,14 @@ public abstract class BlobCacheBufferedIndexInput extends IndexInput implements
                     buffer.get(b, offset, len);
                 }
             } else {
-                // The amount left to read is larger than the buffer
-                // or we've been asked to not use our buffer -
-                // there's no performance reason not to read it all
-                // at once. Note that unlike the previous code of
-                // this function, there is no need to do a seek
-                // here, because there's no need to reread what we
-                // had in the buffer.
+                // The amount left to read is larger than the buffer or we've been asked to not use our buffer - there's no performance
+                // reason not to read it all at once.
+                if (buffer == EMPTY_BYTEBUFFER) {
+                    // fresh clone, must seek
+                    // [NOTE: Missing Seek] This deviates from Lucene's BufferedIndexInput implementation - see #98970
+                    seekInternal(bufferStart);
+                } // else there's no need to do a seek here because we are already positioned correctly
+
                 long after = bufferStart + buffer.position() + len;
                 if (after > length()) throw new EOFException("read past EOF: " + this);
                 readInternal(ByteBuffer.wrap(b, offset, len));

+ 19 - 0
x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/store/input/DirectBlobContainerIndexInputTests.java

@@ -142,6 +142,25 @@ public class DirectBlobContainerIndexInputTests extends ESIndexInputTestCase {
         }
     }
 
+    public void testCloneAndLargeRead() throws IOException {
+        final Tuple<String, byte[]> bytes = randomChecksumBytes(between(ByteSizeUnit.KB.toIntBytes(2), ByteSizeUnit.KB.toIntBytes(10)));
+        try (var indexInput = createIndexInput(bytes)) {
+            indexInput.readLong();
+
+            final var clone = indexInput.clone();
+
+            // do a read which is large enough to exercise the path which bypasses the buffer and fills the output directly
+
+            final var originalBytes = new byte[2048];
+            indexInput.readBytes(originalBytes, 0, originalBytes.length);
+
+            final var cloneBytes = new byte[originalBytes.length];
+            clone.readBytes(cloneBytes, 0, cloneBytes.length);
+
+            assertArrayEquals(originalBytes, cloneBytes);
+        }
+    }
+
     public void testRandomOverflow() throws IOException {
         for (int i = 0; i < 100; i++) {
             final Tuple<String, byte[]> bytes = randomChecksumBytes(randomIntBetween(1, 1000));