From 2577080a862c836a00dbf577a9b94a3b7a593794 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 2 Nov 2021 12:08:00 -0400 Subject: [PATCH 1/3] Prevent stack overflow in rounding The code that rounds dates - `Rounding.round` could possbly stack overflow on sufficiently strange time zones, bringing the whole node down. These time zones shouldn't be a thing - the largest stack depth I was able to get with my tzdb was 500 and that's using 80,000 day date ranges in the time zone with the most DST transitions - `Asia/Tehran`. Even so, if you are able to cause thousands of transitions it'd be kinder for us to fail with a "your time zone is bad" message rather than a huge stackoverflow that knocks over the whole node. So this changes the date resolution code to be a loop instead of a recursive function. I looked into making it not have to repeat at all, but I couldn't put all the of the rounding things into my head to have much confidence in it. --- .../org/elasticsearch/common/Rounding.java | 94 +++++++++++-------- .../elasticsearch/common/RoundingTests.java | 6 ++ 2 files changed, 60 insertions(+), 40 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/Rounding.java b/server/src/main/java/org/elasticsearch/common/Rounding.java index 6165c9fed85fd..02d0f3ebc59f2 100644 --- a/server/src/main/java/org/elasticsearch/common/Rounding.java +++ b/server/src/main/java/org/elasticsearch/common/Rounding.java @@ -1132,50 +1132,64 @@ public long beforeOverlap(long localMillis, Overlap overlap) { */ private class JavaTimeRounding extends TimeIntervalPreparedRounding { @Override - public long round(long utcMillis) { - final Instant utcInstant = Instant.ofEpochMilli(utcMillis); - final LocalDateTime rawLocalDateTime = LocalDateTime.ofInstant(utcInstant, timeZone); - - // a millisecond value with the same local time, in UTC, as `utcMillis` has in `timeZone` - final long localMillis = utcMillis + timeZone.getRules().getOffset(utcInstant).getTotalSeconds() * 1000; - assert localMillis == rawLocalDateTime.toInstant(ZoneOffset.UTC).toEpochMilli(); - - final long roundedMillis = roundKey(localMillis, interval) * interval; - final LocalDateTime roundedLocalDateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(roundedMillis), ZoneOffset.UTC); - - // Now work out what roundedLocalDateTime actually means - final List currentOffsets = timeZone.getRules().getValidOffsets(roundedLocalDateTime); - if (currentOffsets.isEmpty() == false) { - // There is at least one instant with the desired local time. In general the desired result is - // the latest rounded time that's no later than the input time, but this could involve rounding across - // a timezone transition, which may yield the wrong result - final ZoneOffsetTransition previousTransition = timeZone.getRules().previousTransition(utcInstant.plusMillis(1)); - for (int offsetIndex = currentOffsets.size() - 1; 0 <= offsetIndex; offsetIndex--) { - final OffsetDateTime offsetTime = roundedLocalDateTime.atOffset(currentOffsets.get(offsetIndex)); - final Instant offsetInstant = offsetTime.toInstant(); - if (previousTransition != null && offsetInstant.isBefore(previousTransition.getInstant())) { - /* - * Rounding down across the transition can yield the - * wrong result. It's best to return to the transition - * time and round that down. - */ - return round(previousTransition.getInstant().toEpochMilli() - 1); + public long round(long originalUtcMillis) { + long utcMillis = originalUtcMillis; + int attempts = 0; + attempt: while (attempts < 5000) { + final Instant utcInstant = Instant.ofEpochMilli(utcMillis); + final LocalDateTime rawLocalDateTime = LocalDateTime.ofInstant(utcInstant, timeZone); + + // a millisecond value with the same local time, in UTC, as `utcMillis` has in `timeZone` + final long localMillis = utcMillis + timeZone.getRules().getOffset(utcInstant).getTotalSeconds() * 1000; + assert localMillis == rawLocalDateTime.toInstant(ZoneOffset.UTC).toEpochMilli(); + + final long roundedMillis = roundKey(localMillis, interval) * interval; + final LocalDateTime roundedLocalDateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(roundedMillis), ZoneOffset.UTC); + + // Now work out what roundedLocalDateTime actually means + final List currentOffsets = timeZone.getRules().getValidOffsets(roundedLocalDateTime); + if (currentOffsets.isEmpty() == false) { + // There is at least one instant with the desired local time. In general the desired result is + // the latest rounded time that's no later than the input time, but this could involve rounding across + // a timezone transition, which may yield the wrong result + final ZoneOffsetTransition previousTransition = timeZone.getRules().previousTransition(utcInstant.plusMillis(1)); + for (int offsetIndex = currentOffsets.size() - 1; 0 <= offsetIndex; offsetIndex--) { + final OffsetDateTime offsetTime = roundedLocalDateTime.atOffset(currentOffsets.get(offsetIndex)); + final Instant offsetInstant = offsetTime.toInstant(); + if (previousTransition != null && offsetInstant.isBefore(previousTransition.getInstant())) { + /* + * Rounding down across the transition can yield the + * wrong result. It's best to return to the transition + * time and round that down. + */ + attempts++; + utcMillis = previousTransition.getInstant().toEpochMilli() - 1; + continue attempt; + } + + if (utcInstant.isBefore(offsetTime.toInstant()) == false) { + return offsetInstant.toEpochMilli(); + } } - if (utcInstant.isBefore(offsetTime.toInstant()) == false) { - return offsetInstant.toEpochMilli(); - } + final OffsetDateTime offsetTime = roundedLocalDateTime.atOffset(currentOffsets.get(0)); + final Instant offsetInstant = offsetTime.toInstant(); + throw new IllegalArgumentException( + this + " failed to round " + utcMillis + " down: " + offsetInstant + " is the earliest possible" + ); + } else { + // The desired time isn't valid because within a gap, so just return the start of the gap + ZoneOffsetTransition zoneOffsetTransition = timeZone.getRules().getTransition(roundedLocalDateTime); + return zoneOffsetTransition.getInstant().toEpochMilli(); } - - final OffsetDateTime offsetTime = roundedLocalDateTime.atOffset(currentOffsets.get(0)); - final Instant offsetInstant = offsetTime.toInstant(); - assert false : this + " failed to round " + utcMillis + " down: " + offsetInstant + " is the earliest possible"; - return offsetInstant.toEpochMilli(); // TODO or throw something? - } else { - // The desired time isn't valid because within a gap, so just return the start of the gap - ZoneOffsetTransition zoneOffsetTransition = timeZone.getRules().getTransition(roundedLocalDateTime); - return zoneOffsetTransition.getInstant().toEpochMilli(); } + throw new IllegalArgumentException( + this + + " failed to round " + + utcMillis + + " down: transitioned backwards through too many daylight savings time" + + " transitions is the earliest possible" + ); } @Override diff --git a/server/src/test/java/org/elasticsearch/common/RoundingTests.java b/server/src/test/java/org/elasticsearch/common/RoundingTests.java index 1e56069a7662d..7962ef8d164dc 100644 --- a/server/src/test/java/org/elasticsearch/common/RoundingTests.java +++ b/server/src/test/java/org/elasticsearch/common/RoundingTests.java @@ -1010,6 +1010,12 @@ public void testIntervalTwoTransitions() { assertThat(rounding.round(time("1982-11-10T02:51:22.662Z")), isDate(time("1982-03-23T05:00:00Z"), tz)); } + public void testHugeTimeInterval() { + ZoneId tz = ZoneId.of("Asia/Tehran"); + Rounding rounding = Rounding.builder(TimeValue.timeValueDays(80000)).timeZone(tz).build(); + assertThat(rounding.round(time("2078-11-10T02:51:22.662Z")), isDate(time("1970-01-01T00:00:00+03:30"), tz)); + } + public void testFixedIntervalRoundingSize() { Rounding unitRounding = Rounding.builder(TimeValue.timeValueHours(10)).build(); Rounding.Prepared prepared = unitRounding.prepare(time("2010-01-01T00:00:00.000Z"), time("2020-01-01T00:00:00.000Z")); From 7f73d0f6e6193ff2be1b6463eddb4e62630eec9c Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Fri, 5 Nov 2021 15:42:50 -0400 Subject: [PATCH 2/3] Explain --- .../src/main/java/org/elasticsearch/common/Rounding.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/Rounding.java b/server/src/main/java/org/elasticsearch/common/Rounding.java index 02d0f3ebc59f2..66daa4887a960 100644 --- a/server/src/main/java/org/elasticsearch/common/Rounding.java +++ b/server/src/main/java/org/elasticsearch/common/Rounding.java @@ -1135,6 +1135,12 @@ private class JavaTimeRounding extends TimeIntervalPreparedRounding { public long round(long originalUtcMillis) { long utcMillis = originalUtcMillis; int attempts = 0; + /* + * We give up after 5000 attempts and throw an exception. The + * most attempts I could get running locally are 500 - for + * Asia/Tehran with an 80,000 day range. You just can't declare + * ranges much larger than that in ES right now. + */ attempt: while (attempts < 5000) { final Instant utcInstant = Instant.ofEpochMilli(utcMillis); final LocalDateTime rawLocalDateTime = LocalDateTime.ofInstant(utcInstant, timeZone); @@ -1187,8 +1193,7 @@ public long round(long originalUtcMillis) { this + " failed to round " + utcMillis - + " down: transitioned backwards through too many daylight savings time" - + " transitions is the earliest possible" + + " down: transitioned backwards through too many daylight savings time transitions" ); } From 6265362ba8ee253d2ae911388ac00c64ecb71f52 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Sun, 7 Nov 2021 07:38:03 -0500 Subject: [PATCH 3/3] Assert exception --- .../org/elasticsearch/common/Rounding.java | 72 ++++++++++++++++--- .../elasticsearch/common/RoundingTests.java | 17 +++++ 2 files changed, 80 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/Rounding.java b/server/src/main/java/org/elasticsearch/common/Rounding.java index 66daa4887a960..00fac20be61d3 100644 --- a/server/src/main/java/org/elasticsearch/common/Rounding.java +++ b/server/src/main/java/org/elasticsearch/common/Rounding.java @@ -651,6 +651,9 @@ public double roundingSize(DateTimeUnit timeUnit) { } } } + + @Override + public abstract String toString(); } private class FixedToMidnightRounding extends TimeUnitPreparedRounding { @@ -670,6 +673,11 @@ public long nextRoundingValue(long utcMillis) { // TODO this is used in date range's collect so we should optimize it too return new JavaTimeToMidnightRounding().nextRoundingValue(utcMillis); } + + @Override + public String toString() { + return TimeUnitRounding.this + "[fixed to midnight]"; + } } private class FixedNotToMidnightRounding extends TimeUnitPreparedRounding { @@ -690,6 +698,11 @@ public long round(long utcMillis) { public final long nextRoundingValue(long utcMillis) { return round(utcMillis + unitMillis); } + + @Override + public String toString() { + return TimeUnitRounding.this + "[fixed to " + unitMillis + "]"; + } } private class ToMidnightRounding extends TimeUnitPreparedRounding implements LocalTimeOffset.Strategy { @@ -738,6 +751,11 @@ protected Prepared maybeUseArray(long minUtcMillis, long maxUtcMillis, int max) } return super.maybeUseArray(minUtcMillis, maxUtcMillis, max); } + + @Override + public String toString() { + return TimeUnitRounding.this + "[across DST to midnight]"; + } } private class NotToMidnightRounding extends AbstractNotToMidnightRounding implements LocalTimeOffset.Strategy { @@ -779,6 +797,11 @@ public long beforeOverlap(long localMillis, Overlap overlap) { } return overlap.previous().localToUtc(localMillis, this); // This is mostly for Asia/Lord_Howe } + + @Override + public String toString() { + return TimeUnitRounding.this + "[across DST to " + unitMillis + "]"; + } } private class JavaTimeToMidnightRounding extends TimeUnitPreparedRounding { @@ -839,6 +862,11 @@ private LocalDateTime nextRelevantMidnight(LocalDateTime localMidnight) { throw new IllegalArgumentException("Unknown round-to-midnight unit: " + unit); } } + + @Override + public String toString() { + return TimeUnitRounding.this + "[java.time to midnight]"; + } } private class JavaTimeNotToMidnightRounding extends AbstractNotToMidnightRounding { @@ -894,6 +922,11 @@ private Instant truncateAsLocalTime(Instant instant, final ZoneRules rules) { return null; } } + + @Override + public String toString() { + return TimeUnitRounding.this + "[java.time to " + unitMillis + "]"; + } } private abstract class AbstractNotToMidnightRounding extends TimeUnitPreparedRounding { @@ -1040,6 +1073,9 @@ public double roundingSize(DateTimeUnit timeUnit) { ); } } + + @Override + public abstract String toString(); } /** @@ -1068,6 +1104,11 @@ public long nextRoundingValue(long utcMillis) { // TODO this is used in date range's collect so we should optimize it too return new JavaTimeRounding().nextRoundingValue(utcMillis); } + + @Override + public String toString() { + return TimeIntervalRounding.this + "[fixed]"; + } } /** @@ -1114,6 +1155,11 @@ public long inOverlap(long localMillis, Overlap overlap) { public long beforeOverlap(long localMillis, Overlap overlap) { return overlap.previous().localToUtc(roundKey(overlap.firstNonOverlappingLocalTime() - 1, interval) * interval, this); } + + @Override + public String toString() { + return TimeIntervalRounding.this + "[lookup]"; + } } /** @@ -1130,18 +1176,22 @@ public long beforeOverlap(long localMillis, Overlap overlap) { * of dates with the same {@link Prepared} instance. * */ - private class JavaTimeRounding extends TimeIntervalPreparedRounding { + class JavaTimeRounding extends TimeIntervalPreparedRounding { @Override public long round(long originalUtcMillis) { - long utcMillis = originalUtcMillis; - int attempts = 0; - /* + /* * We give up after 5000 attempts and throw an exception. The * most attempts I could get running locally are 500 - for * Asia/Tehran with an 80,000 day range. You just can't declare - * ranges much larger than that in ES right now. + * ranges much larger than that in ES right now. */ - attempt: while (attempts < 5000) { + return round(originalUtcMillis, 5000); + } + + long round(long originalUtcMillis, int maxAttempts) { + long utcMillis = originalUtcMillis; + int attempts = 0; + attempt: while (attempts < maxAttempts) { final Instant utcInstant = Instant.ofEpochMilli(utcMillis); final LocalDateTime rawLocalDateTime = LocalDateTime.ofInstant(utcInstant, timeZone); @@ -1180,9 +1230,8 @@ public long round(long originalUtcMillis) { final OffsetDateTime offsetTime = roundedLocalDateTime.atOffset(currentOffsets.get(0)); final Instant offsetInstant = offsetTime.toInstant(); - throw new IllegalArgumentException( - this + " failed to round " + utcMillis + " down: " + offsetInstant + " is the earliest possible" - ); + assert false : this + " failed to round " + utcMillis + " down: " + offsetInstant + " is the earliest possible"; + return offsetInstant.toEpochMilli(); // TODO or throw something? } else { // The desired time isn't valid because within a gap, so just return the start of the gap ZoneOffsetTransition zoneOffsetTransition = timeZone.getRules().getTransition(roundedLocalDateTime); @@ -1259,6 +1308,11 @@ public long nextRoundingValue(long utcMillis) { ); return round(from); } + + @Override + public String toString() { + return TimeIntervalRounding.this + "[java.time]"; + } } } diff --git a/server/src/test/java/org/elasticsearch/common/RoundingTests.java b/server/src/test/java/org/elasticsearch/common/RoundingTests.java index 7962ef8d164dc..696827df81c6b 100644 --- a/server/src/test/java/org/elasticsearch/common/RoundingTests.java +++ b/server/src/test/java/org/elasticsearch/common/RoundingTests.java @@ -1016,6 +1016,23 @@ public void testHugeTimeInterval() { assertThat(rounding.round(time("2078-11-10T02:51:22.662Z")), isDate(time("1970-01-01T00:00:00+03:30"), tz)); } + public void testHugeTimeFewAttempts() { + ZoneId tz = ZoneId.of("Asia/Tehran"); + Rounding.TimeIntervalRounding.JavaTimeRounding prepared = (Rounding.TimeIntervalRounding.JavaTimeRounding) Rounding.builder( + TimeValue.timeValueDays(80000) + ).timeZone(tz).build().prepareJavaTime(); + Exception e = expectThrows(IllegalArgumentException.class, () -> prepared.round(time("2178-11-10T02:51:22.662Z"), 200)); + assertThat( + e.getMessage(), + equalTo( + "Rounding[" + + TimeValue.timeValueDays(80000).millis() + + " in Asia/Tehran][java.time] failed to round 3446656199999 down: " + + "transitioned backwards through too many daylight savings time transitions" + ) + ); + } + public void testFixedIntervalRoundingSize() { Rounding unitRounding = Rounding.builder(TimeValue.timeValueHours(10)).build(); Rounding.Prepared prepared = unitRounding.prepare(time("2010-01-01T00:00:00.000Z"), time("2020-01-01T00:00:00.000Z"));