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

Localtime using tz-rs #656

Closed
wants to merge 15 commits into from
Closed

Conversation

esheppa
Copy link
Collaborator

@esheppa esheppa commented Mar 14, 2022

This is a potential way to solve the CVE-2020-0159. This uses the library tz-rs to read the time zone information file and calculate the local offset from the information contained within.

tz-rs is quite comprehensive as leap second handling has been implemented, however there are some outstanding questions on how to handle this in from_local_datetime (should we assume the local datetime does or doesn't include leap seconds?).

Thanks for contributing to chrono!

  • Have you added yourself and the change to the changelog? (Don't worry
    about adding the PR number)
  • If this pull request fixes a bug, does it add a test that verifies that
    we can't reintroduce it?

@djc
Copy link
Contributor

djc commented Mar 17, 2022

Thanks for working on this! I'm not yet sure if this is the right direction, will have to dig into it more.

@esheppa
Copy link
Collaborator Author

esheppa commented Mar 18, 2022

Thanks for enabling CI @djc - I'll try and get the CI passing - then I recognize there are a bunch more things to figure out, this is intended as a proof of concept. If this is later decided to be a good solution, I'll go through and add more documentation etc.

Potential issues:

  • Minimum supported rust version: This would require some changes to not affect chronos MSRV - but these would be solvable, for example via PR'ing to the timezone file libraries to reduce their MSRV to match that of chrono (or alternatively increasing chronos MSRV, but this may be less preferable)
  • Extra dependency: there isn't really any way around this unless the code of a TZif parsing library is vendored into chrono itself.

Other options:

  • Providing this integration as a separate crate, potentially alongside removing or altering the Local functionality for unix systems in the main chrono crate.

@x-hgg-x
Copy link

x-hgg-x commented Mar 21, 2022

For information, I have reduced the MSRV to rustc 1.45 for the version v0.6.6 of tz-rs.

@esheppa
Copy link
Collaborator Author

esheppa commented Mar 24, 2022

Thanks @x-hgg-x - I've updated to the latest version of tz-rs

src/offset/local.rs Outdated Show resolved Hide resolved
@djc
Copy link
Contributor

djc commented Apr 12, 2022

Thanks for working on this! Unfortunately, I'm inclined to take a different direction, see #677. Would be happy to get your feedback.

@esheppa
Copy link
Collaborator Author

esheppa commented Apr 13, 2022

Thanks @djc - I'll close this and leave some comments on #677.

@esheppa esheppa closed this Apr 13, 2022
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.

None yet

4 participants