[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
elastic · Dec 7, 2018 · 7870ae8 · 7870ae8
1 parent ead2b9e
commit 7870ae8
Show file tree

Hide file tree

Showing 6 changed files with 34 additions and 18 deletions.
diff --git a/...nt/rest-high-level/src/main/java/org/elasticsearch/client/ml/datafeed/DatafeedConfig.java b/...nt/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
@@ -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/...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java b/...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java
@@ -42,6 +42,8 @@
 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 @@ 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 +589,6 @@ public static class Builder {
         private Map<String, String> headers = Collections.emptyMap();
         private DelayedDataCheckConfig delayedDataCheckConfig = DelayedDataCheckConfig.defaultDelayedDataCheckConfig();
 
-
-
         public Builder() {
             try {
                 this.query = QUERY_TRANSFORMER.toMap(QueryBuilders.matchAllQuery());
@@ -606,14 +606,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 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
@@ -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/...ugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java b/...ugin/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
@@ -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);