time: Integer overflow in Date

sylvainpelissier commented 1 year ago

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

bcmills commented 1 year ago

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 commented 1 year ago

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

yasunaga-shuto commented 1 year ago

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?🤔

https://github.com/golang/go/issues/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 commented 1 year ago

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

bcmills commented 1 year ago

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×10¹¹ (1 Jan 1 ± <2⁶³ seconds), or about ±2³⁸ years.
with a monotonic timestamp, it can represent years in the range of approximately 2021±136 (1 Jan 1885 + <2³³ seconds)

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

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 2⁴⁶ years, not the full 2⁶⁴ 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 commented 1 year ago

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 commented 1 year ago

… 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 commented 1 year ago

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

yes, exactly

tebeka commented 10 months ago

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 commented 7 months ago

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.

golang / go