Skip to content
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.

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

time: unchecked overflow in Add and AddDays #20678

Open
bcmills opened this Issue Jun 14, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@bcmills
Copy link
Member

commented Jun 14, 2017

I'm trying to define a mapping between time.Time and a C++ time library.

The library that I'm trying to map to supports distinct "infinite past" and "infinite future" times, which need to be mapped to distinct time.Time values. The logical choices would seem to be the maximum and minimum representable time.Time values.

One way to try to obtain those is to call (time.Time).AddDate with absurdly positive or absurdly negative values. AddDate does not return an error, and it cannot reasonably panic on overflow (because the package does not define a way for users to check for such an overflow ahead of time). That leaves one "obvious" behavior: saturation.

Sadly, the current implementation fails to provide that behavior, and instead silently overflows to nonsensical values (https://play.golang.org/p/UUC2JG7Xcj).

(Further evidence for #19624?)

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 14, 2017

What is the C++ time library? Got a link?

@bradfitz bradfitz added this to the Go1.10 milestone Jun 14, 2017

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2017

What is the C++ time library? Got a link?

I'm not sure whether it's public. So, no.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2017

time.Duration does do saturated arithmetic. For time.Time that really seems like overkill. There's no reasonable way you could end up with times that large anyway.

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2017

There's no reasonable way you could end up with times that large anyway.

The time.Time data structure itself is surely capable of representing usefully-distant values: int64 seconds gives a range of ~292 billion years in both directions, which is longer than virtually all estimates for the current age of the universe (albeit shorter than some estimates of the remaining duration of the Stelliferous Era). So having limits on the range supported by the implementation seems fine.

The trouble is, at the moment those limits are ad-hoc: the point at which methods stop working is a complicated function of both the method (AddDate, Add, String, and Unix all break down at different points) and CPU architecture (specifically, the size of int), and many of the individual methods break down long before the limits inherent to the representation.

Some points of interest are shown here: https://play.golang.org/p/-WgwEfN1Vs

In practice, it appears that the portable range is limited by the functions involving dates, which break down outside the range [-2147483648-01-01 00:00:00, 2147483647-12-31 23:59:59.999999999] on 32-bit platforms. That gives a minimum time.Time value that lies well short of most scientific estimates for the age of the universe, one measure of "reasonable" times. (For example, you could imagine attempting to use time.Time to sort a list of events interesting to humans, starting with the Big Bang.)


For the use-case I have in mind, I explicitly need two distinct unreasonable times: one far enough in the past to represent "infinite past", and one far enough in the future to represent "infinite future". It doesn't really matter what the specific values are, but they need to be well-behaved w.r.t. all "reasonable" values: for example, infiniteFuture.AddDate(<any reasonable offset>).After(<any reasonable date>) and infiniteFuture.Add(<any reasonable duration>).After(<any reasonable date>) must always be true.

It would be nice to also have infiniteFuture.Add(<any reasonable duration>).Equal(infiniteFuture) , but with enough space between the "reasonable" and "infinite" values I can probably work around that. I could consider values of extreme magnitude at either end of the range to be "infinite", but the overflow issues in the date functions in particular make me worry about "infinite" values overflowing into the "reasonable" range.

@opennota

This comment has been minimized.

Copy link

commented Jun 20, 2017

Fixed the link: Stelliferous Era

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2017

If you want to make a proposal for the proper min/max and do the implementation, I guess it is OK with me. It does match Duration.

@rsc rsc added NeedsFix and removed NeedsDecision labels Jun 26, 2017

@golang golang deleted a comment from Allthewaylive247 Jul 12, 2017

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2017

What is the C++ time library? Got a link?

I'm interested in interoperating with two libraries in particular:

  1. absl::Time, which supports “precision of at least one nanosecond, and range +/-100 billion years”. It implements saturating integer arithmetic.

  2. std::chrono, which represents counts of ticks parameterized on both the representation and the tick period. It implements saturating arithmetic if the underlying representation saturates (e.g., is a floating-point type).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.