Ensure we protect Collections obtained from scripts from self-referen…

…cing (#28335) Self referencing maps can cause SOE if they are iterated ie. in their toString methods. This chance adds some protected to the usage of those collections.
elastic · Jan 24, 2018 · 0fa58d2 · 0fa58d2
1 parent bbf9edd
commit 0fa58d2
Show file tree

Hide file tree

Showing 13 changed files with 129 additions and 48 deletions.
diff --git a/core/src/main/java/org/elasticsearch/common/util/CollectionUtils.java b/core/src/main/java/org/elasticsearch/common/util/CollectionUtils.java
@@ -19,16 +19,20 @@
 
 package org.elasticsearch.common.util;
 
+import java.nio.file.Path;
 import java.util.AbstractList;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.IdentityHashMap;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Map;
 import java.util.Objects;
 import java.util.RandomAccess;
+import java.util.Set;
 
 import com.carrotsearch.hppc.ObjectArrayList;
 import org.apache.lucene.util.BytesRef;
@@ -221,6 +225,40 @@ public static int[] toArray(Collection<Integer> ints) {
         return ints.stream().mapToInt(s -> s).toArray();
     }
 
+    public static void ensureNoSelfReferences(Object value) {
+        Iterable<?> it = convert(value);
+        if (it != null) {
+            ensureNoSelfReferences(it, value, Collections.newSetFromMap(new IdentityHashMap<>()));
+        }
+    }
+
+    private static Iterable<?> convert(Object value) {
+        if (value == null) {
+            return null;
+        }
+        if (value instanceof Map) {
+            return ((Map<?,?>) value).values();
+        } else if ((value instanceof Iterable) && (value instanceof Path == false)) {
+            return (Iterable<?>) value;
+        } else if (value instanceof Object[]) {
+            return Arrays.asList((Object[]) value);
+        } else {
+            return null;
+        }
+    }
+
+    private static void ensureNoSelfReferences(final Iterable<?> value, Object originalReference, final Set<Object> ancestors) {
+        if (value != null) {
+            if (ancestors.add(originalReference) == false) {
+                throw new IllegalArgumentException("Iterable object is self-referencing itself");
+            }
+            for (Object o : value) {
+                ensureNoSelfReferences(convert(o), o, ancestors);
+            }
+            ancestors.remove(originalReference);
+        }
+    }
+
     private static class RotatedList<T> extends AbstractList<T> implements RandomAccess {
 
         private final List<T> in;

diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java b/core/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java
@@ -28,6 +28,7 @@
 import org.elasticsearch.common.text.Text;
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.common.unit.TimeValue;
+import org.elasticsearch.common.util.CollectionUtils;
 import org.joda.time.DateTimeZone;
 import org.joda.time.ReadableInstant;
 import org.joda.time.format.DateTimeFormatter;
@@ -38,12 +39,10 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.nio.file.Path;
-import java.util.Arrays;
 import java.util.Calendar;
 import java.util.Collections;
 import java.util.Date;
 import java.util.HashMap;
-import java.util.IdentityHashMap;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
@@ -783,7 +782,7 @@ XContentBuilder values(Object[] values) throws IOException {
 
         // checks that the array of object does not contain references to itself because
         // iterating over entries will cause a stackoverflow error
-        ensureNoSelfReferences(values);
+        CollectionUtils.ensureNoSelfReferences(values);
 
         startArray();
         for (Object o : values) {
@@ -869,7 +868,7 @@ public XContentBuilder map(Map<String, ?> values) throws IOException {
 
         // checks that the map does not contain references to itself because
         // iterating over map entries will cause a stackoverflow error
-        ensureNoSelfReferences(values);
+        CollectionUtils.ensureNoSelfReferences(values);
 
         startObject();
         for (Map.Entry<String, ?> value : values.entrySet()) {
@@ -895,7 +894,7 @@ private XContentBuilder value(Iterable<?> values) throws IOException {
         } else {
             // checks that the iterable does not contain references to itself because
             // iterating over entries will cause a stackoverflow error
-            ensureNoSelfReferences(values);
+            CollectionUtils.ensureNoSelfReferences(values);
 
             startArray();
             for (Object value : values) {
@@ -1066,32 +1065,4 @@ static void ensureNotNull(Object value, String message) {
             throw new IllegalArgumentException(message);
         }
     }
-
-    static void ensureNoSelfReferences(Object value) {
-        ensureNoSelfReferences(value, Collections.newSetFromMap(new IdentityHashMap<>()));
-    }
-
-    private static void ensureNoSelfReferences(final Object value, final Set<Object> ancestors) {
-        if (value != null) {
-
-            Iterable<?> it;
-            if (value instanceof Map) {
-                it = ((Map) value).values();
-            } else if ((value instanceof Iterable) && (value instanceof Path == false)) {
-                it = (Iterable) value;
-            } else if (value instanceof Object[]) {
-                it = Arrays.asList((Object[]) value);
-            } else {
-                return;
-            }
-
-            if (ancestors.add(value) == false) {
-                throw new IllegalArgumentException("Object has already been built and is self-referencing itself");
-            }
-            for (Object o : it) {
-                ensureNoSelfReferences(o, ancestors);
-            }
-            ancestors.remove(value);
-        }
-    }
 }
diff --git a/...java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregator.java b/...java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregator.java
@@ -20,6 +20,7 @@
 package org.elasticsearch.search.aggregations.metrics.scripted;
 
 import org.apache.lucene.index.LeafReaderContext;
+import org.elasticsearch.common.util.CollectionUtils;
 import org.elasticsearch.script.ExecutableScript;
 import org.elasticsearch.script.LeafSearchScript;
 import org.elasticsearch.script.Script;
@@ -78,6 +79,7 @@ public InternalAggregation buildAggregation(long owningBucketOrdinal) {
         Object aggregation;
         if (combineScript != null) {
             aggregation = combineScript.run();
+            CollectionUtils.ensureNoSelfReferences(aggregation);
         } else {
             aggregation = params.get("_agg");
         }

diff --git a/...asticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java b/...asticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java
@@ -114,10 +114,11 @@ public InternalAggregation reduce(InternalAggregation aggregation, ReduceContext
             } else {
                 ExecutableScript executableScript = reduceContext.scriptService().executable(compiledScript, vars);
                 Object returned = executableScript.run();
+                // no need to check for self references since only numbers are valid
                 if (returned == null) {
                     newBuckets.add(bucket);
                 } else {
-                    if (!(returned instanceof Number)) {
+                    if ((returned instanceof Number) == false) {
                         throw new AggregationExecutionException("series_arithmetic script for reducer [" + name()
                                 + "] must return a Number");
                     }

diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java b/core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java
@@ -29,6 +29,7 @@
 import org.apache.lucene.util.Bits;
 import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.common.lucene.ScorerAware;
+import org.elasticsearch.common.util.CollectionUtils;
 import org.elasticsearch.index.fielddata.AtomicOrdinalsFieldData;
 import org.elasticsearch.index.fielddata.IndexFieldData;
 import org.elasticsearch.index.fielddata.IndexGeoPointFieldData;
@@ -437,7 +438,9 @@ public void setDocument(int docId) {
                 for (int i = 0; i < count; ++i) {
                     final BytesRef value = bytesValues.valueAt(i);
                     script.setNextAggregationValue(value.utf8ToString());
-                    values[i].copyChars(script.run().toString());
+                    Object run = script.run();
+                    CollectionUtils.ensureNoSelfReferences(run);
+                    values[i].copyChars(run.toString());
                 }
                 sort();
             }

diff --git a/...src/main/java/org/elasticsearch/search/aggregations/support/values/ScriptBytesValues.java b/...src/main/java/org/elasticsearch/search/aggregations/support/values/ScriptBytesValues.java
@@ -20,6 +20,7 @@
 
 import org.apache.lucene.search.Scorer;
 import org.elasticsearch.common.lucene.ScorerAware;
+import org.elasticsearch.common.util.CollectionUtils;
 import org.elasticsearch.index.fielddata.SortedBinaryDocValues;
 import org.elasticsearch.index.fielddata.SortingBinaryDocValues;
 import org.elasticsearch.script.LeafSearchScript;
@@ -43,6 +44,7 @@ private void set(int i, Object o) {
         if (o == null) {
             values[i].clear();
         } else {
+            CollectionUtils.ensureNoSelfReferences(o);
             values[i].copyChars(o.toString());
         }
     }

diff --git a/core/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsFetchSubPhase.java b/core/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsFetchSubPhase.java
@@ -20,6 +20,7 @@
 
 import org.elasticsearch.script.LeafSearchScript;
 import org.elasticsearch.search.SearchHitField;
+import org.elasticsearch.common.util.CollectionUtils;
 import org.elasticsearch.search.fetch.FetchSubPhase;
 import org.elasticsearch.search.internal.SearchContext;
 
@@ -49,6 +50,7 @@ public void hitExecute(SearchContext context, HitContext hitContext) {
             final Object value;
             try {
                 value = leafScript.unwrap(leafScript.run());
+                CollectionUtils.ensureNoSelfReferences(value);
             } catch (RuntimeException e) {
                 if (scriptField.ignoreException()) {
                     continue;

diff --git a/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java
@@ -29,6 +29,7 @@
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
+import org.elasticsearch.common.util.CollectionUtils;
 import org.elasticsearch.common.xcontent.ConstructingObjectParser;
 import org.elasticsearch.common.xcontent.ObjectParser.ValueType;
 import org.elasticsearch.common.xcontent.XContentBuilder;
@@ -266,7 +267,9 @@ protected SortedBinaryDocValues getValues(LeafReaderContext context) throws IOEx
                             @Override
                             public BytesRef get(int docID) {
                                 leafScript.setDocument(docID);
-                                spare.copyChars(leafScript.run().toString());
+                                final Object run = leafScript.run();
+                                CollectionUtils.ensureNoSelfReferences(run);
+                                spare.copyChars(run.toString());
                                 return spare.get();
                             }
                         };

diff --git a/core/src/test/java/org/elasticsearch/common/util/CollectionUtilsTests.java b/core/src/test/java/org/elasticsearch/common/util/CollectionUtilsTests.java
@@ -25,16 +25,21 @@
 import org.apache.lucene.util.Counter;
 import org.elasticsearch.test.ESTestCase;
 
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 import java.util.SortedSet;
 import java.util.TreeSet;
 
+import static java.util.Collections.emptyMap;
 import static org.elasticsearch.common.util.CollectionUtils.eagerPartition;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 
@@ -176,4 +181,15 @@ public void testPerfectPartition() {
                 eagerPartition(Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12), 6)
         );
     }
+
+    public void testEnsureNoSelfReferences() {
+        CollectionUtils.ensureNoSelfReferences(emptyMap());
+        CollectionUtils.ensureNoSelfReferences(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"));
+    }
 }
diff --git a/core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java b/core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java
@@ -34,6 +34,7 @@
 import org.elasticsearch.common.io.PathUtils;
 import org.elasticsearch.common.text.Text;
 import org.elasticsearch.common.unit.DistanceUnit;
+import org.elasticsearch.common.util.CollectionUtils;
 import org.elasticsearch.common.xcontent.XContentParser.Token;
 import org.elasticsearch.test.ESTestCase;
 import org.hamcrest.Matcher;
@@ -853,19 +854,19 @@ public void testEnsureNotNull() {
     }
 
     public void testEnsureNoSelfReferences() throws IOException {
-        XContentBuilder.ensureNoSelfReferences(emptyMap());
-        XContentBuilder.ensureNoSelfReferences(null);
+        CollectionUtils.ensureNoSelfReferences(emptyMap());
+        CollectionUtils.ensureNoSelfReferences(null);
 
         Map<String, Object> map = new HashMap<>();
         map.put("field", map);
 
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder().map(map));
-        assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
+        assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
     }
 
     /**
      * Test that the same map written multiple times do not trigger the self-reference check in
-     * {@link XContentBuilder#ensureNoSelfReferences(Object)}
+     * {@link CollectionUtils#ensureNoSelfReferences(Object)}
      */
     public void testRepeatedMapsAndNoSelfReferences() throws Exception {
         Map<String, Object> mapB = singletonMap("b", "B");
@@ -898,7 +899,7 @@ public void testSelfReferencingMapsOneLevel() throws IOException {
         map1.put("map0", map0); // map 1 -> map 0 loop
 
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder().map(map0));
-        assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
+        assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
     }
 
     public void testSelfReferencingMapsTwoLevels() throws IOException {
@@ -916,7 +917,7 @@ public void testSelfReferencingMapsTwoLevels() throws IOException {
         map2.put("map0", map0); // map 2 -> map 0 loop
 
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder().map(map0));
-        assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
+        assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
     }
 
     public void testSelfReferencingObjectsArray() throws IOException {
@@ -929,13 +930,13 @@ public void testSelfReferencingObjectsArray() throws IOException {
                 .startObject()
                 .field("field", values)
                 .endObject());
-        assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
+        assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
 
         e = expectThrows(IllegalArgumentException.class, () -> builder()
                 .startObject()
                 .array("field", values)
                 .endObject());
-        assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
+        assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
     }
 
     public void testSelfReferencingIterable() throws IOException {
@@ -948,7 +949,7 @@ public void testSelfReferencingIterable() throws IOException {
                 .startObject()
                 .field("field", (Iterable) values)
                 .endObject());
-        assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
+        assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
     }
 
     public void testSelfReferencingIterableOneLevel() throws IOException {
@@ -963,7 +964,7 @@ public void testSelfReferencingIterableOneLevel() throws IOException {
                 .startObject()
                 .field("field", (Iterable) values)
                 .endObject());
-        assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
+        assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
     }
 
     public void testSelfReferencingIterableTwoLevels() throws IOException {
@@ -983,7 +984,7 @@ public void testSelfReferencingIterableTwoLevels() throws IOException {
         map2.put("map0", map0); // map 2 -> map 0 loop
 
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder().map(map0));
-        assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
+        assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
     }
 
     public void testNamedObject() throws IOException {

diff --git a/...stache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.java b/...stache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.java
@@ -20,6 +20,7 @@
 package org.elasticsearch.script.mustache;
 
 import com.github.mustachejava.reflect.ReflectionObjectHandler;
+import org.elasticsearch.common.util.CollectionUtils;
 import org.elasticsearch.common.util.iterable.Iterables;
 
 import java.lang.reflect.Array;
@@ -154,4 +155,9 @@ public Iterator<Object> iterator() {
         }
     }
 
+    @Override
+    public String stringify(Object object) {
+        CollectionUtils.ensureNoSelfReferences(object);
+        return super.stringify(object);
+    }
 }
diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/15_update.yaml b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/15_update.yaml
@@ -137,4 +137,4 @@
 
   - match: { error.root_cause.0.type: "remote_transport_exception" }
   - match: { error.type: "illegal_argument_exception" }
-  - match: { error.reason: "Object has already been built and is self-referencing itself" }
+  - match: { error.reason: "Iterable object is self-referencing itself" }