[tz] Minor (unimportant really) technical UB bug in strftime() ?

[Jonathan Leffler]

The code has:

tm = *t;
mkt = mktime(&tm);

This makes a copy of the `const struct tm` that is the argument to
strftime(), and passes that copy to mktime(), which reads a few fields and
sets most fields.  But since it is working on a copy of the argument to
strftime(), I don't see a problem.  Can you elaborate?

Is there a good reason not to write this?

struct tm tm = *t;
time_t mkt = mktime(&tm);

It's a stylistic question wholly separate from the const-ness issue.


On Tue, Nov 8, 2022 at 6:07 AM Robert Elz via tz <tz at iana.org> wrote:

> The code for the %s strftime conversion starts as:
> (this taken from tzcode2022f strftime.c - but I suspect
> that many versions are the same)
>
>                         case 's':
>                                 {
>                                         struct tm       tm;
>                                         char
> buf[INT_STRLEN_MAXIMUM(
>                                                                 time_t) +
> 1];
>                                         time_t          mkt;
>
>                                         tm = *t;
>                                         mkt = mktime(&tm);
>
> The "tm = *t;" line is the problem.   It (or more correctly, something
> to replace that) is needed, as *t is const, strftime() is not permitted
> to modify it, yet the struct tm * passed to mktime() is not const, as
> mktime is sometimes required to modify that struct before returning.
>
> However, mktime() refers only to the fields tm_year tm_mon tm_mday
> tm_hour tm_min tm_sec and tm_isdst of the struct passed to it.  The
> %s conversion is defined as producing (in a decimal string format) the
> result of mktime() applied to the struct tm passed to strftime.
> Hence an application which intends to use strftime(..., "%s", ...)
> need only initialise those 7 fields, the other 2 (currently) standard
> fields (tm_yday and tm_wday) are explicitly ignored by mktime() on
> input, so there is no need to assign those values, and any other
> fields that might exist in a particular implementation are out of
> scope for a portable (conforming) application to touch, so cannot
> be initialised.
>
> Since referring to an uninitialised variable is, I believe, technically
> undefined behaviour, doing a struct copy (copying all fields) can have
> that effect.   (An application could memset() the entire struct before
> providing the values it must add, but is not required to do that).
>
> It seems likely that the simple fix is to replace the struct assignment
> with 7 individual assignment statements
>
>         tm.tm_year = t->tm_year;
>         tm.tm_mon = t->tm_mon;
>         /* ... */
>
> On the other hand, hardware that balks at simply loading or copying,
> uninitialised values, without using them for any purpose is not exactly
> common (and half the world's code would probably break if an implementation
> that worked in such a manner was ever created) so I'd call this one of
> the less significant issues to worry about fixing.
>
> kre
>
>

-- 
Jonathan Leffler <jonathan.leffler at gmail.com>  #include <disclaimer.h>
Guardian of DBD::Informix - v2018.1031 - http://dbi.perl.org
"Blessed are we who can laugh at ourselves, for we shall never cease to be
amused."
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mm.icann.org/pipermail/tz/attachments/20221108/e5e3ddb7/attachment.html>