[6.8] Disallow negative TimeValues (#53913) (#54304)

* Disallow negative TimeValues (#53913)

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.

* Re-apply IndexLifecycleRunner changes lost in merge

* Make SnapshotStatsTests generate more realistic times

* Fix negative TimeValue in deprecation test

* Re-apply CcrStatsResponseTests fix lost in merge

client/rest-high-level/src/test/java/org/elasticsearch/client/ccr/CcrStatsResponseTests.java

@@ -390,7 +390,7 @@ static IndicesFollowStats randomIndicesFollowStats() {
                     randomNonNegativeLong(),
                     randomNonNegativeLong(),
                     randomNonNegativeLong(),
-                    randomLong(),
+                    randomNonNegativeLong(),
                     readExceptions,
                     randomBoolean() ? new ElasticsearchException("fatal error") : null));
             }

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);

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);
+    }
 }

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

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;
     }
 

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);

server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStatsTests.java

@@ -19,17 +19,17 @@
 
 package org.elasticsearch.action.admin.cluster.snapshots.status;
 
-import java.io.IOException;
-
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.test.AbstractXContentTestCase;
 
+import java.io.IOException;
+
 public class SnapshotStatsTests extends AbstractXContentTestCase<SnapshotStats> {
 
     @Override
     protected SnapshotStats createTestInstance() {
         long startTime = randomNonNegativeLong();
-        long time = randomNonNegativeLong();
+        long time = randomLongBetween(0, Long.MAX_VALUE - startTime);
         int incrementalFileCount = randomIntBetween(0, Integer.MAX_VALUE);
         int totalFileCount = randomIntBetween(0, Integer.MAX_VALUE);
         int processedFileCount = randomIntBetween(0, Integer.MAX_VALUE);

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();

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());
 
     }
 

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);
     }
 

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);
     }
 

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));
         }

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() {

x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecks.java

@@ -14,7 +14,6 @@
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.joda.JodaDeprecationPatterns;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.index.analysis.AnalysisRegistry;
 import org.elasticsearch.xpack.core.deprecation.DeprecationIssue;
@@ -218,8 +217,7 @@ static DeprecationIssue nodeLeftDelayedTimeCheck(IndexMetaData indexMetaData) {
         String setting = UnassignedInfo.INDEX_DELAYED_NODE_LEFT_TIMEOUT_SETTING.getKey();
         String value = indexMetaData.getSettings().get(setting);
         if (Strings.isNullOrEmpty(value) == false) {
-            TimeValue parsedValue = TimeValue.parseTimeValue(value, setting);
-            if (parsedValue.getNanos() < 0) {
+            if (value.startsWith("-")) {
                 return new DeprecationIssue(DeprecationIssue.Level.WARNING,
                     "Negative values for " + setting + " are deprecated and should be set to 0",
                     "https://www.elastic.co/guide/en/elasticsearch/reference/7.0/breaking-changes-7.0.html" +

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java

@@ -81,14 +81,22 @@ boolean isReadyToTransitionToThisPhase(final String policy, final IndexMetaData
         assert lifecycleDate != null && lifecycleDate >= 0 : "expected index to have a lifecycle date but it did not";
         final TimeValue after = stepRegistry.getIndexAgeForPhase(policy, phase);
         final long now = nowSupplier.getAsLong();
-        final TimeValue age = new TimeValue(now - lifecycleDate);
+        final long ageMillis = now - lifecycleDate;
+        final TimeValue age;
+        if (ageMillis >= 0) {
+            age = new TimeValue(ageMillis);
+        } else if (ageMillis == Long.MIN_VALUE) {
+            age = new TimeValue(Long.MAX_VALUE);
+        } else {
+            age = new TimeValue(-ageMillis);
+        }
         if (logger.isTraceEnabled()) {
             logger.trace("[{}] checking for index age to be at least [{}] before performing actions in " +
-                    "the \"{}\" phase. Now: {}, lifecycle date: {}, age: [{}/{}s]",
+                    "the \"{}\" phase. Now: {}, lifecycle date: {}, age: [{}{}/{}s]",
                 indexMetaData.getIndex().getName(), after, phase,
                 new TimeValue(now).seconds(),
                 new TimeValue(lifecycleDate).seconds(),
-                age, age.seconds());
+                ageMillis < 0 ? "-" : "", age, age.seconds());
         }
         return now >= lifecycleDate + after.getMillis();
     }

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;
     }

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