From 7870ae81b151ca6bc189551362eae2a860e83253 Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Fri, 7 Dec 2018 20:16:25 +0000 Subject: [PATCH] [ML] Prevent stack overflow while copying ML jobs and datafeeds (#36370) 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 | 20 +++++++++---------- .../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, 34 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 4b9bc8abf53..b5bd1367beb 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 class DatafeedConfig implements ToXContentObject { 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 13b4dcb955a..7a8b2c12f31 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.ToXContentObject; 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 class Job implements ToXContentObject { 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 class Job implements ToXContentObject { 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 4ebb12555e2..afb25363ce0 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,8 @@ import java.util.ArrayList; import java.util.Collection; 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; @@ -232,8 +234,8 @@ public class DatafeedConfig extends AbstractDiffable implements 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 +589,6 @@ public class DatafeedConfig extends AbstractDiffable implements private Map headers = Collections.emptyMap(); private DelayedDataCheckConfig delayedDataCheckConfig = DelayedDataCheckConfig.defaultDelayedDataCheckConfig(); - - public Builder() { try { this.query = QUERY_TRANSFORMER.toMap(QueryBuilders.matchAllQuery()); @@ -606,14 +606,14 @@ public class DatafeedConfig extends AbstractDiffable implements 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 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; - 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 673e6f45105..5350a03ccc0 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.ArrayList; 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 class Job extends AbstractDiffable implements Writeable, ToXContentO 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 class Job extends AbstractDiffable implements Writeable, ToXContentO 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 d39fd35dd9c..e5c1dfb6cab 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 class DatafeedConfigTests extends AbstractSerializingTestCase { 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);