From 2bee9dc83ee8a4b6260aa2cf765a97554449e760 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 12 Feb 2015 18:18:41 +0100 Subject: [PATCH] Aggregations: Prevent negative intervals in date_histogram Negative settings for interval in date_histogram could lead to OOM errors in conjunction with min_doc_count=0. This fix raises exceptions in the histogram builder and the TimeZoneRounding classes so that the query fails before this can happen. Closes #9690 --- .../common/rounding/TimeZoneRounding.java | 9 +++++++++ .../bucket/histogram/HistogramParser.java | 2 +- .../bucket/DateHistogramTests.java | 19 +++++++++++++++++-- .../aggregations/bucket/HistogramTests.java | 14 ++++++++++++++ 4 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java b/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java index 4a07d27fc399d..b2aec9f6776c5 100644 --- a/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java +++ b/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java @@ -19,6 +19,7 @@ package org.elasticsearch.common.rounding; +import org.elasticsearch.ElasticsearchIllegalArgumentException; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.unit.TimeValue; @@ -63,6 +64,8 @@ public Builder(DateTimeUnit unit) { public Builder(TimeValue interval) { this.unit = null; + if (interval.millis() < 1) + throw new ElasticsearchIllegalArgumentException("Zero or negative time interval not supported"); this.interval = interval.millis(); } @@ -309,6 +312,8 @@ static class UTCIntervalTimeZoneRounding extends TimeZoneRounding { } UTCIntervalTimeZoneRounding(long interval) { + if (interval < 1) + throw new ElasticsearchIllegalArgumentException("Zero or negative time interval not supported"); this.interval = interval; } @@ -356,6 +361,8 @@ static class TimeIntervalTimeZoneRounding extends TimeZoneRounding { } TimeIntervalTimeZoneRounding(long interval, DateTimeZone preTz, DateTimeZone postTz) { + if (interval < 1) + throw new ElasticsearchIllegalArgumentException("Zero or negative time interval not supported"); this.interval = interval; this.preTz = preTz; this.postTz = postTz; @@ -414,6 +421,8 @@ static class DayIntervalTimeZoneRounding extends TimeZoneRounding { } DayIntervalTimeZoneRounding(long interval, DateTimeZone preTz, DateTimeZone postTz) { + if (interval < 1) + throw new ElasticsearchIllegalArgumentException("Zero or negative time interval not supported"); this.interval = interval; this.preTz = preTz; this.postTz = postTz; diff --git a/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramParser.java b/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramParser.java index 8dc2abbd900b7..8bf544ce99c48 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramParser.java +++ b/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramParser.java @@ -118,7 +118,7 @@ public AggregatorFactory parse(String aggregationName, XContentParser parser, Se } } - if (interval < 0) { + if (interval < 1) { throw new SearchParseException(context, "Missing required field [interval] for histogram aggregation [" + aggregationName + "]"); } diff --git a/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramTests.java b/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramTests.java index 6aebad9011528..58595403aaf69 100644 --- a/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramTests.java +++ b/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.bucket; import org.elasticsearch.action.index.IndexRequestBuilder; +import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.common.joda.Joda; import org.elasticsearch.common.settings.ImmutableSettings; @@ -43,13 +44,13 @@ import java.util.Arrays; import java.util.Collection; import java.util.List; +import java.util.concurrent.TimeUnit; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; import static org.elasticsearch.search.aggregations.AggregationBuilders.*; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.*; import static org.hamcrest.core.IsNull.notNullValue; /** @@ -1261,4 +1262,18 @@ public void testIssue6965() { assertThat(bucket.getKeyAsNumber().longValue(), equalTo(key.getMillis())); assertThat(bucket.getDocCount(), equalTo(3l)); } + + /** + * see issue #9634, negative interval in date_histogram should raise exception + */ + public void testExeptionOnNegativerInterval() { + try { + client().prepareSearch("idx") + .addAggregation(dateHistogram("histo").field("date").interval(-TimeUnit.DAYS.toMillis(1)).minDocCount(0)).execute() + .actionGet(); + fail(); + } catch (SearchPhaseExecutionException e) { + assertThat(e.getMessage(), containsString("IllegalArgumentException")); + } + } } diff --git a/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java b/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java index 68e4ece17acad..4acddf6cd8c19 100644 --- a/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java +++ b/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java @@ -20,6 +20,7 @@ import com.carrotsearch.hppc.LongOpenHashSet; import org.elasticsearch.action.index.IndexRequestBuilder; +import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.search.aggregations.Aggregator.SubAggCollectionMode; import org.elasticsearch.search.aggregations.bucket.filter.Filter; @@ -941,4 +942,17 @@ public void singleValuedField_WithExtendedBounds() throws Exception { } } + /** + * see issue #9634, negative interval in histogram should raise exception + */ + public void testExeptionOnNegativerInterval() { + try { + client().prepareSearch("empty_bucket_idx") + .addAggregation(histogram("histo").field(SINGLE_VALUED_FIELD_NAME).interval(-1).minDocCount(0)).execute().actionGet(); + fail(); + } catch (SearchPhaseExecutionException e) { + assertThat(e.getMessage(), containsString("Missing required field [interval]")); + } + } + }