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

@@ -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

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).

examples/uptime/main.rs

@@ -53,7 +53,7 @@ impl UptimeDuration {
     /// This assumes the output of `uptime` is on stdin.
     fn find() -> anyhow::Result<UptimeDuration> {
         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}'"))?;

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

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<TimeZone, Error> {
             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<TimeZone, Error> {
 /// 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<Option<TimeZone>, 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<Option<TimeZone>, 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<Option<TimeZone>, 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<Option<TimeZone>, 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<Option<TimeZone>, 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<Option<TimeZone>, 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.

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

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"

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.

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,
+    );
+}