From 08abeadd090e1f309eaed9bd3efa5947073c8d1e Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Sat, 17 May 2025 10:22:58 -0400 Subject: [PATCH] tz: fallback to `Etc/Unknown` when `TZ` is set to an invalid value Previously, when `TZ` was set to an invalid value, Jiff would still attempt to detect the system configured time zone. But this is arguably not great, because it silently (aside from logs) rejects an invalid `TZ` value. In reality, if `TZ` is set, it is likely that the user intends for it to have an impact. So if it doesn't, we should bleat about it. This manifests as an error when using `TimeZone::try_system()` and manifests as a error sentinel in the form of `Etc/Unknown` when using `TimeZone::system()`. We also tweak some of the logging levels. Namely, in #370, I increased the number of TRACE-level log messages, which makes it much noisier. So I've promoted a few things that were TRACE to DEBUG without making the output much noisier. I guess TRACE should be reserved for variable length things. Fixes #364 --- .github/workflows/ci.yml | 2 + CHANGELOG.md | 2 + examples/uptime/main.rs | 12 ++-- scripts/test-programs | 14 +++++ src/tz/system/mod.rs | 60 +++++++++++++------ src/tz/timezone.rs | 10 +++- .../Cargo.toml | 18 ++++++ .../invalid-tz-environment-variable/README.md | 8 +++ .../invalid-tz-environment-variable/main.rs | 33 ++++++++++ 9 files changed, 136 insertions(+), 23 deletions(-) create mode 100755 scripts/test-programs create mode 100644 testprograms/invalid-tz-environment-variable/Cargo.toml create mode 100644 testprograms/invalid-tz-environment-variable/README.md create mode 100644 testprograms/invalid-tz-environment-variable/main.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ad7e3a2..8d970f6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -348,6 +348,8 @@ jobs: toolchain: stable - name: Build all examples run: ./scripts/test-examples + - name: Run all test programs + run: ./scripts/test-programs # This job builds and runs tests for Jiff's "integration" crates. That is, # the crates that provide wrappers or traits for integrating with other diff --git a/CHANGELOG.md b/CHANGELOG.md index 74e2095..af7e494 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ Bug fixes: * [#365](https://github.com/BurntSushi/jiff/issues/365): Fixes a compile error in Jiff when only the `tzdb-concatenated` feature was enabled. +* [#364](https://github.com/BurntSushi/jiff/issues/364): +Jiff now falls back to `Etc/Unknown` for invalid `TZ` values. * [#366](https://github.com/BurntSushi/jiff/issues/366): Fixes slow initial `Zoned::now()` in environments where `/usr/share/zoneinfo` is on a very slow file system (like CI environments). diff --git a/examples/uptime/main.rs b/examples/uptime/main.rs index 787a3d5..1b07a3c 100644 --- a/examples/uptime/main.rs +++ b/examples/uptime/main.rs @@ -53,7 +53,7 @@ impl UptimeDuration { /// This assumes the output of `uptime` is on stdin. fn find() -> anyhow::Result { let re = Regex::new( - r"(\d+):(\d+)(?::\d+)?\s+up\s+(\d+)\s+days,\s+(\d+):(\d+)", + r"(\d+):(\d+)(?::\d+)?\s+up\s+(?:(\d+)\s+days,\s+|())(\d+):(\d+)", ) .unwrap(); for result in std::io::stdin().lines() { @@ -66,9 +66,13 @@ impl UptimeDuration { let minute: i8 = minute.parse().with_context(|| { format!("failed to parse current minute integer '{minute}'") })?; - let days: i32 = days - .parse() - .with_context(|| format!("failed to parse days '{days}'"))?; + let days: i32 = if days.is_empty() { + 0 + } else { + days.parse().with_context(|| { + format!("failed to parse days '{days}'") + })? + }; let hours: i32 = hours .parse() .with_context(|| format!("failed to parse hours '{hours}'"))?; diff --git a/scripts/test-programs b/scripts/test-programs new file mode 100755 index 0000000..b0d57c6 --- /dev/null +++ b/scripts/test-programs @@ -0,0 +1,14 @@ +#!/bin/bash + +# This script tests that every test program builds. + +set -e + +# cd to the directory containing this crate's Cargo.toml so that we don't need +# to pass --manifest-path to every `cargo` command. +cd "$(dirname "$0")/.." + +for d in ./testprograms/*/; do + echo "===== RUNNING TEST PROGRAMS: $d =====" + RUST_LOG=trace cargo run --manifest-path "$d/Cargo.toml" +done diff --git a/src/tz/system/mod.rs b/src/tz/system/mod.rs index acf7253..8cae45c 100644 --- a/src/tz/system/mod.rs +++ b/src/tz/system/mod.rs @@ -52,7 +52,7 @@ mod sys { match super::read_unnamed_tzif_file(path) { Ok(tz) => Some(tz), Err(_err) => { - trace!("failed to read {path} as unnamed time zone: {_err}"); + debug!("failed to read {path} as unnamed time zone: {_err}"); None } } @@ -145,10 +145,12 @@ pub(crate) fn get_force(db: &TimeZoneDatabase) -> Result { return Ok(tz); } Ok(None) => { - trace!("checked TZ environment variable but found nothing"); + debug!("TZ environment variable is not set"); } - Err(_err) => { - trace!("checked TZ environment variable but got error: {_err}"); + Err(err) => { + return Err(err.context( + "TZ environment variable set, but failed to read value", + )); } } if let Some(tz) = sys::get(db) { @@ -167,6 +169,9 @@ pub(crate) fn get_force(db: &TimeZoneDatabase) -> Result { /// We try very hard to extract a time zone name from `TZ` and use that to look /// it up via `TimeZoneDatabase`. But we will fall back to unnamed TZif /// `TimeZone` if necessary. +/// +/// This routine only returns `Ok(None)` when `TZ` is not set. If it is set +/// but a `TimeZone` could not be extracted, then an error is returned. fn get_env_tz(db: &TimeZoneDatabase) -> Result, Error> { // This routine is pretty Unix-y, but there's no reason it can't // partially work on Windows. For example, setting TZ=America/New_York @@ -174,15 +179,20 @@ fn get_env_tz(db: &TimeZoneDatabase) -> Result, Error> { // support it anyway. let Some(tzenv) = std::env::var_os("TZ") else { return Ok(None) }; + // It is commonly agreed (across GNU and BSD tooling at least), but + // not standard, that setting an empty `TZ=` is indistinguishable from + // `TZ=UTC`. if tzenv.is_empty() { - // It is commonly agreed (across GNU and BSD tooling at least), - // but not standard, that setting an empty `TZ=` is indistinguishable - // from `TZ=UTC`. + debug!( + "TZ environment variable set to empty value, \ + assuming TZ=UTC in order to conform to \ + widespread convention among Unix tooling", + ); return Ok(Some(TimeZone::UTC)); } let tz_name_or_path = match PosixTzEnv::parse_os_str(&tzenv) { Err(_err) => { - trace!( + debug!( "failed to parse {tzenv:?} as POSIX TZ rule \ (attempting to treat it as an IANA time zone): {_err}", ); @@ -191,8 +201,7 @@ fn get_env_tz(db: &TimeZoneDatabase) -> Result, Error> { .ok_or_else(|| { err!( "failed to parse {tzenv:?} as a POSIX TZ transition \ - string, or as valid UTF-8 \ - (therefore ignoring TZ environment variable)", + string, or as valid UTF-8", ) })? .to_string() @@ -221,19 +230,28 @@ fn get_env_tz(db: &TimeZoneDatabase) -> Result, Error> { let Some(rpos) = tz_name_or_path.rfind(needle) else { // No zoneinfo means this is probably a IANA Time Zone name. But... // it could just be a file path. - trace!( + debug!( "could not find {needle:?} in TZ={tz_name_or_path:?}, \ therefore attempting lookup in {db:?}", ); return match db.get(&tz_name_or_path) { Ok(tz) => Ok(Some(tz)), Err(_err) => { - trace!( + debug!( "using TZ={tz_name_or_path:?} as time zone name failed, \ could not find time zone in zoneinfo database {db:?} \ - (continuing to try and use {tz_name_or_path:?})", + (continuing to try and read `{tz_name_or_path}` as \ + a TZif file)", ); - Ok(sys::read(db, &tz_name_or_path)) + sys::read(db, &tz_name_or_path) + .ok_or_else(|| { + err!( + "failed to read TZ={tz_name_or_path:?} \ + as a TZif file after attempting a tzdb \ + lookup for `{tz_name_or_path}`", + ) + }) + .map(Some) } }; }; @@ -241,14 +259,14 @@ fn get_env_tz(db: &TimeZoneDatabase) -> Result, Error> { // from what we now believe is a file path by taking everything after // `zoneinfo/`. Once we have that, we try to look it up in our tzdb. let name = &tz_name_or_path[rpos + needle.len()..]; - trace!( + debug!( "extracted {name:?} from TZ={tz_name_or_path:?} \ and assuming it is an IANA time zone name", ); match db.get(&name) { Ok(tz) => return Ok(Some(tz)), Err(_err) => { - trace!( + debug!( "using {name:?} from TZ={tz_name_or_path:?}, \ could not find time zone in zoneinfo database {db:?} \ (continuing to try and use {tz_name_or_path:?})", @@ -260,7 +278,15 @@ fn get_env_tz(db: &TimeZoneDatabase) -> Result, Error> { // The only thing left for us to do is treat the value as a file path // and read the data as TZif. This will give us time zone data if it works, // but without a name. - Ok(sys::read(db, &tz_name_or_path)) + sys::read(db, &tz_name_or_path) + .ok_or_else(|| { + err!( + "failed to read TZ={tz_name_or_path:?} \ + as a TZif file after attempting a tzdb \ + lookup for `{name}`", + ) + }) + .map(Some) } /// Returns the given file path as TZif data without a time zone name. diff --git a/src/tz/timezone.rs b/src/tz/timezone.rs index 44cbd9f..3548809 100644 --- a/src/tz/timezone.rs +++ b/src/tz/timezone.rs @@ -279,8 +279,8 @@ impl TimeZone { /// An unknown time zone _behaves_ like [`TimeZone::UTC`], but will /// print as `Etc/Unknown` when converting a `Zoned` to a string. /// - /// If you would instead like to fall back to UTC instead - /// of the special "unknown" time zone, then you can do + /// If you would like to fall back to UTC instead of + /// the special "unknown" time zone, then you can do /// `TimeZone::try_system().unwrap_or(TimeZone::UTC)`. /// /// # Platform behavior @@ -300,6 +300,9 @@ impl TimeZone { /// * `TZ=EST5EDT,M3.2.0,M11.1.0` for setting a time zone via a daylight /// saving time transition rule. /// + /// When `TZ` is set to an invalid value, Jiff uses the fallback behavior + /// described above. + /// /// Otherwise, when `TZ` isn't set, then: /// /// On Unix non-Android systems, this inspects `/etc/localtime`. If it's @@ -365,6 +368,9 @@ impl TimeZone { /// * `TZ=EST5EDT,M3.2.0,M11.1.0` for setting a time zone via a daylight /// saving time transition rule. /// + /// When `TZ` is set to an invalid value, then this routine returns an + /// error. + /// /// Otherwise, when `TZ` isn't set, then: /// /// On Unix systems, this inspects `/etc/localtime`. If it's a symbolic diff --git a/testprograms/invalid-tz-environment-variable/Cargo.toml b/testprograms/invalid-tz-environment-variable/Cargo.toml new file mode 100644 index 0000000..f23f9d8 --- /dev/null +++ b/testprograms/invalid-tz-environment-variable/Cargo.toml @@ -0,0 +1,18 @@ +[package] +publish = false +name = "invalid-tz-environment-variable" +version = "0.1.0" +edition = "2021" + +# Test programs in Jiff are explicitly isolated from the workspace to avoid +# dev-dependencies accumulating. (Not all test programs have big dependency +# trees, but we still exclude them from the workspace as a general rule.) +[workspace] + +[dependencies] +env_logger = "0.11.0" +jiff = { path = "../..", features = ["logging"] } + +[[bin]] +name = "invalid-tz-environment-variable" +path = "main.rs" diff --git a/testprograms/invalid-tz-environment-variable/README.md b/testprograms/invalid-tz-environment-variable/README.md new file mode 100644 index 0000000..280d177 --- /dev/null +++ b/testprograms/invalid-tz-environment-variable/README.md @@ -0,0 +1,8 @@ +This tests that Jiff falls back to the special `Etc/Unknown` time zone when the +`TZ` environment variable is set to a non-empty and invalid value. This also +checks that a set but empty `TZ` environment variable is indistinguishable from +`TZ=UTC` (which follows existing convention for GNU and BSD implementations of +POSIX `date`). + +See [#364](https://github.com/BurntSushi/jiff/issues/364) for discussion about +this behavior. diff --git a/testprograms/invalid-tz-environment-variable/main.rs b/testprograms/invalid-tz-environment-variable/main.rs new file mode 100644 index 0000000..75544d2 --- /dev/null +++ b/testprograms/invalid-tz-environment-variable/main.rs @@ -0,0 +1,33 @@ +fn main() { + env_logger::init(); + + // SAFETY: This is a single threaded program. + unsafe { + std::env::set_var("TZ", "WAT5HUH"); + } + assert_eq!( + jiff::tz::TimeZone::try_system().unwrap_err().to_string(), + "TZ environment variable set, but failed to read value: \ + failed to read TZ=\"WAT5HUH\" as a TZif file \ + after attempting a tzdb lookup for `WAT5HUH`", + ); + + // SAFETY: This is a single threaded program. + unsafe { + std::env::set_var("TZ", "/usr/share/zoneinfo/WAT5HUH"); + } + assert_eq!( + jiff::tz::TimeZone::try_system().unwrap_err().to_string(), + "TZ environment variable set, but failed to read value: \ + failed to read TZ=\"/usr/share/zoneinfo/WAT5HUH\" as a TZif file \ + after attempting a tzdb lookup for `WAT5HUH`", + ); + + unsafe { + std::env::set_var("TZ", ""); + } + assert_eq!( + jiff::tz::TimeZone::try_system().unwrap(), + jiff::tz::TimeZone::UTC, + ); +}