From dae717ae8d40509e8105e78ede8f84f887078b50 Mon Sep 17 00:00:00 2001 From: Yannick Welsch Date: Mon, 26 Sep 2022 11:38:04 +0200 Subject: [PATCH] Check object depth limit at parse time (#90285) Helps avoiding stack overflow errors when the total field limit is set too high. Relates #87926 --- .../index/mapper/DynamicMappingIT.java | 39 +++++++++++++++---- .../index/mapper/DocumentParserContext.java | 4 ++ .../index/mapper/MappingLookup.java | 26 +++++++------ .../index/mapper/DocumentParserTests.java | 28 +++++++++++++ 4 files changed, 78 insertions(+), 19 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java b/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java index d31475a172056..c5d262a088e53 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java @@ -22,6 +22,7 @@ import org.elasticsearch.cluster.metadata.MappingMetadata; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Randomness; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.core.TimeValue; @@ -45,7 +46,7 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicReference; -import static org.elasticsearch.index.mapper.MapperService.INDEX_MAPPING_DEPTH_LIMIT_SETTING; +import static org.elasticsearch.index.mapper.MapperService.INDEX_MAPPING_NESTED_FIELDS_LIMIT_SETTING; import static org.elasticsearch.index.mapper.MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchHits; @@ -137,12 +138,34 @@ public void run() { } } - public void testPreflightCheckAvoidsMaster() throws InterruptedException { - // can't use INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING as a check here, as that is already checked at parse time, - // see testTotalFieldsLimitForDynamicMappingsUpdateCheckedAtDocumentParseTime - createIndex("index", Settings.builder().put(INDEX_MAPPING_DEPTH_LIMIT_SETTING.getKey(), 2).build()); + public void testPreflightCheckAvoidsMaster() throws InterruptedException, IOException { + // can't use INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING nor INDEX_MAPPING_DEPTH_LIMIT_SETTING as a check here, as that is already + // checked at parse time, see testTotalFieldsLimitForDynamicMappingsUpdateCheckedAtDocumentParseTime + createIndex("index", Settings.builder().put(INDEX_MAPPING_NESTED_FIELDS_LIMIT_SETTING.getKey(), 2).build()); ensureGreen("index"); - client().prepareIndex("index").setId("1").setSource("field1", Map.of("field2", "value1")).get(); + client().admin() + .indices() + .preparePutMapping("index") + .setSource( + Strings.toString( + XContentFactory.jsonBuilder() + .startObject() + .startArray("dynamic_templates") + .startObject() + .startObject("test") + .field("match", "nested*") + .startObject("mapping") + .field("type", "nested") + .endObject() + .endObject() + .endObject() + .endArray() + .endObject() + ), + XContentType.JSON + ) + .get(); + client().prepareIndex("index").setId("1").setSource("nested1", Map.of("foo", "bar"), "nested2", Map.of("foo", "bar")).get(); final CountDownLatch masterBlockedLatch = new CountDownLatch(1); final CountDownLatch indexingCompletedLatch = new CountDownLatch(1); @@ -165,11 +188,11 @@ public void onFailure(Exception e) { masterBlockedLatch.await(); final IndexRequestBuilder indexRequestBuilder = client().prepareIndex("index") .setId("2") - .setSource("field1", Map.of("field3", Map.of("field4", "value2"))); + .setSource("nested3", Map.of("foo", "bar")); try { assertThat( expectThrows(IllegalArgumentException.class, () -> indexRequestBuilder.get(TimeValue.timeValueSeconds(10))).getMessage(), - Matchers.containsString("Limit of mapping depth [2] has been exceeded due to object field [field1.field3]") + Matchers.containsString("Limit of nested fields [2] has been exceeded") ); } finally { indexingCompletedLatch.countDown(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java index a79bc9003ef75..b2e8990144170 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java @@ -226,6 +226,10 @@ public final String documentDescription() { * Add a new mapper dynamically created while parsing. */ public final void addDynamicMapper(Mapper mapper) { + // eagerly check object depth limit here to avoid stack overflow errors + if (mapper instanceof ObjectMapper) { + MappingLookup.checkObjectDepthLimit(indexSettings().getMappingDepthLimit(), mapper.name()); + } // eagerly check field name limit here to avoid OOM errors // only check fields that are not already mapped or tracked in order to avoid hitting field limit too early via double-counting // note that existing fields can also receive dynamic mapping updates (e.g. constant_keyword to fix the value) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java index 6cbb903a6251f..e112a676ad38d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java @@ -294,19 +294,23 @@ private void checkDimensionFieldLimit(long limit) { private void checkObjectDepthLimit(long limit) { for (String objectPath : objectMappers.keySet()) { - int numDots = 0; - for (int i = 0; i < objectPath.length(); ++i) { - if (objectPath.charAt(i) == '.') { - numDots += 1; - } - } - final int depth = numDots + 2; - if (depth > limit) { - throw new IllegalArgumentException( - "Limit of mapping depth [" + limit + "] has been exceeded due to object field [" + objectPath + "]" - ); + checkObjectDepthLimit(limit, objectPath); + } + } + + static void checkObjectDepthLimit(long limit, String objectPath) { + int numDots = 0; + for (int i = 0; i < objectPath.length(); ++i) { + if (objectPath.charAt(i) == '.') { + numDots += 1; } } + final int depth = numDots + 2; + if (depth > limit) { + throw new IllegalArgumentException( + "Limit of mapping depth [" + limit + "] has been exceeded due to object field [" + objectPath + "]" + ); + } } private void checkFieldNameLengthLimit(long limit) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java index e72add443eafc..17ebe2d153cc3 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -2407,6 +2407,34 @@ same name need to be part of the same mappings (hence the same document). If th assertArrayEquals(new long[] { 10, 10 }, longs2); } + public void testDeeplyNestedDocument() throws Exception { + int depth = 10000; + + DocumentMapper docMapper = createMapperService(Settings.builder().put(getIndexSettings()).build(), mapping(b -> {})) + .documentMapper(); + // hits the mapping object depth limit (defaults to 20) + MapperParsingException mpe = expectThrows(MapperParsingException.class, () -> docMapper.parse(source(b -> { + for (int i = 0; i < depth; i++) { + b.startObject("obj"); + } + b.field("foo", 10); + for (int i = 0; i < depth; i++) { + b.endObject(); + } + }))); + assertThat(mpe.getCause().getMessage(), containsString("Limit of mapping depth [20] has been exceeded due to object field")); + + // check that multiple-dotted field name underneath an object mapper with subobjects=false does not trigger this + DocumentMapper docMapper2 = createMapperService(topMapping(b -> { b.field("subobjects", false); })).documentMapper(); + + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < depth; i++) { + sb.append("obj."); + } + sb.append("foo"); + docMapper2.parse(source(b -> { b.field(sb.toString(), 10); })); + } + /** * Mapper plugin providing a mock metadata field mapper implementation that supports setting its value */