Skip to content

Commit

Permalink
Check object depth limit at parse time (#90285)
Browse files Browse the repository at this point in the history
Helps avoiding stack overflow errors when the total field limit is set too high.

Relates #87926
  • Loading branch information
ywelsch authored Sep 26, 2022
1 parent 4134eee commit dae717a
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down

0 comments on commit dae717a

Please sign in to comment.