From bdb7d58e3b40ae69387c0f39c7847b8934988b38 Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Fri, 7 Dec 2018 15:12:25 +0000 Subject: [PATCH 1/2] [ML] Prevent stack overflow while copying ML jobs and datafeeds ML jobs and datafeeds wrap collections into their unmodifiable equivalents in their constructor. However, the copying builder does not make a copy of some of those collections resulting in wrapping those again and again. This can eventually result to stack overflow. This commit addressed this issue by copying the collections in question in the copying builder constructor. Closes #36360 --- .../client/ml/datafeed/DatafeedConfig.java | 6 +++--- .../client/ml/job/config/Job.java | 6 ++++-- .../core/ml/datafeed/DatafeedConfig.java | 19 +++++++++---------- .../xpack/core/ml/job/config/Job.java | 5 +++-- .../core/ml/datafeed/DatafeedConfigTests.java | 8 +++++++- .../xpack/core/ml/job/config/JobTests.java | 7 +++++++ 6 files changed, 33 insertions(+), 18 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/datafeed/DatafeedConfig.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/datafeed/DatafeedConfig.java index 4b9bc8abf5337..b5bd1367beb61 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/datafeed/DatafeedConfig.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/datafeed/DatafeedConfig.java @@ -300,11 +300,11 @@ public Builder(DatafeedConfig config) { this.jobId = config.jobId; this.queryDelay = config.queryDelay; this.frequency = config.frequency; - this.indices = config.indices; - this.types = config.types; + this.indices = config.indices == null ? null : new ArrayList<>(config.indices); + this.types = config.types == null ? null : new ArrayList<>(config.types); this.query = config.query; this.aggregations = config.aggregations; - this.scriptFields = config.scriptFields; + this.scriptFields = config.scriptFields == null ? null : new ArrayList<>(config.scriptFields); this.scrollSize = config.scrollSize; this.chunkingConfig = config.chunkingConfig; this.delayedDataCheckConfig = config.getDelayedDataCheckConfig(); diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/job/config/Job.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/job/config/Job.java index 13b4dcb955a05..7a8b2c12f313a 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/job/config/Job.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/job/config/Job.java @@ -28,8 +28,10 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; import java.util.Date; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -426,7 +428,7 @@ public Builder(String id) { public Builder(Job job) { this.id = job.getId(); this.jobType = job.getJobType(); - this.groups = job.getGroups(); + this.groups = new ArrayList<>(job.getGroups()); this.description = job.getDescription(); this.analysisConfig = job.getAnalysisConfig(); this.analysisLimits = job.getAnalysisLimits(); @@ -439,7 +441,7 @@ public Builder(Job job) { this.backgroundPersistInterval = job.getBackgroundPersistInterval(); this.modelSnapshotRetentionDays = job.getModelSnapshotRetentionDays(); this.resultsRetentionDays = job.getResultsRetentionDays(); - this.customSettings = job.getCustomSettings(); + this.customSettings = job.getCustomSettings() == null ? null : new HashMap<>(job.getCustomSettings()); this.modelSnapshotId = job.getModelSnapshotId(); this.resultsIndexName = job.getResultsIndexNameNoPrefix(); this.deleting = job.getDeleting(); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java index 4ebb12555e27a..3297aeb0e5578 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java @@ -42,6 +42,7 @@ import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -232,8 +233,8 @@ private DatafeedConfig(String id, String jobId, TimeValue queryDelay, TimeValue this.frequency = frequency; this.indices = indices == null ? null : Collections.unmodifiableList(indices); this.types = types == null ? null : Collections.unmodifiableList(types); - this.query = query; - this.aggregations = aggregations; + this.query = query == null ? null : Collections.unmodifiableMap(query); + this.aggregations = aggregations == null ? null : Collections.unmodifiableMap(aggregations); this.scriptFields = scriptFields == null ? null : Collections.unmodifiableList(scriptFields); this.scrollSize = scrollSize; this.chunkingConfig = chunkingConfig; @@ -587,8 +588,6 @@ public static class Builder { private Map headers = Collections.emptyMap(); private DelayedDataCheckConfig delayedDataCheckConfig = DelayedDataCheckConfig.defaultDelayedDataCheckConfig(); - - public Builder() { try { this.query = QUERY_TRANSFORMER.toMap(QueryBuilders.matchAllQuery()); @@ -606,14 +605,14 @@ public Builder(DatafeedConfig config) { this.jobId = config.jobId; this.queryDelay = config.queryDelay; this.frequency = config.frequency; - this.indices = config.indices; - this.types = config.types; - this.query = config.query; - this.aggregations = config.aggregations; - this.scriptFields = config.scriptFields; + this.indices = new ArrayList<>(config.indices); + this.types = new ArrayList<>(config.types); + this.query = config.query == null ? null : new HashMap<>(config.query); + this.aggregations = config.aggregations == null ? null : new HashMap<>(config.aggregations); + this.scriptFields = config.scriptFields == null ? null : new ArrayList<>(config.scriptFields); this.scrollSize = config.scrollSize; this.chunkingConfig = config.chunkingConfig; - this.headers = config.headers; + this.headers = new HashMap<>(config.headers); this.delayedDataCheckConfig = config.getDelayedDataCheckConfig(); } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Job.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Job.java index 673e6f45105d7..5350a03ccc042 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Job.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Job.java @@ -32,6 +32,7 @@ import java.util.Collection; import java.util.Collections; import java.util.Date; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -660,7 +661,7 @@ public Builder(Job job) { this.id = job.getId(); this.jobType = job.getJobType(); this.jobVersion = job.getJobVersion(); - this.groups = job.getGroups(); + this.groups = new ArrayList<>(job.getGroups()); this.description = job.getDescription(); this.analysisConfig = job.getAnalysisConfig(); this.analysisLimits = job.getAnalysisLimits(); @@ -673,7 +674,7 @@ public Builder(Job job) { this.backgroundPersistInterval = job.getBackgroundPersistInterval(); this.modelSnapshotRetentionDays = job.getModelSnapshotRetentionDays(); this.resultsRetentionDays = job.getResultsRetentionDays(); - this.customSettings = job.getCustomSettings(); + this.customSettings = job.getCustomSettings() == null ? null : new HashMap<>(job.getCustomSettings()); this.modelSnapshotId = job.getModelSnapshotId(); this.modelSnapshotMinVersion = job.getModelSnapshotMinVersion(); this.resultsIndexName = job.getResultsIndexNameNoPrefix(); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java index d39fd35dd9c47..e5c1dfb6cabb1 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.core.ml.datafeed; import com.carrotsearch.randomizedtesting.generators.CodepointSetGenerator; - import org.elasticsearch.ElasticsearchException; import org.elasticsearch.Version; import org.elasticsearch.common.bytes.BytesReference; @@ -700,6 +699,13 @@ public void testSerializationOfComplexAggsBetweenVersions() throws IOException { } } + public void testCopyingDatafeedDoesNotCauseStackOverflow() { + DatafeedConfig datafeed = createTestInstance(); + for (int i = 0; i < 100000; i++) { + datafeed = new DatafeedConfig.Builder(datafeed).build(); + } + } + public static String randomValidDatafeedId() { CodepointSetGenerator generator = new CodepointSetGenerator("abcdefghijklmnopqrstuvwxyz".toCharArray()); return generator.ofCodePointsLength(random(), 10, 10); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobTests.java index 4fa6617f045f6..934d710719590 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobTests.java @@ -563,6 +563,13 @@ public void testEarliestValidTimestamp_GivenDataCountsAndLatency() { assertThat(builder.build().earliestValidTimestamp(dataCounts), equalTo(123455789L)); } + public void testCopyingJobDoesNotCauseStackOverflow() { + Job job = createRandomizedJob(); + for (int i = 0; i < 100000; i++) { + job = new Job.Builder(job).build(); + } + } + public static Job.Builder buildJobBuilder(String id, Date date) { Job.Builder builder = new Job.Builder(id); builder.setCreateTime(date); From 2e8f03709789f9da0695202b5446ced2542eac37 Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Fri, 7 Dec 2018 18:04:35 +0000 Subject: [PATCH 2/2] Use LinkedHashMap for query and aggs --- .../elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java index 3297aeb0e5578..afb25363ce0e9 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java @@ -43,6 +43,7 @@ import java.util.Collections; import java.util.Comparator; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -607,8 +608,8 @@ public Builder(DatafeedConfig config) { this.frequency = config.frequency; this.indices = new ArrayList<>(config.indices); this.types = new ArrayList<>(config.types); - this.query = config.query == null ? null : new HashMap<>(config.query); - this.aggregations = config.aggregations == null ? null : new HashMap<>(config.aggregations); + this.query = config.query == null ? null : new LinkedHashMap<>(config.query); + this.aggregations = config.aggregations == null ? null : new LinkedHashMap<>(config.aggregations); this.scriptFields = config.scriptFields == null ? null : new ArrayList<>(config.scriptFields); this.scrollSize = config.scrollSize; this.chunkingConfig = config.chunkingConfig;