From a7c985797608b9d1b8f425ba1ca76ba635ff8427 Mon Sep 17 00:00:00 2001
From: Jim Ferenczi <jim.ferenczi@elastic.co>
Date: Wed, 18 Apr 2018 13:01:06 +0200
Subject: [PATCH]  Fix binary doc values fetching in _search (#29567)

Binary doc values are retrieved during the DocValueFetchSubPhase through an instance of ScriptDocValues.
Since 6.0 ScriptDocValues instances are not allowed to reuse the object that they return (https://github.com/elastic/elasticsearch/issues/26775) but BinaryScriptDocValues doesn't follow this restriction and reuses instances of BytesRefBuilder among different documents.
This results in `field` values assigned to the wrong document in the response.
This commit fixes this issue by recreating the BytesRef for each value that needs to be returned.

 Fixes #29565
---
 .../index/fielddata/ScriptDocValues.java      | 16 ++++++++--
 .../fielddata/BinaryDVFieldDataTests.java     | 31 ++++++++++---------
 2 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java
index 552ddbf9d61..01f6bc19298 100644
--- a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java
+++ b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java
@@ -633,7 +633,12 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
 
         public BytesRef getBytesValue() {
             if (size() > 0) {
-                return values[0].get();
+                /**
+                 * We need to make a copy here because {@link BinaryScriptDocValues} might reuse the
+                 * returned value and the same instance might be used to
+                 * return values from multiple documents.
+                 **/
+                return values[0].toBytesRef();
             } else {
                 return null;
             }
@@ -658,14 +663,19 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
 
         @Override
         public BytesRef get(int index) {
-            return values[index].get();
+            /**
+             * We need to make a copy here because {@link BinaryScriptDocValues} might reuse the
+             * returned value and the same instance might be used to
+             * return values from multiple documents.
+             **/
+            return values[index].toBytesRef();
         }
 
         public BytesRef getValue() {
             if (count == 0) {
                 return new BytesRef();
             }
-            return values[0].get();
+            return values[0].toBytesRef();
         }
 
     }
diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/BinaryDVFieldDataTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/BinaryDVFieldDataTests.java
index 7f407dd1c01..3b29d15bf3f 100644
--- a/server/src/test/java/org/elasticsearch/index/fielddata/BinaryDVFieldDataTests.java
+++ b/server/src/test/java/org/elasticsearch/index/fielddata/BinaryDVFieldDataTests.java
@@ -52,7 +52,6 @@ public class BinaryDVFieldDataTests extends AbstractFieldDataTestCase {
 
         final DocumentMapper mapper = mapperService.documentMapperParser().parse("test", new CompressedXContent(mapping));
 
-
         List<BytesRef> bytesList1 = new ArrayList<>(2);
         bytesList1.add(randomBytes());
         bytesList1.add(randomBytes());
@@ -123,22 +122,26 @@ public class BinaryDVFieldDataTests extends AbstractFieldDataTestCase {
         // Test whether ScriptDocValues.BytesRefs makes a deepcopy
         fieldData = indexFieldData.load(reader);
         ScriptDocValues<?> scriptValues = fieldData.getScriptValues();
-        scriptValues.setNextDocId(0);
-        assertEquals(2, scriptValues.size());
-        assertEquals(bytesList1.get(0), scriptValues.get(0));
-        assertEquals(bytesList1.get(1), scriptValues.get(1));
+        Object[][] retValues = new BytesRef[4][0];
+        for (int i = 0; i < 4; i++) {
+            scriptValues.setNextDocId(i);
+            retValues[i] = new BytesRef[scriptValues.size()];
+            for (int j = 0; j < retValues[i].length; j++) {
+                retValues[i][j] = scriptValues.get(j);
+            }
+        }
+        assertEquals(2, retValues[0].length);
+        assertEquals(bytesList1.get(0), retValues[0][0]);
+        assertEquals(bytesList1.get(1), retValues[0][1]);
 
-        scriptValues.setNextDocId(1);
-        assertEquals(1, scriptValues.size());
-        assertEquals(bytes1, scriptValues.get(0));
+        assertEquals(1, retValues[1].length);
+        assertEquals(bytes1, retValues[1][0]);
 
-        scriptValues.setNextDocId(2);
-        assertEquals(0, scriptValues.size());
+        assertEquals(0, retValues[2].length);
 
-        scriptValues.setNextDocId(3);
-        assertEquals(2, scriptValues.size());
-        assertEquals(bytesList2.get(0), scriptValues.get(0));
-        assertEquals(bytesList2.get(1), scriptValues.get(1));
+        assertEquals(2, retValues[3].length);
+        assertEquals(bytesList2.get(0), retValues[3][0]);
+        assertEquals(bytesList2.get(1), retValues[3][1]);
     }
 
     private static BytesRef randomBytes() {