From e4c7d8183eb0d8db1c65ddd8f89171530639174a Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Tue, 11 Oct 2016 11:41:54 +0200 Subject: [PATCH] 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 --- .../common/xcontent/XContentBuilder.java | 44 ++++++ .../common/xcontent/BaseXContentTestCase.java | 137 ++++++++++++++++++ .../rest-api-spec/test/plan_a/15_update.yaml | 28 +++- .../test/plan_a/20_scriptfield.yaml | 20 ++- 4 files changed, 227 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java b/core/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java index c416aeffe39..df34ec726fd 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java +++ b/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 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 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/core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java b/core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java index 227918ff971..e1311483777 100644 --- a/core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java +++ b/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 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 mapB = singletonMap("b", "B"); + Map mapC = singletonMap("c", "C"); + Map mapD = singletonMap("d", "D"); + Map 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 map0 = new HashMap<>(); + Map 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 map0 = new HashMap<>(); + Map map1 = new HashMap<>(); + Map 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 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 map = new HashMap<>(); + map.put("foo", 0); + map.put("bar", 1); + + Iterable 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 map0 = new HashMap<>(); + Map map1 = new HashMap<>(); + Map map2 = new HashMap<>(); + + List 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")); diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/plan_a/15_update.yaml b/modules/lang-painless/src/test/resources/rest-api-spec/test/plan_a/15_update.yaml index 8e7e3d787e2..a031cea86e5 100644 --- a/modules/lang-painless/src/test/resources/rest-api-spec/test/plan_a/15_update.yaml +++ b/modules/lang-painless/src/test/resources/rest-api-spec/test/plan_a/15_update.yaml @@ -58,4 +58,30 @@ - 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" } diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/plan_a/20_scriptfield.yaml b/modules/lang-painless/src/test/resources/rest-api-spec/test/plan_a/20_scriptfield.yaml index b92012959d1..cf2e9eb4133 100644 --- a/modules/lang-painless/src/test/resources/rest-api-spec/test/plan_a/20_scriptfield.yaml +++ b/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." }