Skip to content

Commit 04d1489

Browse files
s1monws1monw
committed
Ensure we protect Collections obtained from scripts from self-referencing (#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.
1 parent e1b2009 commit 04d1489

File tree

13 files changed

+129
-48
lines changed

13 files changed

+129
-48
lines changed

‎core/src/main/java/org/elasticsearch/common/util/CollectionUtils.java‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,20 @@
1919

2020
package org.elasticsearch.common.util;
2121

22+
import java.nio.file.Path;
2223
import java.util.AbstractList;
2324
import java.util.ArrayList;
2425
import java.util.Arrays;
2526
import java.util.Collection;
2627
import java.util.Collections;
2728
import java.util.Comparator;
29+
import java.util.IdentityHashMap;
2830
import java.util.LinkedList;
2931
import java.util.List;
32+
import java.util.Map;
3033
import java.util.Objects;
3134
import java.util.RandomAccess;
35+
import java.util.Set;
3236

3337
import com.carrotsearch.hppc.ObjectArrayList;
3438
import org.apache.lucene.util.BytesRef;
@@ -221,6 +225,40 @@ public static int[] toArray(Collection<Integer> ints) {
221225
return ints.stream().mapToInt(s -> s).toArray();
222226
}
223227

228+
public static void ensureNoSelfReferences(Object value) {
229+
Iterable<?> it = convert(value);
230+
if (it != null) {
231+
ensureNoSelfReferences(it, value, Collections.newSetFromMap(new IdentityHashMap<>()));
232+
}
233+
}
234+
235+
private static Iterable<?> convert(Object value) {
236+
if (value == null) {
237+
return null;
238+
}
239+
if (value instanceof Map) {
240+
return ((Map<?,?>) value).values();
241+
} else if ((value instanceof Iterable) && (value instanceof Path == false)) {
242+
return (Iterable<?>) value;
243+
} else if (value instanceof Object[]) {
244+
return Arrays.asList((Object[]) value);
245+
} else {
246+
return null;
247+
}
248+
}
249+
250+
private static void ensureNoSelfReferences(final Iterable<?> value, Object originalReference, final Set<Object> ancestors) {
251+
if (value != null) {
252+
if (ancestors.add(originalReference) == false) {
253+
throw new IllegalArgumentException("Iterable object is self-referencing itself");
254+
}
255+
for (Object o : value) {
256+
ensureNoSelfReferences(convert(o), o, ancestors);
257+
}
258+
ancestors.remove(originalReference);
259+
}
260+
}
261+
224262
private static class RotatedList<T> extends AbstractList<T> implements RandomAccess {
225263

226264
private final List<T> in;

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

Lines changed: 4 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.elasticsearch.common.text.Text;
2929
import org.elasticsearch.common.unit.ByteSizeValue;
3030
import org.elasticsearch.common.unit.TimeValue;
31+
import org.elasticsearch.common.util.CollectionUtils;
3132
import org.joda.time.DateTimeZone;
3233
import org.joda.time.ReadableInstant;
3334
import org.joda.time.format.DateTimeFormatter;
@@ -38,12 +39,10 @@
3839
import java.io.InputStream;
3940
import java.io.OutputStream;
4041
import java.nio.file.Path;
41-
import java.util.Arrays;
4242
import java.util.Calendar;
4343
import java.util.Collections;
4444
import java.util.Date;
4545
import java.util.HashMap;
46-
import java.util.IdentityHashMap;
4746
import java.util.Locale;
4847
import java.util.Map;
4948
import java.util.Objects;
@@ -783,7 +782,7 @@ XContentBuilder values(Object[] values) throws IOException {
783782

784783
// checks that the array of object does not contain references to itself because
785784
// iterating over entries will cause a stackoverflow error
786-
ensureNoSelfReferences(values);
785+
CollectionUtils.ensureNoSelfReferences(values);
787786

788787
startArray();
789788
for (Object o : values) {
@@ -869,7 +868,7 @@ public XContentBuilder map(Map<String, ?> values) throws IOException {
869868

870869
// checks that the map does not contain references to itself because
871870
// iterating over map entries will cause a stackoverflow error
872-
ensureNoSelfReferences(values);
871+
CollectionUtils.ensureNoSelfReferences(values);
873872

874873
startObject();
875874
for (Map.Entry<String, ?> value : values.entrySet()) {
@@ -895,7 +894,7 @@ private XContentBuilder value(Iterable<?> values) throws IOException {
895894
} else {
896895
// checks that the iterable does not contain references to itself because
897896
// iterating over entries will cause a stackoverflow error
898-
ensureNoSelfReferences(values);
897+
CollectionUtils.ensureNoSelfReferences(values);
899898

900899
startArray();
901900
for (Object value : values) {
@@ -1066,32 +1065,4 @@ static void ensureNotNull(Object value, String message) {
10661065
throw new IllegalArgumentException(message);
10671066
}
10681067
}
1069-
1070-
static void ensureNoSelfReferences(Object value) {
1071-
ensureNoSelfReferences(value, Collections.newSetFromMap(new IdentityHashMap<>()));
1072-
}
1073-
1074-
private static void ensureNoSelfReferences(final Object value, final Set<Object> ancestors) {
1075-
if (value != null) {
1076-
1077-
Iterable<?> it;
1078-
if (value instanceof Map) {
1079-
it = ((Map) value).values();
1080-
} else if ((value instanceof Iterable) && (value instanceof Path == false)) {
1081-
it = (Iterable) value;
1082-
} else if (value instanceof Object[]) {
1083-
it = Arrays.asList((Object[]) value);
1084-
} else {
1085-
return;
1086-
}
1087-
1088-
if (ancestors.add(value) == false) {
1089-
throw new IllegalArgumentException("Object has already been built and is self-referencing itself");
1090-
}
1091-
for (Object o : it) {
1092-
ensureNoSelfReferences(o, ancestors);
1093-
}
1094-
ancestors.remove(value);
1095-
}
1096-
}
10971068
}

‎core/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregator.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
package org.elasticsearch.search.aggregations.metrics.scripted;
2121

2222
import org.apache.lucene.index.LeafReaderContext;
23+
import org.elasticsearch.common.util.CollectionUtils;
2324
import org.elasticsearch.script.ExecutableScript;
2425
import org.elasticsearch.script.Script;
2526
import org.elasticsearch.script.SearchScript;
@@ -77,6 +78,7 @@ public InternalAggregation buildAggregation(long owningBucketOrdinal) {
7778
Object aggregation;
7879
if (combineScript != null) {
7980
aggregation = combineScript.run();
81+
CollectionUtils.ensureNoSelfReferences(aggregation);
8082
} else {
8183
aggregation = params.get("_agg");
8284
}

‎core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,10 +112,11 @@ public InternalAggregation reduce(InternalAggregation aggregation, ReduceContext
112112
} else {
113113
ExecutableScript executableScript = factory.newInstance(vars);
114114
Object returned = executableScript.run();
115+
// no need to check for self references since only numbers are valid
115116
if (returned == null) {
116117
newBuckets.add(bucket);
117118
} else {
118-
if (!(returned instanceof Number)) {
119+
if ((returned instanceof Number) == false) {
119120
throw new AggregationExecutionException("series_arithmetic script for reducer [" + name()
120121
+ "] must return a Number");
121122
}

‎core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import org.apache.lucene.util.Bits;
3030
import org.apache.lucene.util.BytesRef;
3131
import org.elasticsearch.common.lucene.ScorerAware;
32+
import org.elasticsearch.common.util.CollectionUtils;
3233
import org.elasticsearch.index.fielddata.AbstractSortingNumericDocValues;
3334
import org.elasticsearch.index.fielddata.AtomicOrdinalsFieldData;
3435
import org.elasticsearch.index.fielddata.IndexFieldData;
@@ -437,7 +438,9 @@ public boolean advanceExact(int doc) throws IOException {
437438
for (int i = 0; i < count; ++i) {
438439
final BytesRef value = bytesValues.nextValue();
439440
script.setNextAggregationValue(value.utf8ToString());
440-
values[i].copyChars(script.run().toString());
441+
Object run = script.run();
442+
CollectionUtils.ensureNoSelfReferences(run);
443+
values[i].copyChars(run.toString());
441444
}
442445
sort();
443446
return true;

‎core/src/main/java/org/elasticsearch/search/aggregations/support/values/ScriptBytesValues.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import org.apache.lucene.search.Scorer;
2222
import org.elasticsearch.common.lucene.ScorerAware;
23+
import org.elasticsearch.common.util.CollectionUtils;
2324
import org.elasticsearch.index.fielddata.SortedBinaryDocValues;
2425
import org.elasticsearch.index.fielddata.SortingBinaryDocValues;
2526
import org.elasticsearch.script.SearchScript;
@@ -44,6 +45,7 @@ private void set(int i, Object o) {
4445
if (o == null) {
4546
values[i].clear();
4647
} else {
48+
CollectionUtils.ensureNoSelfReferences(o);
4749
values[i].copyChars(o.toString());
4850
}
4951
}

‎core/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsFetchSubPhase.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import org.apache.lucene.index.LeafReaderContext;
2323
import org.apache.lucene.index.ReaderUtil;
2424
import org.elasticsearch.common.document.DocumentField;
25+
import org.elasticsearch.common.util.CollectionUtils;
2526
import org.elasticsearch.script.SearchScript;
2627
import org.elasticsearch.search.SearchHit;
2728
import org.elasticsearch.search.fetch.FetchSubPhase;
@@ -64,6 +65,7 @@ public void hitsExecute(SearchContext context, SearchHit[] hits) throws IOExcept
6465
final Object value;
6566
try {
6667
value = leafScripts[i].run();
68+
CollectionUtils.ensureNoSelfReferences(value);
6769
} catch (RuntimeException e) {
6870
if (scriptFields.get(i).ignoreException()) {
6971
continue;

‎core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import org.elasticsearch.common.io.stream.Writeable;
3333
import org.elasticsearch.common.logging.DeprecationLogger;
3434
import org.elasticsearch.common.logging.Loggers;
35+
import org.elasticsearch.common.util.CollectionUtils;
3536
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
3637
import org.elasticsearch.common.xcontent.ObjectParser.ValueType;
3738
import org.elasticsearch.common.xcontent.XContentBuilder;
@@ -341,7 +342,9 @@ public boolean advanceExact(int doc) throws IOException {
341342
}
342343
@Override
343344
public BytesRef binaryValue() {
344-
spare.copyChars(leafScript.run().toString());
345+
final Object run = leafScript.run();
346+
CollectionUtils.ensureNoSelfReferences(run);
347+
spare.copyChars(run.toString());
345348
return spare.get();
346349
}
347350
};

‎core/src/test/java/org/elasticsearch/common/util/CollectionUtilsTests.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,16 +25,21 @@
2525
import org.apache.lucene.util.Counter;
2626
import org.elasticsearch.test.ESTestCase;
2727

28+
import java.io.IOException;
2829
import java.util.ArrayList;
2930
import java.util.Arrays;
3031
import java.util.Collections;
32+
import java.util.HashMap;
3133
import java.util.HashSet;
3234
import java.util.Iterator;
3335
import java.util.List;
36+
import java.util.Map;
3437
import java.util.SortedSet;
3538
import java.util.TreeSet;
3639

40+
import static java.util.Collections.emptyMap;
3741
import static org.elasticsearch.common.util.CollectionUtils.eagerPartition;
42+
import static org.hamcrest.Matchers.containsString;
3843
import static org.hamcrest.Matchers.equalTo;
3944
import static org.hamcrest.Matchers.is;
4045

@@ -176,4 +181,15 @@ public void testPerfectPartition() {
176181
eagerPartition(Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12), 6)
177182
);
178183
}
184+
185+
public void testEnsureNoSelfReferences() {
186+
CollectionUtils.ensureNoSelfReferences(emptyMap());
187+
CollectionUtils.ensureNoSelfReferences(null);
188+
189+
Map<String, Object> map = new HashMap<>();
190+
map.put("field", map);
191+
192+
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> CollectionUtils.ensureNoSelfReferences(map));
193+
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
194+
}
179195
}

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

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import org.elasticsearch.common.io.PathUtils;
3636
import org.elasticsearch.common.text.Text;
3737
import org.elasticsearch.common.unit.DistanceUnit;
38+
import org.elasticsearch.common.util.CollectionUtils;
3839
import org.elasticsearch.common.xcontent.XContentParser.Token;
3940
import org.elasticsearch.test.ESTestCase;
4041
import org.hamcrest.Matcher;
@@ -855,19 +856,19 @@ public void testEnsureNotNull() {
855856
}
856857

857858
public void testEnsureNoSelfReferences() throws IOException {
858-
XContentBuilder.ensureNoSelfReferences(emptyMap());
859-
XContentBuilder.ensureNoSelfReferences(null);
859+
CollectionUtils.ensureNoSelfReferences(emptyMap());
860+
CollectionUtils.ensureNoSelfReferences(null);
860861

861862
Map<String, Object> map = new HashMap<>();
862863
map.put("field", map);
863864

864865
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder().map(map));
865-
assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
866+
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
866867
}
867868

868869
/**
869870
* Test that the same map written multiple times do not trigger the self-reference check in
870-
* {@link XContentBuilder#ensureNoSelfReferences(Object)}
871+
* {@link CollectionUtils#ensureNoSelfReferences(Object)}
871872
*/
872873
public void testRepeatedMapsAndNoSelfReferences() throws Exception {
873874
Map<String, Object> mapB = singletonMap("b", "B");
@@ -900,7 +901,7 @@ public void testSelfReferencingMapsOneLevel() throws IOException {
900901
map1.put("map0", map0); // map 1 -> map 0 loop
901902

902903
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder().map(map0));
903-
assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
904+
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
904905
}
905906

906907
public void testSelfReferencingMapsTwoLevels() throws IOException {
@@ -918,7 +919,7 @@ public void testSelfReferencingMapsTwoLevels() throws IOException {
918919
map2.put("map0", map0); // map 2 -> map 0 loop
919920

920921
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder().map(map0));
921-
assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
922+
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
922923
}
923924

924925
public void testSelfReferencingObjectsArray() throws IOException {
@@ -931,13 +932,13 @@ public void testSelfReferencingObjectsArray() throws IOException {
931932
.startObject()
932933
.field("field", values)
933934
.endObject());
934-
assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
935+
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
935936

936937
e = expectThrows(IllegalArgumentException.class, () -> builder()
937938
.startObject()
938939
.array("field", values)
939940
.endObject());
940-
assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
941+
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
941942
}
942943

943944
public void testSelfReferencingIterable() throws IOException {
@@ -950,7 +951,7 @@ public void testSelfReferencingIterable() throws IOException {
950951
.startObject()
951952
.field("field", (Iterable) values)
952953
.endObject());
953-
assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
954+
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
954955
}
955956

956957
public void testSelfReferencingIterableOneLevel() throws IOException {
@@ -965,7 +966,7 @@ public void testSelfReferencingIterableOneLevel() throws IOException {
965966
.startObject()
966967
.field("field", (Iterable) values)
967968
.endObject());
968-
assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
969+
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
969970
}
970971

971972
public void testSelfReferencingIterableTwoLevels() throws IOException {
@@ -985,7 +986,7 @@ public void testSelfReferencingIterableTwoLevels() throws IOException {
985986
map2.put("map0", map0); // map 2 -> map 0 loop
986987

987988
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder().map(map0));
988-
assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
989+
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself"));
989990
}
990991

991992
public void testChecksForDuplicates() throws Exception {

0 commit comments

Comments
 (0)