Disallow negative TimeValues (elastic#53913)

gwbrown · gwbrown · commit 3543e96a2cbb · 2020-03-26T10:10:08.000-06:00
This commit causes negative TimeValues, other than -1 which is sometimes used as
a sentinel value, to be rejected during parsing.

Also introduces a hack to allow ILM to load policies which were written to the
cluster state with a negative min_age, treating those values as 0, which should
match the behavior of prior versions.
diff --git a/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java b/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java
@@ -74,6 +74,9 @@ public TimeValue(long millis) {
     }
 
     public TimeValue(long duration, TimeUnit timeUnit) {
+        if (duration < -1) {
+            throw new IllegalArgumentException("duration cannot be negative, was given [" + duration + "]");
+        }
         this.duration = duration;
         this.timeUnit = timeUnit;
     }
@@ -186,7 +189,7 @@ public double getDaysFrac() {
      * Returns a {@link String} representation of the current {@link TimeValue}.
      *
      * Note that this method might produce fractional time values (ex 1.6m) which cannot be
-     * parsed by method like {@link TimeValue#parse(String, String, String)}.
+     * parsed by method like {@link TimeValue#parse(String, String, String, String)}.
      */
     @Override
     public String toString() {
@@ -278,20 +281,20 @@ public static TimeValue parseTimeValue(String sValue, TimeValue defaultValue, St
         }
         final String normalized = sValue.toLowerCase(Locale.ROOT).trim();
         if (normalized.endsWith("nanos")) {
-            return new TimeValue(parse(sValue, normalized, "nanos"), TimeUnit.NANOSECONDS);
+            return new TimeValue(parse(sValue, normalized, "nanos", settingName), TimeUnit.NANOSECONDS);
         } else if (normalized.endsWith("micros")) {
-            return new TimeValue(parse(sValue, normalized, "micros"), TimeUnit.MICROSECONDS);
+            return new TimeValue(parse(sValue, normalized, "micros", settingName), TimeUnit.MICROSECONDS);
         } else if (normalized.endsWith("ms")) {
-            return new TimeValue(parse(sValue, normalized, "ms"), TimeUnit.MILLISECONDS);
+            return new TimeValue(parse(sValue, normalized, "ms", settingName), TimeUnit.MILLISECONDS);
         } else if (normalized.endsWith("s")) {
-            return new TimeValue(parse(sValue, normalized, "s"), TimeUnit.SECONDS);
+            return new TimeValue(parse(sValue, normalized, "s", settingName), TimeUnit.SECONDS);
         } else if (sValue.endsWith("m")) {
             // parsing minutes should be case-sensitive as 'M' means "months", not "minutes"; this is the only special case.
-            return new TimeValue(parse(sValue, normalized, "m"), TimeUnit.MINUTES);
+            return new TimeValue(parse(sValue, normalized, "m", settingName), TimeUnit.MINUTES);
         } else if (normalized.endsWith("h")) {
-            return new TimeValue(parse(sValue, normalized, "h"), TimeUnit.HOURS);
+            return new TimeValue(parse(sValue, normalized, "h", settingName), TimeUnit.HOURS);
         } else if (normalized.endsWith("d")) {
-            return new TimeValue(parse(sValue, normalized, "d"), TimeUnit.DAYS);
+            return new TimeValue(parse(sValue, normalized, "d", settingName), TimeUnit.DAYS);
         } else if (normalized.matches("-0*1")) {
             return TimeValue.MINUS_ONE;
         } else if (normalized.matches("0+")) {
@@ -303,10 +306,16 @@ public static TimeValue parseTimeValue(String sValue, TimeValue defaultValue, St
         }
     }
 
-    private static long parse(final String initialInput, final String normalized, final String suffix) {
+    private static long parse(final String initialInput, final String normalized, final String suffix, String settingName) {
         final String s = normalized.substring(0, normalized.length() - suffix.length()).trim();
         try {
-            return Long.parseLong(s);
+            final long value = Long.parseLong(s);
+            if (value < -1) {
+                // -1 is magic, but reject any other negative values
+                throw new IllegalArgumentException("failed to parse setting [" + settingName + "] with value [" + initialInput +
+                    "] as a time value: negative durations are not supported");
+            }
+            return value;
         } catch (final NumberFormatException e) {
             try {
                 @SuppressWarnings("unused") final double ignored = Double.parseDouble(s);
diff --git a/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java b/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java
@@ -218,4 +218,26 @@ public void testConversionHashCode() {
         TimeValue secondValue = new TimeValue(firstValue.getSeconds(), TimeUnit.SECONDS);
         assertEquals(firstValue.hashCode(), secondValue.hashCode());
     }
+
+    public void testRejectsNegativeValuesDuringParsing() {
+        final String settingName = "test-value";
+        final long negativeValue = randomLongBetween(Long.MIN_VALUE, -2);
+        final String negativeTimeValueString = Long.toString(negativeValue) + randomTimeUnit();
+        IllegalArgumentException ex = expectThrows(IllegalArgumentException.class,
+            () -> TimeValue.parseTimeValue(negativeTimeValueString, settingName));
+        assertThat(ex.getMessage(),
+            equalTo("failed to parse setting [" + settingName + "] with value [" + negativeTimeValueString +
+                "] as a time value: negative durations are not supported"));
+    }
+
+    public void testRejectsNegativeValuesAtCreation() {
+        final long duration = randomLongBetween(Long.MIN_VALUE, -2);
+        IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> new TimeValue(duration, randomTimeUnitObject()));
+        assertThat(ex.getMessage(), containsString("duration cannot be negative"));
+    }
+
+    private TimeUnit randomTimeUnitObject() {
+        return randomFrom(TimeUnit.NANOSECONDS, TimeUnit.MICROSECONDS, TimeUnit.MILLISECONDS, TimeUnit.SECONDS,
+            TimeUnit.MINUTES, TimeUnit.HOURS, TimeUnit.DAYS);
+    }
 }
diff --git a/server/src/main/java/org/elasticsearch/index/shard/IndexingStats.java b/server/src/main/java/org/elasticsearch/index/shard/IndexingStats.java
@@ -120,7 +120,9 @@ public long getDeleteCount() {
         /**
          * The total amount of time spend on executing delete operations.
          */
-        public TimeValue getDeleteTime() { return new TimeValue(deleteTimeInMillis); }
+        public TimeValue getDeleteTime() {
+            return new TimeValue(deleteTimeInMillis);
+        }
 
         /**
          * Returns the currently in-flight delete operations
diff --git a/server/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java b/server/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java
@@ -28,8 +28,8 @@
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.monitor.jvm.JvmStats.GarbageCollector;
-import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.threadpool.Scheduler.Cancellable;
+import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.threadpool.ThreadPool.Names;
 
 import java.util.HashMap;
@@ -165,13 +165,20 @@ public JvmGcMonitorService(Settings settings, ThreadPool threadPool) {
     }
 
     private static TimeValue getValidThreshold(Settings settings, String key, String level) {
-        TimeValue threshold = settings.getAsTime(level, null);
+        final TimeValue threshold;
+
+        try {
+            threshold = settings.getAsTime(level, null);
+        } catch (RuntimeException ex) {
+            final String settingValue = settings.get(level);
+            throw new IllegalArgumentException("failed to parse setting [" + getThresholdName(key, level) + "] with value [" +
+                settingValue + "] as a time value", ex);
+        }
+
         if (threshold == null) {
             throw new IllegalArgumentException("missing gc_threshold for [" + getThresholdName(key, level) + "]");
         }
-        if (threshold.nanos() <= 0) {
-            throw new IllegalArgumentException("invalid gc_threshold [" + threshold + "] for [" + getThresholdName(key, level) + "]");
-        }
+
         return threshold;
     }
 
diff --git a/server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java b/server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java
@@ -520,7 +520,7 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa
         if (verbose || endTime != 0) {
             builder.field(END_TIME, DATE_TIME_FORMATTER.format(Instant.ofEpochMilli(endTime).atZone(ZoneOffset.UTC)));
             builder.field(END_TIME_IN_MILLIS, endTime);
-            builder.humanReadableField(DURATION_IN_MILLIS, DURATION, new TimeValue(endTime - startTime));
+            builder.humanReadableField(DURATION_IN_MILLIS, DURATION, new TimeValue(Math.max(0L, endTime - startTime)));
         }
         if (verbose || !shardFailures.isEmpty()) {
             builder.startArray(FAILURES);
diff --git a/server/src/test/java/org/elasticsearch/common/rounding/TimeZoneRoundingTests.java b/server/src/test/java/org/elasticsearch/common/rounding/TimeZoneRoundingTests.java
@@ -104,8 +104,8 @@ public void testDayRounding() {
         Rounding tzRounding = Rounding.builder(DateTimeUnit.DAY_OF_MONTH).timeZone(DateTimeZone.forOffsetHours(timezoneOffset))
                 .build();
         assertThat(tzRounding.round(0), equalTo(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis()));
-        assertThat(tzRounding.nextRoundingValue(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis()), equalTo(0L - TimeValue
-                .timeValueHours(timezoneOffset).millis()));
+        assertThat(tzRounding.nextRoundingValue(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis()), equalTo(TimeValue
+                .timeValueHours(-timezoneOffset).millis()));
 
         DateTimeZone tz = DateTimeZone.forID("-08:00");
         tzRounding = Rounding.builder(DateTimeUnit.DAY_OF_MONTH).timeZone(tz).build();
diff --git a/server/src/test/java/org/elasticsearch/indices/IndexingMemoryControllerTests.java b/server/src/test/java/org/elasticsearch/indices/IndexingMemoryControllerTests.java
@@ -260,15 +260,17 @@ public void testNegativeInterval() {
         Exception e = expectThrows(IllegalArgumentException.class,
                                    () -> new MockController(Settings.builder()
                                                             .put("indices.memory.interval", "-42s").build()));
-        assertEquals("failed to parse value [-42s] for setting [indices.memory.interval], must be >= [0ms]", e.getMessage());
+        assertEquals("failed to parse setting [indices.memory.interval] with value " +
+            "[-42s] as a time value: negative durations are not supported", e.getMessage());
 
     }
 
     public void testNegativeShardInactiveTime() {
         Exception e = expectThrows(IllegalArgumentException.class,
                                    () -> new MockController(Settings.builder()
                                                             .put("indices.memory.shard_inactive_time", "-42s").build()));
-        assertEquals("failed to parse value [-42s] for setting [indices.memory.shard_inactive_time], must be >= [0ms]", e.getMessage());
+        assertEquals("failed to parse setting [indices.memory.shard_inactive_time] with value " +
+            "[-42s] as a time value: negative durations are not supported", e.getMessage());
 
     }
 
diff --git a/server/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java b/server/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java
@@ -33,8 +33,8 @@
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Consumer;
 
-import static org.hamcrest.CoreMatchers.allOf;
 import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.instanceOf;
 
 public class JvmGcMonitorServiceSettingsTests extends ESTestCase {
@@ -62,11 +62,12 @@ public void testDisabledSetting() throws InterruptedException {
 
     public void testNegativeSetting() throws InterruptedException {
         String collector = randomAlphaOfLength(5);
-        Settings settings = Settings.builder().put("monitor.jvm.gc.collector." + collector + ".warn", "-" + randomTimeValue()).build();
+        final String timeValue = "-" + randomTimeValue();
+        Settings settings = Settings.builder().put("monitor.jvm.gc.collector." + collector + ".warn", timeValue).build();
         execute(settings, (command, interval, name) -> null, e -> {
             assertThat(e, instanceOf(IllegalArgumentException.class));
-            assertThat(e.getMessage(), allOf(containsString("invalid gc_threshold"),
-                containsString("for [monitor.jvm.gc.collector." + collector + ".")));
+            assertThat(e.getMessage(), equalTo("failed to parse setting [monitor.jvm.gc.collector." + collector + ".warn] " +
+                "with value [" + timeValue + "] as a time value"));
         }, true, null);
     }
 
diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTaskStatusTests.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTaskStatusTests.java
@@ -60,7 +60,7 @@ protected ShardFollowNodeTaskStatus createTestInstance() {
                 randomNonNegativeLong(),
                 randomNonNegativeLong(),
                 randomReadExceptions(),
-                randomLong(),
+                randomNonNegativeLong(),
                 randomBoolean() ? new ElasticsearchException("fatal error") : null);
     }
 
diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/StatsResponsesTests.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/StatsResponsesTests.java
@@ -57,7 +57,7 @@ static FollowStatsAction.StatsResponses createStatsResponse() {
                 randomNonNegativeLong(),
                 randomNonNegativeLong(),
                 Collections.emptyNavigableMap(),
-                randomLong(),
+                randomNonNegativeLong(),
                 randomBoolean() ? new ElasticsearchException("fatal error") : null);
             responses.add(new FollowStatsAction.StatsResponse(status));
         }
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
@@ -366,8 +366,8 @@ public void testCheckValid_GivenIndicesContainsOnlyEmptyStrings() {
     public void testCheckValid_GivenNegativeQueryDelay() {
         DatafeedConfig.Builder conf = new DatafeedConfig.Builder("datafeed1", "job1");
         IllegalArgumentException e = ESTestCase.expectThrows(IllegalArgumentException.class,
-                () -> conf.setQueryDelay(TimeValue.timeValueMillis(-10)));
-        assertEquals("query_delay cannot be less than 0. Value = -10", e.getMessage());
+                () -> conf.setQueryDelay(TimeValue.timeValueMillis(-1)));
+        assertEquals("query_delay cannot be less than 0. Value = -1", e.getMessage());
     }
 
     public void testCheckValid_GivenZeroFrequency() {
diff --git a/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/action/EnrichStatsResponseTests.java b/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/action/EnrichStatsResponseTests.java
diff --git a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/collector/indices/IndicesStatsMonitoringDocTests.java b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/collector/indices/IndicesStatsMonitoringDocTests.java
@@ -143,14 +143,14 @@ public void testToXContent() throws IOException {
 
     private CommonStats mockCommonStats() {
         final CommonStats commonStats = new CommonStats(CommonStatsFlags.ALL);
-        commonStats.getDocs().add(new DocsStats(1L, -1L, randomNonNegativeLong()));
+        commonStats.getDocs().add(new DocsStats(1L, 0L, randomNonNegativeLong()));
         commonStats.getStore().add(new StoreStats(2L));
 
-        final IndexingStats.Stats indexingStats = new IndexingStats.Stats(3L, 4L, -1L, -1L, -1L, -1L, -1L, -1L, true, 5L);
+        final IndexingStats.Stats indexingStats = new IndexingStats.Stats(3L, 4L, 0L, 0L, 0L, 0L, 0L, 0L, true, 5L);
         commonStats.getIndexing().add(new IndexingStats(indexingStats, null));
 
-        final SearchStats.Stats searchStats = new SearchStats.Stats(6L, 7L, -1L, -1L, -1L, -1L, -1L, -1L, -1L, -1L, -1L, -1L);
-        commonStats.getSearch().add(new SearchStats(searchStats, -1L, null));
+        final SearchStats.Stats searchStats = new SearchStats.Stats(6L, 7L, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0L);
+        commonStats.getSearch().add(new SearchStats(searchStats, 0L, null));
 
         return commonStats;
     }
diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherDateTimeUtilsTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherDateTimeUtilsTests.java
@@ -89,28 +89,6 @@ public void testParseTimeValueString() throws Exception {
         assertThat(parsed.millis(), is(values.get(key).millis()));
     }
 
-    public void testParseTimeValueStringNegative() throws Exception {
-        int value = -1 * randomIntBetween(2, 200);
-        Map<String, TimeValue> values = new HashMap<>();
-        values.put(value + "s", TimeValue.timeValueSeconds(value));
-        values.put(value + "m", TimeValue.timeValueMinutes(value));
-        values.put(value + "h", TimeValue.timeValueHours(value));
-
-        String key = randomFrom(values.keySet().toArray(new String[values.size()]));
-
-        XContentParser parser = createParser(jsonBuilder().startObject().field("value", key).endObject());
-        parser.nextToken(); // start object
-        parser.nextToken(); // field name
-        parser.nextToken(); // value
-
-        try {
-            WatcherDateTimeUtils.parseTimeValue(parser, "test");
-            fail("Expected ElasticsearchParseException");
-        } catch (ElasticsearchParseException e) {
-            assertThat(e.getMessage(), is("failed to parse time unit"));
-        }
-    }
-
     public void testParseTimeValueNull() throws Exception {
         XContentParser parser = createParser(jsonBuilder().startObject().nullField("value").endObject());
         parser.nextToken(); // start object

Original file line number	Diff line number	Diff line change
`@@ -520,7 +520,7 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa`
`520`	`520`	`if (verbose \|\| endTime != 0) {`
`521`	`521`	`builder.field(END_TIME, DATE_TIME_FORMATTER.format(Instant.ofEpochMilli(endTime).atZone(ZoneOffset.UTC)));`
`522`	`522`	`builder.field(END_TIME_IN_MILLIS, endTime);`
`523`		`- builder.humanReadableField(DURATION_IN_MILLIS, DURATION, new TimeValue(endTime - startTime));`
	`523`	`+ builder.humanReadableField(DURATION_IN_MILLIS, DURATION, new TimeValue(Math.max(0L, endTime - startTime)));`
`524`	`524`	`}`
`525`	`525`	`if (verbose \|\| !shardFailures.isEmpty()) {`
`526`	`526`	`builder.startArray(FAILURES);`
Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,7 @@ protected ShardFollowNodeTaskStatus createTestInstance() {`
`60`	`60`	`randomNonNegativeLong(),`
`61`	`61`	`randomNonNegativeLong(),`
`62`	`62`	`randomReadExceptions(),`
`63`		`- randomLong(),`
	`63`	`+ randomNonNegativeLong(),`
`64`	`64`	`randomBoolean() ? new ElasticsearchException("fatal error") : null);`
`65`	`65`	`}`
`66`	`66`
Original file line number	Diff line number	Diff line change
`@@ -57,7 +57,7 @@ static FollowStatsAction.StatsResponses createStatsResponse() {`
`57`	`57`	`randomNonNegativeLong(),`
`58`	`58`	`randomNonNegativeLong(),`
`59`	`59`	`Collections.emptyNavigableMap(),`
`60`		`- randomLong(),`
	`60`	`+ randomNonNegativeLong(),`
`61`	`61`	`randomBoolean() ? new ElasticsearchException("fatal error") : null);`
`62`	`62`	`responses.add(new FollowStatsAction.StatsResponse(status));`
`63`	`63`	`}`