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 · 10 comments
Open

time: unchecked overflow in Add and AddDays #20678

bcmills opened this issue Jun 14, 2017 · 10 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bcmills
Copy link
Contributor

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

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

@bradfitz bradfitz added this to the Go1.10 milestone Jun 14, 2017
@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 14, 2017
@bcmills
Copy link
Contributor Author

bcmills commented Jun 14, 2017

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

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

@rsc
Copy link
Contributor

rsc 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
Copy link
Contributor Author

bcmills 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.

@ghost
Copy link

ghost commented Jun 20, 2017

Fixed the link: Stelliferous Era

@rsc
Copy link
Contributor

rsc 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 The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jun 26, 2017
@golang golang deleted a comment from Allthewaylive247 Jul 12, 2017
@bcmills
Copy link
Contributor Author

bcmills 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).

@bcmills
Copy link
Contributor Author

bcmills commented Aug 15, 2019

Ah, fun: there may have been an undetected overflow in Sub and/or Unix too:
https://play.golang.org/p/Tdo-5VgCJto reported -1s in Go 1.12, even though the dates are supposed to be 64 bits apart!

If the overflow was in Sub, it may have been fixed in CL 131196 (#17858), although I haven't verified either way.

(CC @pongad @martisch @rasky )

@bcmills
Copy link
Contributor Author

bcmills commented Aug 15, 2019

Looks like there is definitely an overflow in Unix, and it is not fixed in go1.13beta1.

~/tmp$ go1.13beta1 run main.go
true
292277026304-08-26 10:42:50.145224192 -0500 EST - 292277026304-08-26 10:42:51.145224192 -0500 EST = -1s

@rsc
Copy link
Contributor

rsc commented Apr 26, 2022

If we do anything more, I think we should clamp the year to |y|<100e9, similar to absl (same as absl? unclear).
There is definitely an overflow in Unix if you pass in a large positive value,
since Unix adds ~62e9 seconds to get the internal (Jan 1 year 1-based) representation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants