Parse time zone info in Rust on Unix systems #677

djc · 2022-04-12T12:13:28Z

Fixes #499 by vendoring a heavily modified copy of tz-rs.

Why we don't use tz-rs directly:

tz-rs only supports 1.46+, whereas we get to support 1.32+
Supply chain: the tz-rs maintainer is relatively unknown, and their GitHub profile is fairly opaque
By constraining the capabilities to what chrono needs, the tz_info module is simpler than tz-rs

Alternative solutions: like time, we could check for concurrently running threads before running localtime_r(), leaving tzinfo handling to libc. Using well-tested Rust code to parse the timezone info files seems preferable to me over platform-specific hacks to find out if other threads are running.

@x-hgg-x thanks for your work on tz-rs!

Dushistov · 2022-04-12T14:39:30Z

Any reason to use once_cell instead of std::sync::Once?

x-hgg-x · 2022-04-12T16:13:05Z

For information, since the last release of tz-rs, I have added additional checks in the AlternateTime constructor, which are necessary for this portion of code to work:

Limit the UTC offset of the LocalTimeType fields between -25h and 26h, which is the maximum allowed range when parsing a POSIX TZ string.
Check for DST transition rules consistency: this ensures that the DST start and end time are always in the same order for each year. Otherwise, there will be an additional implementation-defined transition at the year boundary when the order is not the same between two consecutive years. This additional transition occurs at Jan 1 00:00:00 UTC for glibc and at Jan 1 00:00:01 UTC for musl libc, which is not what the TZ string would suggest.

I also added some documentation mentioning what a value of Julian0WithLeap(365) means for a non-leap year.

djc · 2022-04-12T17:53:25Z

Any reason to use once_cell instead of std::sync::Once?

I wasn't aware of that! I found the interface substantially more painful but for this purpose it makes sense, so this PR now no longer pulls in once_cell. Thanks for the pointer!

For information, since the last release of tz-rs, I have added additional checks in the AlternateTime constructor, which are necessary for this portion of code to work:

Thanks, I will definitely be watching your follow-up commits after this is merged.

esheppa · 2022-04-13T13:00:43Z

Hi @djc - I've had an attempt to fix my above comments here and also added some more tests: https://github.com/esheppa/chrono/tree/tz-info-extra

(Please note that I've not yet fixed at least two areas with the *_from_local functions: AlternativeTime::find_local_time_type_from_local (local to and from UTC adjustments where necessary) and TimeZoneRef::find_local_time_type_from_local (leap second handling))

djc · 2022-04-13T13:10:11Z

@esheppa I'm not seeing your "above comments" -- did you forget to submit them? If you have a chance, could you submit any fixes as a PR that targets this PR's branch?

esheppa · 2022-04-13T12:29:56Z

src/offset/local/unix.rs

+
+pub(super) fn naive_to_local(d: &NaiveDateTime, local: bool) -> DateTime<Local> {
+    let offset = match local {
+        true => offset(d.timestamp()),


the d.timestamp() in this case is in local timezone, not in UTC, so it needs to be adjusted back (or forward) to UTC before it can be passed to the offset function. Alternatively, a custom version of find_local_time_type could be implemented which takes a local timestamp instead of a UTC timestamp.

I just published the version 0.6.8 of tz-rs where I have added a method DateTime::find() which covers this use case.

Nice @x-hgg-x - I tried this out on my old branch and it passes all the tests locally (however I'd need to properly handle leap seconds)

I see that you used unwrap(), which will panic if the datetime is after the last transition and there is no extra rule.

esheppa · 2022-04-13T12:31:10Z

src/offset/local/unix.rs

+        false => FixedOffset::east(0),
+    };
+
+    DateTime::from_utc(*d - offset, offset)


where d is a UTC timestamp we still need to provide the local offset, for example:

DateTime::from_utc(*d - offset, offset(d.timestamp()))

esheppa · 2022-04-13T21:57:04Z

I forgot to submit before, I've now submitted the comments and created PR #678

djc · 2022-05-05T20:47:33Z

@esheppa CI passed on #682, any clue why it failed after merging?

esheppa · 2022-05-06T12:32:19Z

@djc - from what I can see #682 never ran on CI due to targeting this branch rather than main - I've temporarily switched #683 to compare with main so that CI can run there, then if that succeeds, I can change it back to target this branch

esheppa · 2022-05-13T11:43:22Z

@djc - I've triggered the CI to test this at #689 however due to a rebase it can't be merged into this branch. I've created #690 which compresses all the localtime changes into one commit and targets this branch. I've temporarily edited the test.yml on #690 so that the CI runs.

Based on tz-rs commit b8675e2b5794d923f80970daf9ea548cfb998db2, with substantial modifications from me.

djc · 2022-06-09T08:43:02Z

See #674 for next release planning.

See the release announcement: - https://github.com/chronotope/chrono/releases/tag/v0.4.20 It looks like the fix for RUSTSEC-2020-0159 vendors much of the relevant code from `tz-rs` (which Artichoke already uses): - chronotope/chrono#677 Previous `cargo deny` error (I think this started triggering today now that there is a fixed version out): ```console $ cargo deny check error[A001]: Potential segfault in `localtime_r` invocations ┌─ /Users/lopopolo/dev/artichoke/artichoke/Cargo.lock:15:1 │ 15 │ chrono 0.4.19 registry+https://github.com/rust-lang/crates.io-index │ ------------------------------------------------------------------- security vulnerability detected │ = ID: RUSTSEC-2020-0159 = Advisory: https://rustsec.org/advisories/RUSTSEC-2020-0159 = ### Impact Unix-like operating systems may segfault due to dereferencing a dangling pointer in specific circumstances. This requires an environment variable to be set in a different thread than the affected functions. This may occur without the user's knowledge, notably in a third-party library. ### Workarounds No workarounds are known. ### References - [time-rs/time#293](time-rs/time#293) = Announcement: chronotope/chrono#499 = Solution: Upgrade to >=0.4.20 = chrono v0.4.19 ├── chrono-tz v0.6.1 │ └── spinoso-time v0.5.0 │ └── artichoke-backend v0.13.0 │ └── artichoke v0.1.0-pre.0 └── spinoso-time v0.5.0 (*) advisories FAILED, bans ok, licenses ok, sources ok ```

djc force-pushed the tz-info branch 5 times, most recently from 43a4ff9 to c0e806e Compare April 12, 2022 13:49

djc marked this pull request as ready for review April 12, 2022 13:50

This was referenced Apr 12, 2022

Localtime using tz-rs #656

Closed

The call to localtime_r may be unsound #499

Closed

0.4.20 release planning #674

Closed

djc force-pushed the tz-info branch 3 times, most recently from 78b2835 to 69169ec Compare April 12, 2022 15:33

djc requested a review from Milo123459 April 12, 2022 15:36

esheppa reviewed Apr 13, 2022

View reviewed changes

tukantje mentioned this pull request Apr 29, 2022

RUSTSEC-2020-0159: Potential segfault in localtime_r invocations tukantje/zero-to-prod#7

Open

djc mentioned this pull request May 5, 2022

Panic in parse_from_rfc2822 with overflowing date #645

Closed

djc mentioned this pull request May 5, 2022

Fix parse panic #686

Merged

vmx mentioned this pull request May 12, 2022

Wrong timeformat in proving-logs filecoin-project/rust-fil-logger#17

Closed

fussybeaver mentioned this pull request May 12, 2022

stop using chrono fussybeaver/bollard#231

Closed

AlephAlpha mentioned this pull request May 13, 2022

Switch to time AlephAlpha/build-time#1

Closed

djc force-pushed the tz-info branch from 0826d63 to aa5fd59 Compare May 13, 2022 19:31

djc and others added 14 commits June 9, 2022 10:22

sys/unix: remove pointless platform guards

f65d1c7

sys/unix: clarify type of time_to_local_tm()

6ddbdc3

sys/unix: inline uses of Timespec type and methods

7d0ccdb

sys/unix: move duplicate code into renamed localize() function

fd41456

sys/unix: inline more single-use functions

167eb7a

sys/unix: skip conversion to intermediate type

c8dfea8

sys/unix: extract imports to global level

a5354be

sys/unix: reorder functions

f799d1e

sys/unix: inline some more trivial functions

8fac04a

sys/unix: parse tzinfo files to find local time offset

3385709

Based on tz-rs commit b8675e2b5794d923f80970daf9ea548cfb998db2, with substantial modifications from me.

sys/unix: cache time zone info

cb01313

Clarify module hierarchy

72b6220

Test Local functions against date command (#682)

4b0a782

handle localtime ambiguity

b3ff13d

djc force-pushed the tz-info branch from 3e067ff to b3ff13d Compare June 9, 2022 08:22

djc merged commit 50eb7e3 into main Jun 9, 2022

djc deleted the tz-info branch June 9, 2022 08:42

cd-work mentioned this pull request Jun 23, 2022

Reconsider of chrono crate due to https://rustsec.org/advisories/RUSTSEC-2020-0071.html phylum-dev/cli#134

Closed

mikemiles-dev mentioned this pull request Jun 27, 2022

0.50 Release schedule #723

Closed

bspeice mentioned this pull request Jul 27, 2022

Upgrade time crate to 0.3 (CVE-2020-26235) bspeice/dtparse#39

Closed

Byron mentioned this pull request Aug 5, 2022

The call to localtime_r may be unsound time-rs/time#293

Closed

lopopolo mentioned this pull request Aug 5, 2022

Upgrade chrono to 0.4.20 to address RUSTSEC-2020-0159 artichoke/artichoke#2044

Merged

x-hgg-x mentioned this pull request Aug 24, 2022

Add additional checks for AlternateTime #789

Open

tqwewe mentioned this pull request Jan 16, 2023

RUSTSEC-2020-0071: Potential segfault in the time crate lunatic-solutions/submillisecond#105

Closed

kdwarn mentioned this pull request Mar 16, 2023

Feature Request: replace chrono waltzofpearls/dateparser#22

Closed

This was referenced Sep 3, 2023

Tzfile support #10

Closed

Clarify localtime and oldtime's docs #626

Closed

Update changelog #1246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse time zone info in Rust on Unix systems #677

Parse time zone info in Rust on Unix systems #677

djc commented Apr 12, 2022 •

edited

Dushistov commented Apr 12, 2022

x-hgg-x commented Apr 12, 2022

djc commented Apr 12, 2022

esheppa commented Apr 13, 2022

djc commented Apr 13, 2022

esheppa Apr 13, 2022

x-hgg-x Apr 13, 2022

esheppa Apr 14, 2022

x-hgg-x Apr 14, 2022

esheppa Apr 13, 2022

esheppa commented Apr 13, 2022

djc commented May 5, 2022

esheppa commented May 6, 2022

esheppa commented May 13, 2022

djc commented Jun 9, 2022

Parse time zone info in Rust on Unix systems #677

Parse time zone info in Rust on Unix systems #677

Conversation

djc commented Apr 12, 2022 • edited

Dushistov commented Apr 12, 2022

x-hgg-x commented Apr 12, 2022

djc commented Apr 12, 2022

esheppa commented Apr 13, 2022

djc commented Apr 13, 2022

esheppa Apr 13, 2022

Choose a reason for hiding this comment

x-hgg-x Apr 13, 2022

Choose a reason for hiding this comment

esheppa Apr 14, 2022

Choose a reason for hiding this comment

x-hgg-x Apr 14, 2022

Choose a reason for hiding this comment

esheppa Apr 13, 2022

Choose a reason for hiding this comment

esheppa commented Apr 13, 2022

djc commented May 5, 2022

esheppa commented May 6, 2022

esheppa commented May 13, 2022

djc commented Jun 9, 2022

djc commented Apr 12, 2022 •

edited