Skip to content

Commit 7870ae8

Browse files
[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
1 parent ead2b9e commit 7870ae8

File tree

6 files changed

+34
-18
lines changed

6 files changed

+34
-18
lines changed

client/rest-high-level/src/main/java/org/elasticsearch/client/ml/datafeed/DatafeedConfig.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -300,11 +300,11 @@ public Builder(DatafeedConfig config) {
300300
this.jobId = config.jobId;
301301
this.queryDelay = config.queryDelay;
302302
this.frequency = config.frequency;
303-
this.indices = config.indices;
304-
this.types = config.types;
303+
this.indices = config.indices == null ? null : new ArrayList<>(config.indices);
304+
this.types = config.types == null ? null : new ArrayList<>(config.types);
305305
this.query = config.query;
306306
this.aggregations = config.aggregations;
307-
this.scriptFields = config.scriptFields;
307+
this.scriptFields = config.scriptFields == null ? null : new ArrayList<>(config.scriptFields);
308308
this.scrollSize = config.scrollSize;
309309
this.chunkingConfig = config.chunkingConfig;
310310
this.delayedDataCheckConfig = config.getDelayedDataCheckConfig();

client/rest-high-level/src/main/java/org/elasticsearch/client/ml/job/config/Job.java

+4-2
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,10 @@
2828
import org.elasticsearch.common.xcontent.XContentBuilder;
2929

3030
import java.io.IOException;
31+
import java.util.ArrayList;
3132
import java.util.Collections;
3233
import java.util.Date;
34+
import java.util.HashMap;
3335
import java.util.List;
3436
import java.util.Map;
3537
import java.util.Objects;
@@ -426,7 +428,7 @@ public Builder(String id) {
426428
public Builder(Job job) {
427429
this.id = job.getId();
428430
this.jobType = job.getJobType();
429-
this.groups = job.getGroups();
431+
this.groups = new ArrayList<>(job.getGroups());
430432
this.description = job.getDescription();
431433
this.analysisConfig = job.getAnalysisConfig();
432434
this.analysisLimits = job.getAnalysisLimits();
@@ -439,7 +441,7 @@ public Builder(Job job) {
439441
this.backgroundPersistInterval = job.getBackgroundPersistInterval();
440442
this.modelSnapshotRetentionDays = job.getModelSnapshotRetentionDays();
441443
this.resultsRetentionDays = job.getResultsRetentionDays();
442-
this.customSettings = job.getCustomSettings();
444+
this.customSettings = job.getCustomSettings() == null ? null : new HashMap<>(job.getCustomSettings());
443445
this.modelSnapshotId = job.getModelSnapshotId();
444446
this.resultsIndexName = job.getResultsIndexNameNoPrefix();
445447
this.deleting = job.getDeleting();

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java

+10-10
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@
4242
import java.util.Collection;
4343
import java.util.Collections;
4444
import java.util.Comparator;
45+
import java.util.HashMap;
46+
import java.util.LinkedHashMap;
4547
import java.util.List;
4648
import java.util.Map;
4749
import java.util.Objects;
@@ -232,8 +234,8 @@ private DatafeedConfig(String id, String jobId, TimeValue queryDelay, TimeValue
232234
this.frequency = frequency;
233235
this.indices = indices == null ? null : Collections.unmodifiableList(indices);
234236
this.types = types == null ? null : Collections.unmodifiableList(types);
235-
this.query = query;
236-
this.aggregations = aggregations;
237+
this.query = query == null ? null : Collections.unmodifiableMap(query);
238+
this.aggregations = aggregations == null ? null : Collections.unmodifiableMap(aggregations);
237239
this.scriptFields = scriptFields == null ? null : Collections.unmodifiableList(scriptFields);
238240
this.scrollSize = scrollSize;
239241
this.chunkingConfig = chunkingConfig;
@@ -587,8 +589,6 @@ public static class Builder {
587589
private Map<String, String> headers = Collections.emptyMap();
588590
private DelayedDataCheckConfig delayedDataCheckConfig = DelayedDataCheckConfig.defaultDelayedDataCheckConfig();
589591

590-
591-
592592
public Builder() {
593593
try {
594594
this.query = QUERY_TRANSFORMER.toMap(QueryBuilders.matchAllQuery());
@@ -606,14 +606,14 @@ public Builder(DatafeedConfig config) {
606606
this.jobId = config.jobId;
607607
this.queryDelay = config.queryDelay;
608608
this.frequency = config.frequency;
609-
this.indices = config.indices;
610-
this.types = config.types;
611-
this.query = config.query;
612-
this.aggregations = config.aggregations;
613-
this.scriptFields = config.scriptFields;
609+
this.indices = new ArrayList<>(config.indices);
610+
this.types = new ArrayList<>(config.types);
611+
this.query = config.query == null ? null : new LinkedHashMap<>(config.query);
612+
this.aggregations = config.aggregations == null ? null : new LinkedHashMap<>(config.aggregations);
613+
this.scriptFields = config.scriptFields == null ? null : new ArrayList<>(config.scriptFields);
614614
this.scrollSize = config.scrollSize;
615615
this.chunkingConfig = config.chunkingConfig;
616-
this.headers = config.headers;
616+
this.headers = new HashMap<>(config.headers);
617617
this.delayedDataCheckConfig = config.getDelayedDataCheckConfig();
618618
}
619619

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Job.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import java.util.Collection;
3333
import java.util.Collections;
3434
import java.util.Date;
35+
import java.util.HashMap;
3536
import java.util.HashSet;
3637
import java.util.List;
3738
import java.util.Map;
@@ -660,7 +661,7 @@ public Builder(Job job) {
660661
this.id = job.getId();
661662
this.jobType = job.getJobType();
662663
this.jobVersion = job.getJobVersion();
663-
this.groups = job.getGroups();
664+
this.groups = new ArrayList<>(job.getGroups());
664665
this.description = job.getDescription();
665666
this.analysisConfig = job.getAnalysisConfig();
666667
this.analysisLimits = job.getAnalysisLimits();
@@ -673,7 +674,7 @@ public Builder(Job job) {
673674
this.backgroundPersistInterval = job.getBackgroundPersistInterval();
674675
this.modelSnapshotRetentionDays = job.getModelSnapshotRetentionDays();
675676
this.resultsRetentionDays = job.getResultsRetentionDays();
676-
this.customSettings = job.getCustomSettings();
677+
this.customSettings = job.getCustomSettings() == null ? null : new HashMap<>(job.getCustomSettings());
677678
this.modelSnapshotId = job.getModelSnapshotId();
678679
this.modelSnapshotMinVersion = job.getModelSnapshotMinVersion();
679680
this.resultsIndexName = job.getResultsIndexNameNoPrefix();

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java

+7-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
package org.elasticsearch.xpack.core.ml.datafeed;
77

88
import com.carrotsearch.randomizedtesting.generators.CodepointSetGenerator;
9-
109
import org.elasticsearch.ElasticsearchException;
1110
import org.elasticsearch.Version;
1211
import org.elasticsearch.common.bytes.BytesReference;
@@ -700,6 +699,13 @@ public void testSerializationOfComplexAggsBetweenVersions() throws IOException {
700699
}
701700
}
702701

702+
public void testCopyingDatafeedDoesNotCauseStackOverflow() {
703+
DatafeedConfig datafeed = createTestInstance();
704+
for (int i = 0; i < 100000; i++) {
705+
datafeed = new DatafeedConfig.Builder(datafeed).build();
706+
}
707+
}
708+
703709
public static String randomValidDatafeedId() {
704710
CodepointSetGenerator generator = new CodepointSetGenerator("abcdefghijklmnopqrstuvwxyz".toCharArray());
705711
return generator.ofCodePointsLength(random(), 10, 10);

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobTests.java

+7
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,13 @@ public void testEarliestValidTimestamp_GivenDataCountsAndLatency() {
563563
assertThat(builder.build().earliestValidTimestamp(dataCounts), equalTo(123455789L));
564564
}
565565

566+
public void testCopyingJobDoesNotCauseStackOverflow() {
567+
Job job = createRandomizedJob();
568+
for (int i = 0; i < 100000; i++) {
569+
job = new Job.Builder(job).build();
570+
}
571+
}
572+
566573
public static Job.Builder buildJobBuilder(String id, Date date) {
567574
Job.Builder builder = new Job.Builder(id);
568575
builder.setCreateTime(date);

0 commit comments

Comments
 (0)