From 64418526187d4666f7001c79c5aac1f6cc35c8cc Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Wed, 9 Dec 2015 21:36:11 +0100 Subject: [PATCH] Fix OOM in AbstractXContentParser This commit fixes an OOM error that happens when the XContentParser.readList() method is asked to parse a single value instead of an array. It fixes the UpdateRequest parsing as well as remove some leniency in the readList() method so that it expect to be in an array before parsing values. closes #15338 --- .../action/update/UpdateRequest.java | 13 +++- .../support/AbstractXContentParser.java | 10 ++- .../action/update/UpdateRequestTests.java | 15 ++++ .../common/xcontent/XContentParserTests.java | 78 +++++++++++++++++++ 4 files changed, 112 insertions(+), 4 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java diff --git a/core/src/main/java/org/elasticsearch/action/update/UpdateRequest.java b/core/src/main/java/org/elasticsearch/action/update/UpdateRequest.java index 14c127c0703..0877ea1c66b 100644 --- a/core/src/main/java/org/elasticsearch/action/update/UpdateRequest.java +++ b/core/src/main/java/org/elasticsearch/action/update/UpdateRequest.java @@ -44,6 +44,7 @@ import org.elasticsearch.script.ScriptService; import org.elasticsearch.script.ScriptService.ScriptType; import java.io.IOException; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -671,9 +672,15 @@ public class UpdateRequest extends InstanceShardOperationRequest } else if ("detect_noop".equals(currentFieldName)) { detectNoop(parser.booleanValue()); } else if ("fields".equals(currentFieldName)) { - List values = parser.list(); - String[] fields = values.toArray(new String[values.size()]); - fields(fields); + List fields = null; + if (token == XContentParser.Token.START_ARRAY) { + fields = (List) parser.list(); + } else if (token.isValue()) { + fields = Collections.singletonList(parser.text()); + } + if (fields != null) { + fields(fields.toArray(new String[fields.size()])); + } } else { //here we don't have settings available, unable to throw deprecation exceptions scriptParameterParser.token(currentFieldName, token, parser, ParseFieldMatcher.EMPTY); diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/support/AbstractXContentParser.java b/core/src/main/java/org/elasticsearch/common/xcontent/support/AbstractXContentParser.java index fd4aa782302..e994b81832f 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/support/AbstractXContentParser.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/support/AbstractXContentParser.java @@ -20,6 +20,7 @@ package org.elasticsearch.common.xcontent.support; import org.apache.lucene.util.BytesRef; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.Booleans; import org.elasticsearch.common.ParseFieldMatcher; import org.elasticsearch.common.xcontent.XContentParser; @@ -286,14 +287,21 @@ public abstract class AbstractXContentParser implements XContentParser { static List readList(XContentParser parser, MapFactory mapFactory) throws IOException { XContentParser.Token token = parser.currentToken(); + if (token == null) { + token = parser.nextToken(); + } if (token == XContentParser.Token.FIELD_NAME) { token = parser.nextToken(); } if (token == XContentParser.Token.START_ARRAY) { token = parser.nextToken(); + } else { + throw new ElasticsearchParseException("Failed to parse list: expecting " + + XContentParser.Token.START_ARRAY + " but got " + token); } + ArrayList list = new ArrayList<>(); - for (; token != XContentParser.Token.END_ARRAY; token = parser.nextToken()) { + for (; token != null && token != XContentParser.Token.END_ARRAY; token = parser.nextToken()) { list.add(readValue(parser, mapFactory, token)); } return list; diff --git a/core/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java b/core/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java index bcb26613388..d105a4bf63b 100644 --- a/core/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java +++ b/core/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java @@ -22,6 +22,7 @@ package org.elasticsearch.action.update; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.Version; import org.elasticsearch.action.index.IndexRequest; +import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.io.stream.Streamable; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; @@ -36,6 +37,7 @@ import org.elasticsearch.test.ESTestCase; import java.util.Map; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; +import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; @@ -179,4 +181,17 @@ public class UpdateRequestTests extends ESTestCase { assertThat(e.getMessage(), equalTo("Failed to derive xcontent")); } } + + // Related to issue 15338 + public void testFieldsParsing() throws Exception { + UpdateRequest request = new UpdateRequest("test", "type1", "1") + .source(new BytesArray("{\"doc\": {\"field1\": \"value1\"}, \"fields\": \"_source\"}")); + assertThat(request.doc().sourceAsMap().get("field1").toString(), equalTo("value1")); + assertThat(request.fields(), arrayContaining("_source")); + + request = new UpdateRequest("test", "type2", "2") + .source(new BytesArray("{\"doc\": {\"field2\": \"value2\"}, \"fields\": [\"field1\", \"field2\"]}")); + assertThat(request.doc().sourceAsMap().get("field2").toString(), equalTo("value2")); + assertThat(request.fields(), arrayContaining("field1", "field2")); + } } diff --git a/core/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java b/core/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java new file mode 100644 index 00000000000..cce349f417c --- /dev/null +++ b/core/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java @@ -0,0 +1,78 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.common.xcontent; + +import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.test.ESTestCase; + +import java.io.IOException; +import java.util.List; + +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.nullValue; + +public class XContentParserTests extends ESTestCase { + + public void testReadList() throws IOException { + assertThat(readList("{\"foo\": [\"bar\"]}"), contains("bar")); + assertThat(readList("{\"foo\": [\"bar\",\"baz\"]}"), contains("bar", "baz")); + assertThat(readList("{\"foo\": [1, 2, 3], \"bar\": 4}"), contains(1, 2, 3)); + assertThat(readList("{\"foo\": [{\"bar\":1},{\"baz\":2},{\"qux\":3}]}"), hasSize(3)); + assertThat(readList("{\"foo\": [null]}"), contains(nullValue())); + assertThat(readList("{\"foo\": []}"), hasSize(0)); + assertThat(readList("{\"foo\": [1]}"), contains(1)); + assertThat(readList("{\"foo\": [1,2]}"), contains(1, 2)); + assertThat(readList("{\"foo\": [{},{},{},{}]}"), hasSize(4)); + } + + public void testReadListThrowsException() throws IOException { + // Calling XContentParser.list() or listOrderedMap() to read a simple + // value or object should throw an exception + assertReadListThrowsException("{\"foo\": \"bar\"}"); + assertReadListThrowsException("{\"foo\": 1, \"bar\": 2}"); + assertReadListThrowsException("{\"foo\": {\"bar\":\"baz\"}}"); + } + + @SuppressWarnings("unchecked") + private static List readList(String source) throws IOException { + try (XContentParser parser = XContentType.JSON.xContent().createParser(source)) { + XContentParser.Token token = parser.nextToken(); + assertThat(token, equalTo(XContentParser.Token.START_OBJECT)); + token = parser.nextToken(); + assertThat(token, equalTo(XContentParser.Token.FIELD_NAME)); + assertThat(parser.currentName(), equalTo("foo")); + return (List) (randomBoolean() ? parser.listOrderedMap() : parser.list()); + } + } + + private void assertReadListThrowsException(String source) { + try { + readList(source); + fail("should have thrown a parse exception"); + } catch (Exception e) { + assertThat(e, instanceOf(ElasticsearchParseException.class)); + assertThat(e.getMessage(), containsString("Failed to parse list")); + } + } +}