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

Integer overflow with parse_rfc3339_ns #4098

Closed
morgante opened this issue Dec 6, 2021 · 4 comments · Fixed by #4117
Closed

Integer overflow with parse_rfc3339_ns #4098

morgante opened this issue Dec 6, 2021 · 4 comments · Fixed by #4117

Comments

@morgante
Copy link

morgante commented Dec 6, 2021

Short description

If you attempt to parse a date in the far future, parse_rfc3339_ns returns a negative number (I assume caused by integer overflow).

Example:

b := "9999-12-31T23:59:59Z"
b_v := time.parse_rfc3339_ns(b)
    "b_v": -4852116232933722000,

Expected behavior

I expect it to either raise an error or return the correct number.

@anderseknert
Copy link
Member

Ah, seems like the max (9223372036854775807) for the int64 data type used by Go is passed after 9276-12-03 .. we can at least be happy we won't be around to deal with all the systems crashing at that date ;)

I guess raising an error would be the right thing to do here, unless we'd want to replace all the calls to time functions with something that handles later dates.

@srenatus
Copy link
Contributor

srenatus commented Dec 6, 2021

I guess raising an error would be the right thing to do here, unless we'd want to replace all the calls to time functions with something that handles later dates.

I'd propose a non-halting built-in error. It's what we do with weird inputs in other places. It'll cause the builtin to go undefined, and raise an error if running with "strict builtin errors" enabled. (We could really use #3742.)

@anderseknert
Copy link
Member

Interestingly, the docs on time.UnixNano says the following:

UnixNano returns t as a Unix time, the number of nanoseconds elapsed since January 1, 1970 UTC. The result is undefined if the Unix time in nanoseconds cannot be represented by an int64 (a date before the year 1678 or after 2262). Note that this means the result of calling UnixNano on the zero Time is undefined. The result does not depend on the location associated with t.

I'm not sure in what way they mean the result is undefined.. I guess that's what's undefined 😄 But if that's the case we might want to take even that into account.

@tsandall
Copy link
Member

tsandall commented Dec 6, 2021

I agree with @srenatus here--I think that the function should generate a non-halting error in this case. That should be a relatively small change. Moving it into the plan for the next release.

@tsandall tsandall moved this from Backlog to Planned - v0.36 in Open Policy Agent Dec 6, 2021
@johanfylling johanfylling self-assigned this Dec 8, 2021
@tsandall tsandall moved this from Planned - v0.36 to In Progress in Open Policy Agent Dec 9, 2021
johanfylling added a commit to johanfylling/opa that referenced this issue Dec 9, 2021
The go Time.UnixNano() function result is undefined for dates that
cannot fit into an int64 when converted to Unix time in nanoseconds.

Updating built-in functions to return error if date is too low/high.

Fixes: open-policy-agent#4098
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Open Policy Agent automation moved this from In Progress to Done Dec 10, 2021
johanfylling added a commit that referenced this issue Dec 10, 2021
#4117)

* topdown: Fixing nanos int overflow issue for time.* built-in functions

The go Time.UnixNano() function result is undefined for dates that
cannot fit into an int64 when converted to Unix time in nanoseconds.

Updating built-in functions to return error if date is too low/high.

Fixes: #4098
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

5 participants