Time 0.2.23 fails to determine local offset

[puetzp]

Hey there,

after upgrading to v0.2.23 I am unable to retrieve the correct local offset from my system.

println!("{}", UtcOffset::current_local_offset());

returns "+0", while

println!("{}", UtcOffset::try_current_local_offset());

returns a IndeterminateOffsetError.

After downgrading to 0.2.22, both functions correctly return "+1" (CET).

date '+%z'

also returns "+0100", as expected.

Rust version: rustc 1.47.0 (18bf6b4f0 2020-10-07)

[jhpratt]

See #293 and the associated security advisory. I've also filed an advisory with RUSTSEC, and this has been assigned a CVE. Not sure what else I could do to make it have more visibility.

Obtaining the systems offset was not and is never guaranteed. It is always best effort.

[quininer]

maybe we should provide a feature gate?

[jhpratt]

Given that it can outright crash the program with no possible way to recover, I think even a feature gate is a bad idea. That would encourage libraries to use it, and libraries of course have no knowledge of what else their users may be writing.

[dekellum]

When you backported 01513cb in f153a1c (diff) (#292) and released it as 0.2.23, you "fixed" #293 by essentially disabling the feature of local TZ detection, correct?

I am surprised by this approach. Should any use of getaddrinfo (see rust-lang/rust#27970) also be removed from the rust std library, in your view? IMO, no. Any consumer of libc/musl/* in any language has the same issue. If you ask the libc maintainers to "fix" this, they'll tell you that the safety constraints are well documented in the man pages, and if you mutate the env in a threaded program, you are responsible for the unsafety. Said another way, the workaround for this footgun is "don't do that."

Shouldn't this (#296) be re-opened since removing a feature (per above) isn't PATCH release compatible?

Or shouldn't this be re-opened, re-titled or replaced with a new open issue because you at least want to re-introduce the local TZ feature in a release like 0.3.0?

Then, if a safe (e.g. retaining the "fix" for #293) implementation of local TZ detection is found, would you backport that to a new 0.2.z release, fixing this removed-feature regression?

Assuming the safe implementation is otherwise not risky (likely to introduce other bugs, for example getting the local timezone wrong vs localtime_r), this would seem to be the right thing to do. Note, you had a similar regression-fix approach in the v0.2.6 release.

[dekellum]

Also, regarding the main branch for 0.3.0, would it be reasonable to re-introduce the feature by marking the functions as unsafe and documenting the safety requirements (don't concurrently setenv)?

[jhpratt]

As I mentioned above, obtaining the local offset was never guaranteed to begin with. By always returning an error value, that is still perfectly valid behavior. What is done is an attempt to obtain the local offset. In this case it is unsound, so I decided the "attempt" does not make sense.

Yes, a solution would be backported to time 0.2.

I don't think that any use of getaddrinfo should be removed in stdlib. The solution, as mentioned there (note I am part of that discussion) is to expose the ability to lock the environment. I could re-expose this as unsafe, but I generally dislike anything unsafe when not strictly necessary. Marking it unsafe would also needlessly affect non-Unix targets. Either way, this would still likely encourage library authors to use the methods, despite them not being capable of knowing what else their users are doing.

With regard to 0.2.6, that was an outright bad decision. IIRC I ended up taking that version because of the breakage it caused (being questionably consistent with existing documentation).

[dekellum]

I haven't personally been bitten by this yet, but I will have to assume that from the perspective of the OP of this issue you have closed, that removing any attempt to obtain the local offset on *nix, is effectively a removed-feature regression in the 0.2.23 PATCH release. That's my perspective at least.

What are the odds that the OP is actually concurrently mutating the environment in the program in question?

[jhpratt]

The public API has not changed in any way; a value is currently being returned that could have always been returned. As such, this isn't a breaking change. The method is by its nature nondeterministic. Sure, some people may have done .unwrap(), assuming that the Ok value was always returned, but doing that was incorrect from day one.

Realistically? The odds are damn near zero. I'd personally be surprised if anyone has run into this (I presume @quininer specifically constructed the example when reviewing some code and didn't run across it naturally). But if I have to choose between returning an Err (avoiding unsoundness) and a potential segfault, the choice is clear to me.

[quininer]

Realistically? The odds are damn near zero.

No, we ran into this problem in production environment, but we used chrono instead of time.

[puetzp]

I can understand @jhpratt unwillingness to introduce an unsafe block into the code and thereby unnecessarily affect other platforms than *nix. I personally have been bitten by this issue in production, because, as surmised, I did not handle the error correctly.
I would also say however, that it took me by surprise that a patch release would introduce said behaviour for *nix targets (effectively disabling the possibility to obtain the local offset).

Perhaps an update of the documentation that marks the function in question as basically "non-functional" on *nix targets would prevent other users to run into this issue until a fix is found?

[jhpratt]

@quininer Interesting. That's quite surprising that you managed to run across this naturally, to be honest.

---

@puetzp A note in documentation is certainly feasible. Unfortunately docs.rs doesn't allow updating already-published docs, so it's not possible for v0.2 (unless there's another release for some reason).