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: Integer overflow in Date #56909

Open
sylvainpelissier opened this issue Nov 22, 2022 · 10 comments
Open

time: Integer overflow in Date #56909

sylvainpelissier opened this issue Nov 22, 2022 · 10 comments
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Unfortunate
Milestone

Comments

@sylvainpelissier
Copy link

sylvainpelissier commented Nov 22, 2022

What version of Go are you using (go version)?

$ go version
go version go1.19.3 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/sylvain/.cache/go-build"
GOENV="/home/sylvain/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/sylvain/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/sylvain/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3696417096=/tmp/go-build -gno-record-gcc-switches"

What did you do?

The function Date of package time has an integer overflow. The year number is used to compute the number of days since epoch and then multiply be the number of second in a day: https://cs.opensource.google/go/go/+/refs/tags/go1.19.3:src/time/time.go;l=1496;drc=4b43b741710eb87cbae25f19cbde7eb733b08df1 without check. If the year number is above 549755813887 it overflows the number of second and the year number is negative.

Proof of concept: https://go.dev/play/p/9dteWLSkjdn

It seems to be related to the issue #20678 but the integer overflow is reached with values on 39 bits only. It allows to have date far in the future seeing in the past.

What did you expect to see?

A positive date and true as output or an error.

What did you see instead?

-34798235367-01-21 16:59:44 +0000 UTC
false
@sylvainpelissier sylvainpelissier changed the title affected/package: Integer overflow in time.Date time: Integer overflow in Date Nov 22, 2022
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Nov 22, 2022
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Nov 22, 2022
@bcmills
Copy link
Contributor

bcmills commented Nov 22, 2022

See previously #5923, #6617, #32501, #36202.

That said, I don't foresee any realistic programs trying to work with dates 500 billion years in the future, and unlike with Add there isn't an obvious saturation behavior. Should an out-of-range Date call return the zero time.Time, or saturate to the largest representable (positive or negative) value?

(Arguably Date should return (time.Time, error) instead of just a raw time.Time, but unfortunately that doesn't seem feasible at this point.)

@sylvainpelissier
Copy link
Author

In my opinion the problem is in the After or Before function results which are wrong with this behavior.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 22, 2022
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Nov 22, 2022
@yasunaga-shuto
Copy link

yasunaga-shuto commented Nov 26, 2022

I investigated this issue and I found that year 292277024628 is the border which results wrong time.After.

And -292277024626 year is the border which results wrong time.Before.

https://go.dev/play/p/pVxa94ZL6t9

I think the reason is that time's seconds from 292277024628 year since Jan 1 year 1 is overflown. After function uses
seconds since Jan 1 year 1, so it probably went wrong.

this issue is related?🤔

#48608

I tried to handle this like below(with tests). Maybe, this is not smart enough and makes a little confusing. I thought there's a way to make panic when passing overflown year. What do you think?

const (
	maxYear = 292277024627
	minYear = -292277024626
)
func Date(year int, month Month, day, hour, min, sec, nsec int, loc *Location) Time {
	if loc == nil {
		panic("time: missing Location in call to Date")
	}

	// Normalize month, overflowing into year.
	m := int(month) - 1
	year, m = norm(year, m, 12)
        // add these
	if year >= maxYear {
		year = maxYear
	} else if year <= minYear {
		year = minYear
	}

ps:

When passing negative year to time.Date, up to -292277022399 holds negative information, but -292277022400 year went positive value (292277026853). This changes minYear like below (or parse negative year to zero?)

minYear = -292277022399

@gopherbot
Copy link

Change https://go.dev/cl/453475 mentions this issue: time: fix years overflow when using time.Date

@bcmills
Copy link
Contributor

bcmills commented Nov 29, 2022

Ideally we would have the invariant that for a time.Time returned by a call to time.Date, the Date, Hour, Minute, Second, and Nanosecond methods all return the same values that were passed to Date in the first place.

That invariant is not possible to provide with the current time.Time representation, which has a range that depends on the presence or absence of a monotonic timestamp:

  • without a monotonic timestamp, it can represent years in the range of approximately 1±2.92×1011 (1 Jan 1 ± <263 seconds), or about ±238 years.
  • with a monotonic timestamp, it can represent years in the range of approximately 2021±136 (1 Jan 1885 + <233 seconds)

time.Date does not supply a monotonic timestamp, so it is possible to provide the round-trip invariant only for years within ±238, whereas the int type on most platforms allows the caller to pass years within ±264.

It would be possible to expand that range to without increasing the size of the time.Time type by extending the “seconds” field into the 33 bits that are currently unused in the wall field for non-monotonic times. However, that would still only provide a range of 246 years, not the full 264 impled by the year int parameter type: it's not a viable solution, and would add a lot of complexity in order to support a very niche use-case.

And it doesn't seem worth making the time.Time type larger to handle this use-case either, since the years that overflow are so far outside the bounds of what essentially any realistic Go program needs to represent.


We could consider instead the invariant that a call to time.Date(year, month, day, hour, min, sec, nsec, loc) is equivalent to a call to time.Date(0, month, day, hour, min, sec, nsec, loc).AddDate(year, 0, 0). However, that invariant is only sensible at all if we simultaneously define the overflow behavior of AddDate (addressing #20678) — otherwise, we still have the same overflow problem, just at a different point in the code.

Even then, it's not at all obvious to me that that invariant would be intuitive or useful for most Go users — and if callers explicitly want behavior equivalent to the AddDate call sequence, they can write it that way to begin with!


So it seems to me that the most straightforward solution is to define that Date returns either a time.Time for the requested date (if in range) or the zero time.Time (otherwise). Then a caller could use IsZero as an inexpensive check for validity and, if they also need to accept the zero time.Time as a valid input, fall back to comparing the Year:

t := time.Date(year, month, day, hour, min, second, nsec, loc)
if t.IsZero() && t.Year() != year { // Just t.Year() != year would suffice, but IsZero is cheaper.// Date out of range.
} 

Then the caller could easily decide what to do about the problem: they could explicitly choose whether to return an error, panic, cap to arbitrary limits, or fall back to the AddDate sequence.

@yasunaga-shuto
Copy link

yasunaga-shuto commented Nov 30, 2022

So it seems to me that the most straightforward solution is to define that Date returns either a time.Time for the requested date (if in range) or the zero time.Time (otherwise).

Thank you for advise and I learned a lot. This sounds great, but what about comparing two times?
When out of range time.Date returns Zero time, so does this code output false?

t1 := time.Date(292277024628, 1, 1, 0, 0, 0, 0, time.UTC) // overflown years
t2 := time.Date(1, 1, 1, 0, 0, 0, 0, time.UTC)
fmt.Printf("after: %v", t1.After(t2)) // -> false ?

ps:
I'm sorry if my phrasing looks rude.

@bcmills
Copy link
Contributor

bcmills commented Nov 30, 2022

… what about comparing two times?
When out of range time.Date returns Zero time, so does this code output false?

If the caller doesn't already know that their parameters are in a reasonable range, they must check that the result from Date is valid before using it either way. Even if time.Date saturated to the highest representable date, there would still be edge-cases where comparisons give unexpected answers.

For example, consider a similar program comparing two very-far-future dates:

t1 := time.Date(292277024629, 1, 1, 0, 0, 0, 0, time.UTC)
t2 := time.Date(292277024628, 1, 1, 0, 0, 0, 0, time.UTC)
fmt.Printf("after: %v", t1.After(t2))  // false‽

@yasunaga-shuto
Copy link

yasunaga-shuto commented Dec 1, 2022

Even if time.Date saturated to the highest representable date, there would still be edge-cases where comparisons give unexpected answers.

yes, exactly

@tebeka
Copy link
Contributor

tebeka commented Nov 23, 2023

We can do what Python does, we can define MaxTime and MinTime and then panic of Date tries to create something out of these values.

@mujinsong
Copy link

mujinsong commented Feb 7, 2024

So it seems to me that the most straightforward solution is to define that Date returns either a time.Time for the requested date (if in range) or the zero time.Time (otherwise).

Thank you for advise and I learned a lot. This sounds great, but what about comparing two times? When out of range time.Date returns Zero time, so does this code output false?

t1 := time.Date(292277024628, 1, 1, 0, 0, 0, 0, time.UTC) // overflown years
t2 := time.Date(1, 1, 1, 0, 0, 0, 0, time.UTC)
fmt.Printf("after: %v", t1.After(t2)) // -> false ?

ps: I'm sorry if my phrasing looks rude.

To be honest, I think it's better to return maxYear after overflow, that big number gives more idea of what have happened than zero time.

ps:
I'm sorry if my phrasing looks rude.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Unfortunate
Projects
None yet
Development

No branches or pull requests

7 participants