XContentBuilder: Avoid building self-referencing objects (#20550)

Some objects like maps, iterables or arrays of objects can self-reference themselves. This is mostly due to a bug in code but the XContentBuilder should be able to detect such situations and throws an IllegalArgumentException instead of building objects over and over until a stackoverflow occurs.

closes #20540
closes #19475

core/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

@@ -38,10 +38,12 @@ import java.io.IOException;
 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;
@@ -778,6 +780,11 @@ public final class XContentBuilder implements BytesStream, Releasable, Flushable
         if (values == null) {
             return nullValue();
         }
+
+        // checks that the array of object does not contain references to itself because
+        // iterating over entries will cause a stackoverflow error
+        ensureNoSelfReferences(values);
+
         startArray();
         for (Object o : values) {
             value(o);
@@ -859,6 +866,11 @@ public final class XContentBuilder implements BytesStream, Releasable, Flushable
         if (values == null) {
             return nullValue();
         }
+
+        // checks that the map does not contain references to itself because
+        // iterating over map entries will cause a stackoverflow error
+        ensureNoSelfReferences(values);
+
         startObject();
         for (Map.Entry<String, ?> value : values.entrySet()) {
             field(value.getKey());
@@ -881,6 +893,10 @@ public final class XContentBuilder implements BytesStream, Releasable, Flushable
             //treat as single value
             value((Path) values);
         } else {
+            // checks that the iterable does not contain references to itself because
+            // iterating over entries will cause a stackoverflow error
+            ensureNoSelfReferences(values);
+
             startArray();
             for (Object value : values) {
                 unknownValue(value);
@@ -1012,4 +1028,32 @@ public final class XContentBuilder implements BytesStream, Releasable, Flushable
             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);
+        }
+    }
 }

core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java

@@ -47,15 +47,18 @@ import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.math.BigInteger;
 import java.nio.file.Path;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Calendar;
 import java.util.Collections;
 import java.util.Date;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 
+import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonMap;
 import static org.hamcrest.Matchers.allOf;
 import static org.hamcrest.Matchers.containsString;
@@ -846,6 +849,140 @@ public abstract class BaseXContentTestCase extends ESTestCase {
         XContentBuilder.ensureNotNull("foo", "No exception must be thrown");
     }
 
+    public void testEnsureNoSelfReferences() throws IOException {
+        XContentBuilder.ensureNoSelfReferences(emptyMap());
+        XContentBuilder.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"));
+    }
+
+    /**
+     * Test that the same map written multiple times do not trigger the self-reference check in
+     * {@link XContentBuilder#ensureNoSelfReferences(Object)}
+     */
+    public void testRepeatedMapsAndNoSelfReferences() throws Exception {
+        Map<String, Object> mapB = singletonMap("b", "B");
+        Map<String, Object> mapC = singletonMap("c", "C");
+        Map<String, Object> mapD = singletonMap("d", "D");
+        Map<String, Object> mapA = new HashMap<>();
+        mapA.put("a", 0);
+        mapA.put("b1", mapB);
+        mapA.put("b2", mapB);
+        mapA.put("c", Arrays.asList(mapC, mapC));
+        mapA.put("d1", mapD);
+        mapA.put("d2", singletonMap("d3", mapD));
+
+        final String expected =
+                "{'map':{'b2':{'b':'B'},'a':0,'c':[{'c':'C'},{'c':'C'}],'d1':{'d':'D'},'d2':{'d3':{'d':'D'}},'b1':{'b':'B'}}}";
+
+        assertResult(expected, () -> builder().startObject().field("map", mapA).endObject());
+        assertResult(expected, () -> builder().startObject().field("map").value(mapA).endObject());
+        assertResult(expected, () -> builder().startObject().field("map").map(mapA).endObject());
+    }
+
+    public void testSelfReferencingMapsOneLevel() throws IOException {
+        Map<String, Object> map0 = new HashMap<>();
+        Map<String, Object> map1 = new HashMap<>();
+
+        map0.put("foo", 0);
+        map0.put("map1", map1); // map 0 -> map 1
+
+        map1.put("bar", 1);
+        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"));
+    }
+
+    public void testSelfReferencingMapsTwoLevels() throws IOException {
+        Map<String, Object> map0 = new HashMap<>();
+        Map<String, Object> map1 = new HashMap<>();
+        Map<String, Object> map2 = new HashMap<>();
+
+        map0.put("foo", 0);
+        map0.put("map1", map1); // map 0 -> map 1
+
+        map1.put("bar", 1);
+        map1.put("map2", map2); // map 1 -> map 2
+
+        map2.put("baz", 2);
+        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"));
+    }
+
+    public void testSelfReferencingObjectsArray() throws IOException {
+        Object[] values = new Object[3];
+        values[0] = 0;
+        values[1] = 1;
+        values[2] = values;
+
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder()
+                .startObject()
+                .field("field", values)
+                .endObject());
+        assertThat(e.getMessage(), containsString("Object has already been built and 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"));
+    }
+
+    public void testSelfReferencingIterable() throws IOException {
+        List<Object> values = new ArrayList<>();
+        values.add("foo");
+        values.add("bar");
+        values.add(values);
+
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder()
+                .startObject()
+                .field("field", (Iterable) values)
+                .endObject());
+        assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
+    }
+
+    public void testSelfReferencingIterableOneLevel() throws IOException {
+        Map<String, Object> map = new HashMap<>();
+        map.put("foo", 0);
+        map.put("bar", 1);
+
+        Iterable<Object> values = Arrays.asList("one", "two", map);
+        map.put("baz", values);
+
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder()
+                .startObject()
+                .field("field", (Iterable) values)
+                .endObject());
+        assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
+    }
+
+    public void testSelfReferencingIterableTwoLevels() throws IOException {
+        Map<String, Object> map0 = new HashMap<>();
+        Map<String, Object> map1 = new HashMap<>();
+        Map<String, Object> map2 = new HashMap<>();
+
+        List<Object> it1 = new ArrayList<>();
+
+        map0.put("foo", 0);
+        map0.put("it1", (Iterable<?>) it1); // map 0 -> it1
+
+        it1.add(map1);
+        it1.add(map2); // it 1 -> map 1, map 2
+
+        map2.put("baz", 2);
+        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"));
+    }
+
     private static void expectUnclosedException(ThrowingRunnable runnable) {
         IllegalStateException e = expectThrows(IllegalStateException.class, runnable);
         assertThat(e.getMessage(), containsString("Failed to close the XContentBuilder"));

modules/lang-painless/src/test/resources/rest-api-spec/test/plan_a/15_update.yaml

@@ -59,3 +59,29 @@
   - match: { _source.foo:        yyy }
   - match: { _source.count:      1   }
 
+---
+"Update Script with script error":
+  - do:
+      index:
+          index:  test_1
+          type:   test
+          id:     2
+          body:
+              foo:    bar
+              count:  1
+
+  - do:
+      catch: request
+      update:
+          index:  test_1
+          type:   test
+          id:     2
+          body:
+            script:
+              lang:   painless
+              inline: "for (def key : params.keySet()) { ctx._source[key] = params[key]}"
+              params: { bar: 'xxx' }
+
+  - 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" }

modules/lang-painless/src/test/resources/rest-api-spec/test/plan_a/20_scriptfield.yaml

@@ -20,7 +20,6 @@ setup:
         indices.refresh: {}
 
 ---
-
 "Scripted Field":
     - do:
         search:
@@ -34,3 +33,22 @@ setup:
                                 x: "bbb"
 
     - match: { hits.hits.0.fields.bar.0: "aaabbb"}
+
+---
+"Scripted Field with script error":
+    - do:
+        catch: request
+        search:
+          body:
+            script_fields:
+              bar:
+                script:
+                  lang: painless
+                  inline: "while (true) {}"
+
+    - match: { error.root_cause.0.type: "script_exception" }
+    - match: { error.root_cause.0.reason: "compile error" }
+    - match: { error.caused_by.type: "script_exception" }
+    - match: { error.caused_by.reason: "compile error" }
+    - match: { error.caused_by.caused_by.type: "illegal_argument_exception" }
+    - match: { error.caused_by.caused_by.reason: "While loop has no escape." }