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

Fixed integer overflow in NumericDate.MarshalJSON #200

Merged

Conversation

qqiao
Copy link
Contributor

@qqiao qqiao commented May 2, 2022

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 them
separately, and then combining the resulting strings.

This approach allows us to correctly marshal all go time.Time instances,
whereas using Time.Unix won't give us enough precision, and either
Time.UnixMicro or Time.UnixNano would overflow at some point for
even valid time.Time instances.

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) will
simply be 0.123456 instead of 0.123456 + small_delta.

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.
Copy link
Collaborator

@oxisto oxisto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay on this. I was quite busy for personal reasons for a while. Looks good overall, just some minor details like variable naming and some comments would be nice.

types.go Outdated Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@oxisto oxisto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the documentation.

Copy link
Member

@mfridman mfridman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, thanks for submitting this. LGTM.

@mfridman mfridman merged commit 2da0bf7 into golang-jwt:main Jun 4, 2022
@qqiao qqiao deleted the fix-numeric-date-marshaljson-overflow branch June 4, 2022 18:40
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NumericDate.MarshalJSON overflows on large unix timestamps
3 participants