Fixed integer overflow in NumericDate.MarshalJSON #200
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The original issue was caused by the fact that if we use a very large unix timestamp. The resulting value from time.UnixNano overflows a int64, as documented here: https://pkg.go.dev/time#Time.UnixNano. This patch works around the issue by calculating the second part and nanosecond part separately, taking the second part from time.Unix, and the nanosecond part from time.Nanosecond and then adding the results together.
We now format the whole part and the decimal part separately. Since the whole part is by definition, we need not covert it to a float, hence the whole part can simply be formatted as an integer. The and since by definition of the `Time.Nanoseconds()`, the return value is between 0-999999999, only this part needs to be formatted separately. We then combine whole + decimals[1:] to form the final result. This allows us to correctly format the maximun timestamp that is allowed by Go.
oxisto
reviewed
May 28, 2022
used to marshal times. Improved variable naming and fixed typos.
oxisto
approved these changes
May 31, 2022
mfridman
approved these changes
Jun 4, 2022
oxisto
pushed a commit
to moneszarrugh/jwt
that referenced
this pull request
Feb 21, 2023
oxisto
pushed a commit
to twocs/jwt
that referenced
this pull request
Mar 29, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixed #199
The original issue was caused by the fact that if we use a very large
unix timestamp. The resulting value from
Time.UnixNano()overflows a int64, as documented here: https://pkg.go.dev/time#Time.UnixNano.
This patch works around the issue by treating the second part and
nanosecond part separately, taking the second part from
Time.Unix(),and the nanosecond part from
Time.Nanosecond(), formatting themseparately, and then combining the resulting strings.
This approach allows us to correctly marshal all go
time.Timeinstances,whereas using
Time.Unixwon't give us enough precision, and eitherTime.UnixMicroorTime.UnixNanowould overflow at some point foreven valid
time.Timeinstances.The added benefit of this approach is that we no longer have to deal
with the added nanosecond delta that Go uses in order to achieve
monotonic time, so the result of marshaling
time.Unix(0, 123456)willsimply be
0.123456instead of0.123456 + small_delta.