Skip to content

Prevent stack overflow in rounding #80450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 10, 2021
Merged

Prevent stack overflow in rounding #80450

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2577080
Prevent stack overflow in rounding
nik9000 Nov 2, 2021
7f73d0f
Explain
nik9000 Nov 5, 2021
6265362
Assert exception
nik9000 Nov 7, 2021
7224308
Merge branch 'master' into rounding_too_many
nik9000 Nov 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 59 additions & 40 deletions server/src/main/java/org/elasticsearch/common/Rounding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,50 +1132,69 @@ 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<ZoneOffset> 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;
/*
* 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) {
Copy link
Contributor

@grcevski grcevski Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can make this constant a field that can be modified with the builder which defaults to 5000? That way we can write a unit test that ensures we correctly throw the exception in case the loop limit is reached.

Copy link
Member Author

@nik9000 nik9000 Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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<ZoneOffset> 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(
Copy link
Contributor

@grcevski grcevski Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is changing behaviour and it's unclear to me how we end up in this state. I'd be concerned that a customer is successfully getting a date now, where their application will effectively break after this change? Is there a way to reproduce this with a unit test? If not maybe we can take the approach to log an error/warning and retain the existing behaviour of returning a value?

Copy link
Member Author

@nik9000 nik9000 Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes. I made this change earlier, took a day off, and then forgot about it. I'll revert it. I don't particularly like the old behavior. And, no, didn't have a way of getting this behavior before with a unit test. I'll just set it back how it was.

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 "
Copy link
Contributor

@grcevski grcevski Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please log the timezone and the interval in the exception message if this happens? This way if we have missed a case that we are not aware of we'll at least be able to debug it. Perhaps a logger warning too?

Copy link
Member Author

@nik9000 nik9000 Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make sure it gets in there. I think folks intended this.toString to have the zone but I expect that got lost and the lack of unit test didn't notice.

+ utcMillis
+ " down: transitioned backwards through too many daylight savings time transitions"
);
}

@Override
Expand Down
6 changes: 6 additions & 0 deletions server/src/test/java/org/elasticsearch/common/RoundingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Copy link
Member Author

@nik9000 nik9000 Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't write a test locally that'd fail without going to pretty intense lengths. So I just used the worse case I could find.


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"));
Expand Down