Call ensureNoSelfReferences() on _agg state variable after scripted metric agg script executions (#31044)

rationull · s1monw · commit 85c26d682a5a · 2018-06-11T08:39:05.000+02:00
Previously this was called for the combine script only. This change checks for self references for
init, map, and reduce scripts as well, and adds unit test coverage for the init, map, and combine cases.
diff --git a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.java b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.java
@@ -157,7 +157,7 @@ public Iterator<Object> iterator() {
 
     @Override
     public String stringify(Object object) {
-        CollectionUtils.ensureNoSelfReferences(object);
+        CollectionUtils.ensureNoSelfReferences(object, "CustomReflectionObjectHandler stringify");
         return super.stringify(object);
     }
 }
diff --git a/server/src/main/java/org/elasticsearch/common/util/CollectionUtils.java b/server/src/main/java/org/elasticsearch/common/util/CollectionUtils.java
@@ -29,6 +29,7 @@
 import java.util.IdentityHashMap;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import java.util.RandomAccess;
@@ -40,6 +41,7 @@
 import org.apache.lucene.util.BytesRefBuilder;
 import org.apache.lucene.util.InPlaceMergeSorter;
 import org.apache.lucene.util.IntroSorter;
+import org.elasticsearch.common.Strings;
 
 /** Collections-related utility methods. */
 public class CollectionUtils {
@@ -225,10 +227,17 @@ public static int[] toArray(Collection<Integer> ints) {
         return ints.stream().mapToInt(s -> s).toArray();
     }
 
-    public static void ensureNoSelfReferences(Object value) {
+    /**
+     * Deeply inspects a Map, Iterable, or Object array looking for references back to itself.
+     * @throws IllegalArgumentException if a self-reference is found
+     * @param value The object to evaluate looking for self references
+     * @param messageHint A string to be included in the exception message if the call fails, to provide
+     *                    more context to the handler of the exception
+     */
+    public static void ensureNoSelfReferences(Object value, String messageHint) {
         Iterable<?> it = convert(value);
         if (it != null) {
-            ensureNoSelfReferences(it, value, Collections.newSetFromMap(new IdentityHashMap<>()));
+            ensureNoSelfReferences(it, value, Collections.newSetFromMap(new IdentityHashMap<>()), messageHint);
         }
     }
 
@@ -247,13 +256,15 @@ private static Iterable<?> convert(Object value) {
         }
     }
 
-    private static void ensureNoSelfReferences(final Iterable<?> value, Object originalReference, final Set<Object> ancestors) {
+    private static void ensureNoSelfReferences(final Iterable<?> value, Object originalReference, final Set<Object> ancestors,
+                                               String messageHint) {
         if (value != null) {
             if (ancestors.add(originalReference) == false) {
-                throw new IllegalArgumentException("Iterable object is self-referencing itself");
+                String suffix = Strings.isNullOrEmpty(messageHint) ? "" : String.format(Locale.ROOT, " (%s)", messageHint);
+                throw new IllegalArgumentException("Iterable object is self-referencing itself" + suffix);
             }
             for (Object o : value) {
-                ensureNoSelfReferences(convert(o), o, ancestors);
+                ensureNoSelfReferences(convert(o), o, ancestors, messageHint);
             }
             ancestors.remove(originalReference);
         }
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/InternalScriptedMetric.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/InternalScriptedMetric.java
@@ -21,6 +21,7 @@
 
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.util.CollectionUtils;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.script.ExecutableScript;
 import org.elasticsearch.script.Script;
@@ -97,7 +98,11 @@ public InternalAggregation doReduce(List<InternalAggregation> aggregations, Redu
             ExecutableScript.Factory factory = reduceContext.scriptService().compile(
                 firstAggregation.reduceScript, ExecutableScript.AGGS_CONTEXT);
             ExecutableScript script = factory.newInstance(vars);
-            aggregation = Collections.singletonList(script.run());
+
+            Object scriptResult = script.run();
+            CollectionUtils.ensureNoSelfReferences(scriptResult, "reduce script");
+
+            aggregation = Collections.singletonList(scriptResult);
         } else if (reduceContext.isFinalReduce())  {
             aggregation = Collections.singletonList(aggregationObjects);
         } else {
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregator.java
@@ -69,6 +69,7 @@ public void collect(int doc, long bucket) throws IOException {
                 assert bucket == 0 : bucket;
                 leafMapScript.setDocument(doc);
                 leafMapScript.run();
+                CollectionUtils.ensureNoSelfReferences(params, "Scripted metric aggs map script");
             }
         };
     }
@@ -78,7 +79,7 @@ public InternalAggregation buildAggregation(long owningBucketOrdinal) {
         Object aggregation;
         if (combineScript != null) {
             aggregation = combineScript.run();
-            CollectionUtils.ensureNoSelfReferences(aggregation);
+            CollectionUtils.ensureNoSelfReferences(aggregation, "Scripted metric aggs combine script");
         } else {
             aggregation = params.get("_agg");
         }
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java
@@ -19,6 +19,7 @@
 
 package org.elasticsearch.search.aggregations.metrics.scripted;
 
+import org.elasticsearch.common.util.CollectionUtils;
 import org.elasticsearch.script.ExecutableScript;
 import org.elasticsearch.script.Script;
 import org.elasticsearch.script.SearchScript;
@@ -89,6 +90,7 @@ public Aggregator createInternal(Aggregator parent, boolean collectsFromSingleBu
         final Script reduceScript = deepCopyScript(this.reduceScript, context);
         if (initScript != null) {
             initScript.run();
+            CollectionUtils.ensureNoSelfReferences(aggParams.get("_agg"), "Scripted metric aggs init script");
         }
         return new ScriptedMetricAggregator(name, mapScript,
                 combineScript, reduceScript, aggParams, context, parent,
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java
@@ -458,7 +458,7 @@ public boolean advanceExact(int doc) throws IOException {
                         final BytesRef value = bytesValues.nextValue();
                         script.setNextAggregationValue(value.utf8ToString());
                         Object run = script.run();
-                        CollectionUtils.ensureNoSelfReferences(run);
+                        CollectionUtils.ensureNoSelfReferences(run, "ValuesSource.BytesValues script");
                         values[i].copyChars(run.toString());
                     }
                     sort();
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/values/ScriptBytesValues.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/values/ScriptBytesValues.java
@@ -45,7 +45,7 @@ private void set(int i, Object o) {
         if (o == null) {
             values[i].clear();
         } else {
-            CollectionUtils.ensureNoSelfReferences(o);
+            CollectionUtils.ensureNoSelfReferences(o, "ScriptBytesValues value");
             values[i].copyChars(o.toString());
         }
     }
diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsFetchSubPhase.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsFetchSubPhase.java
@@ -65,7 +65,7 @@ public void hitsExecute(SearchContext context, SearchHit[] hits) throws IOExcept
                 final Object value;
                 try {
                     value = leafScripts[i].run();
-                    CollectionUtils.ensureNoSelfReferences(value);
+                    CollectionUtils.ensureNoSelfReferences(value, "ScriptFieldsFetchSubPhase leaf script " + i);
                 } catch (RuntimeException e) {
                     if (scriptFields.get(i).ignoreException()) {
                         continue;
diff --git a/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java
@@ -343,7 +343,7 @@ public boolean advanceExact(int doc) throws IOException {
                             @Override
                             public BytesRef binaryValue() {
                                 final Object run = leafScript.run();
-                                CollectionUtils.ensureNoSelfReferences(run);
+                                CollectionUtils.ensureNoSelfReferences(run, "ScriptSortBuilder leaf script");
                                 spare.copyChars(run.toString());
                                 return spare.get();
                             }
diff --git a/server/src/test/java/org/elasticsearch/common/util/CollectionUtilsTests.java b/server/src/test/java/org/elasticsearch/common/util/CollectionUtilsTests.java
@@ -183,13 +183,14 @@ public void testPerfectPartition() {
     }
 
     public void testEnsureNoSelfReferences() {
-        CollectionUtils.ensureNoSelfReferences(emptyMap());
-        CollectionUtils.ensureNoSelfReferences(null);
+        CollectionUtils.ensureNoSelfReferences(emptyMap(), "test with empty map");
+        CollectionUtils.ensureNoSelfReferences(null, "test with null");
 
         Map<String, Object> map = new HashMap<>();
         map.put("field", map);
 
-        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->  CollectionUtils.ensureNoSelfReferences(map));
-        assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
+            () ->  CollectionUtils.ensureNoSelfReferences(map, "test with self ref"));
+        assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself (test with self ref)"));
     }
 }
diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java b/server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java
@@ -843,8 +843,8 @@ public void testEnsureNotNull() {
     }
 
     public void testEnsureNoSelfReferences() throws IOException {
-        CollectionUtils.ensureNoSelfReferences(emptyMap());
-        CollectionUtils.ensureNoSelfReferences(null);
+        builder().map(emptyMap());
+        builder().map(null);
 
         Map<String, Object> map = new HashMap<>();
         map.put("field", map);
diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorTests.java
@@ -73,6 +73,13 @@ public class ScriptedMetricAggregatorTests extends AggregatorTestCase {
             Collections.singletonMap("divisor", 4));
     private static final String CONFLICTING_PARAM_NAME = "initialValue";
 
+    private static final Script INIT_SCRIPT_SELF_REF = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "initScriptSelfRef",
+            Collections.emptyMap());
+    private static final Script MAP_SCRIPT_SELF_REF = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "mapScriptSelfRef",
+            Collections.emptyMap());
+    private static final Script COMBINE_SCRIPT_SELF_REF = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "combineScriptSelfRef",
+            Collections.emptyMap());
+
     private static final Map<String, Function<Map<String, Object>, Object>> SCRIPTS = new HashMap<>();
 
     @BeforeClass
@@ -127,6 +134,25 @@ public static void initMockScripts() {
             int divisor = ((Integer) params.get("divisor"));
             return ((List<Integer>) agg.get("collector")).stream().mapToInt(Integer::intValue).map(i -> i / divisor).sum();
         });
+
+        SCRIPTS.put("initScriptSelfRef", params -> {
+            Map<String, Object> agg = (Map<String, Object>) params.get("_agg");
+            agg.put("collector", new ArrayList<Integer>());
+            agg.put("selfRef", agg);
+            return agg;
+        });
+
+        SCRIPTS.put("mapScriptSelfRef", params -> {
+            Map<String, Object> agg = (Map<String, Object>) params.get("_agg");
+            agg.put("selfRef", agg);
+            return agg;
+        });
+
+        SCRIPTS.put("combineScriptSelfRef", params -> {
+           Map<String, Object> agg = (Map<String, Object>) params.get("_agg");
+           agg.put("selfRef", agg);
+           return agg;
+        });
     }
 
     @SuppressWarnings("unchecked")
@@ -257,6 +283,60 @@ public void testConflictingAggAndScriptParams() throws IOException {
         }
     }
 
+    public void testSelfReferencingAggStateAfterInit() throws IOException {
+        try (Directory directory = newDirectory()) {
+            try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
+                // No need to add docs for this test
+            }
+            try (IndexReader indexReader = DirectoryReader.open(directory)) {
+                ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME);
+                aggregationBuilder.initScript(INIT_SCRIPT_SELF_REF).mapScript(MAP_SCRIPT);
+
+                IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () ->
+                    search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder)
+                );
+                assertEquals("Iterable object is self-referencing itself (Scripted metric aggs init script)", ex.getMessage());
+            }
+        }
+    }
+
+    public void testSelfReferencingAggStateAfterMap() throws IOException {
+        try (Directory directory = newDirectory()) {
+            Integer numDocs = randomInt(100);
+            try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
+                for (int i = 0; i < numDocs; i++) {
+                    indexWriter.addDocument(singleton(new SortedNumericDocValuesField("number", i)));
+                }
+            }
+            try (IndexReader indexReader = DirectoryReader.open(directory)) {
+                ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME);
+                aggregationBuilder.initScript(INIT_SCRIPT).mapScript(MAP_SCRIPT_SELF_REF);
+
+                IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () ->
+                    search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder)
+                );
+                assertEquals("Iterable object is self-referencing itself (Scripted metric aggs map script)", ex.getMessage());
+            }
+        }
+    }
+
+    public void testSelfReferencingAggStateAfterCombine() throws IOException {
+        try (Directory directory = newDirectory()) {
+            try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
+                // No need to add docs for this test
+            }
+            try (IndexReader indexReader = DirectoryReader.open(directory)) {
+                ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME);
+                aggregationBuilder.initScript(INIT_SCRIPT).mapScript(MAP_SCRIPT).combineScript(COMBINE_SCRIPT_SELF_REF);
+
+                IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () ->
+                    search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder)
+                );
+                assertEquals("Iterable object is self-referencing itself (Scripted metric aggs combine script)", ex.getMessage());
+            }
+        }
+    }
+
     /**
      * We cannot use Mockito for mocking QueryShardContext in this case because
      * script-related methods (e.g. QueryShardContext#getLazyExecutableScript)

Original file line number	Diff line number	Diff line change
`@@ -157,7 +157,7 @@ public Iterator<Object> iterator() {`
`157`	`157`
`158`	`158`	`@Override`
`159`	`159`	`public String stringify(Object object) {`
`160`		`- CollectionUtils.ensureNoSelfReferences(object);`
	`160`	`+ CollectionUtils.ensureNoSelfReferences(object, "CustomReflectionObjectHandler stringify");`
`161`	`161`	`return super.stringify(object);`
`162`	`162`	`}`
`163`	`163`	`}`
Original file line number	Diff line number	Diff line change
`@@ -69,6 +69,7 @@ public void collect(int doc, long bucket) throws IOException {`
`69`	`69`	`assert bucket == 0 : bucket;`
`70`	`70`	`leafMapScript.setDocument(doc);`
`71`	`71`	`leafMapScript.run();`
	`72`	`+ CollectionUtils.ensureNoSelfReferences(params, "Scripted metric aggs map script");`
`72`	`73`	`}`
`73`	`74`	`};`
`74`	`75`	`}`
`@@ -78,7 +79,7 @@ public InternalAggregation buildAggregation(long owningBucketOrdinal) {`
`78`	`79`	`Object aggregation;`
`79`	`80`	`if (combineScript != null) {`
`80`	`81`	`aggregation = combineScript.run();`
`81`		`- CollectionUtils.ensureNoSelfReferences(aggregation);`
	`82`	`+ CollectionUtils.ensureNoSelfReferences(aggregation, "Scripted metric aggs combine script");`
`82`	`83`	`} else {`
`83`	`84`	`aggregation = params.get("_agg");`
`84`	`85`	`}`
Original file line number	Diff line number	Diff line change
`@@ -458,7 +458,7 @@ public boolean advanceExact(int doc) throws IOException {`
`458`	`458`	`final BytesRef value = bytesValues.nextValue();`
`459`	`459`	`script.setNextAggregationValue(value.utf8ToString());`
`460`	`460`	`Object run = script.run();`
`461`		`- CollectionUtils.ensureNoSelfReferences(run);`
	`461`	`+ CollectionUtils.ensureNoSelfReferences(run, "ValuesSource.BytesValues script");`
`462`	`462`	`values[i].copyChars(run.toString());`
`463`	`463`	`}`
`464`	`464`	`sort();`
Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,7 @@ private void set(int i, Object o) {`
`45`	`45`	`if (o == null) {`
`46`	`46`	`values[i].clear();`
`47`	`47`	`} else {`
`48`		`- CollectionUtils.ensureNoSelfReferences(o);`
	`48`	`+ CollectionUtils.ensureNoSelfReferences(o, "ScriptBytesValues value");`
`49`	`49`	`values[i].copyChars(o.toString());`
`50`	`50`	`}`
`51`	`51`	`}`
Original file line number	Diff line number	Diff line change
`@@ -343,7 +343,7 @@ public boolean advanceExact(int doc) throws IOException {`
`343`	`343`	`@Override`
`344`	`344`	`public BytesRef binaryValue() {`
`345`	`345`	`final Object run = leafScript.run();`
`346`		`- CollectionUtils.ensureNoSelfReferences(run);`
	`346`	`+ CollectionUtils.ensureNoSelfReferences(run, "ScriptSortBuilder leaf script");`
`347`	`347`	`spare.copyChars(run.toString());`
`348`	`348`	`return spare.get();`
`349`	`349`	`}`
Original file line number	Diff line number	Diff line change
`@@ -183,13 +183,14 @@ public void testPerfectPartition() {`
`183`	`183`	`}`
`184`	`184`
`185`	`185`	`public void testEnsureNoSelfReferences() {`
`186`		`- CollectionUtils.ensureNoSelfReferences(emptyMap());`
`187`		`- CollectionUtils.ensureNoSelfReferences(null);`
	`186`	`+ CollectionUtils.ensureNoSelfReferences(emptyMap(), "test with empty map");`
	`187`	`+ CollectionUtils.ensureNoSelfReferences(null, "test with null");`
`188`	`188`
`189`	`189`	`Map<String, Object> map = new HashMap<>();`
`190`	`190`	`map.put("field", map);`
`191`	`191`
`192`		`- IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> CollectionUtils.ensureNoSelfReferences(map));`
`193`		`- assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));`
	`192`	`+ IllegalArgumentException e = expectThrows(IllegalArgumentException.class,`
	`193`	`+ () -> CollectionUtils.ensureNoSelfReferences(map, "test with self ref"));`
	`194`	`+ assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself (test with self ref)"));`
`194`	`195`	`}`
`195`	`196`	`}`
Original file line number	Diff line number	Diff line change
`@@ -843,8 +843,8 @@ public void testEnsureNotNull() {`
`843`	`843`	`}`
`844`	`844`
`845`	`845`	`public void testEnsureNoSelfReferences() throws IOException {`
`846`		`- CollectionUtils.ensureNoSelfReferences(emptyMap());`
`847`		`- CollectionUtils.ensureNoSelfReferences(null);`
	`846`	`+ builder().map(emptyMap());`
	`847`	`+ builder().map(null);`
`848`	`848`
`849`	`849`	`Map<String, Object> map = new HashMap<>();`
`850`	`850`	`map.put("field", map);`