[tz] Invalid tm handling in %s pattern in strftime

[enh]

On Mon, Jun 6, 2022 at 5:49 PM Paul Eggert <eggert at cs.ucla.edu> wrote:

> On 6/6/22 15:56, enh wrote:
>
> > it's your mktime(), so you tell me :-)
>
> tzdb mktime doesn't make any attempt to preserve errno on success. Even
> on a platform like bionic with primitives following a
> preserve-errno-on-success philosophy, tzdb functions can succeed after
> calling other functions that may fail and set errno. For example,
> tzset_unlocked can recover from a failed zoneinit (which sets errno) by
> calling zoneinit again.
>
> > in bionic we use RAII to make functions "errno clean" all
> > the time to avoid this kind of problem because it's too hard to reason
> > about...)
>
> If we wanted to make tzdb localtime.c and strftime.c "errno clean" it'd
> require some careful work. It'd be doable but I expect it's not a
> one-hour job (though perhaps you could prove me wrong :-).
>

i think worse than that, it's just too hard to reason about --- we'd bitrot
anyway.

it's really not obvious that there's a good fix (beyond "wait for 32-bit to
die") at all...

i don't think we can easily fix mktime64() --- i think we'd basically need
to fix it differently, so we built tzcode twice (once with the public ABI
32-bit time_t and again with the 64-bit time_t), which at the very least
would be quite a behavior change for any code using the 64-bit stuff. we'd
also have to change the const-ness of a few arguments, which could be a
breaking change. (i mean, it _is_ a breaking change; the question is really
"how many apps would break?".)

i don't think we can really go the errno route. you've convinced me of that.

i don't see how we can save the localtime_r() comparisons --- our
fundamental problem is that we don't have anything from mktime64() to
compare against.

i think the safest choice for bionic is probably just "disable the new
check and preserve the old behavior". given that you can only get into this
mess by passing a bad `struct tm` to strftime(), and given that most use
cases i'm aware of have literally just asked us to fill out that `struct
tm` from a `time_t`, so -1 is almost certain to be "legit -1" rather than
"error -1", i think i'm fine with that? especially since we haven't
seen/heard any complaints about the existing behavior (we haven't yet
shipped
https://github.com/eggert/tz/commit/8b238ec54c09556eb2aa405c1741eedfd12c4a87,
despite it being from 2020 --- it's mingaleev's work trying to get back in
sync that uncovered this).

it sounds from the checkin comment like there wasn't a motivating example
for this behavior change, but let us know if you can think of an actual
downside to not taking this change...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mm.icann.org/pipermail/tz/attachments/20220606/efff0cc5/attachment.html>