From 3abd28ac7b5173ad55b3224dd83b10514acdb177 Mon Sep 17 00:00:00 2001 From: Karl Williamson Date: Mon, 1 Jul 2024 06:31:30 -0600 Subject: [PATCH] Fix POSIX::strftime() This fixes GH #22351 The new sv_strftime_ints() API function acts consistently with regards to daylight savings time, with POSIX-mandated behavior, which takes the current locale into account. POSIX::strftime() is problematic in regard to this. This commit adds a backwards compatibility mode to preserve its behavior that inadvertently was changed by 86a9c18b6fab1949a26de790418b8b897a71e4ac. These functions are intended to wrap libc strftime(), including normalizing the input values. For example, if you pass 63 seconds as a value, they will typically change that to be 3 seconds into the next minute up. In C, the mktime() function is typically called first to do that normalization. (I don't know what happens if plain strftime() is called with unnormalized values.). mktime() looks at the current locale, determines if daylight savings time is in effect, and adjusts accordingly. Perl calls its own mktime-equivalent for you, eliminating the need for explicitly calling something that would need to always be called anyway, hence saving an extra step. mini_mktime() is the Perl mktime-equivalent. It is unaffected by locales, and does not consider the possibility of there being daylight savings time. The problem is that libc strftime() is affected by locale, and does consider dst. Perl uses these two functions together, and this inconsistency between them is bound to cause problems. The 'isdst' parameter to POSIX::strftime() is used to indicate if daylight savings is in effect for the time and date given by the other parameters. If perl used mktime() to normalize those values, it would calculate this for itself, using the passed-in value only as a hint. But since it uses mini_mktime(), it has to take that value as-is. Thus, daylight savings alone, out of all the input values, is not normalized. This is contrary to the documentation, and I didn't know this when I wrote the blamed commit. It turns out that typical uses of this function, like POSIX::strftime('%s', localtime) POSIX::strftime('%s', gmtime) cause the correct 'isdst' to be passed. But this is a defect in the interface that can bite you; changing it would cause cpan breakage. I wrote the API function sv_strftime_ints() to not have these issues. And, to workaround a defect in the POSIX definition of mktime(). It turns out you can't tell it you don't want to adjust for dayight time, except by changing the locale to one that doesn't have dst, such as the "C" locale. But this isn't a Perl-level function. --- ext/POSIX/POSIX.xs | 8 +++++--- ext/POSIX/lib/POSIX.pod | 17 ++++++++++------- ext/POSIX/t/time.t | 11 ++++++++++- locale.c | 35 +++++++++++++++++++++++++++++------ 4 files changed, 54 insertions(+), 17 deletions(-) diff --git a/ext/POSIX/POSIX.xs b/ext/POSIX/POSIX.xs index b79e2f80ab7b..2a8feae40487 100644 --- a/ext/POSIX/POSIX.xs +++ b/ext/POSIX/POSIX.xs @@ -3588,7 +3588,7 @@ difftime(time1, time2) # sv_setpv(TARG, ...) could be used rather than # ST(0) = sv_2mortal(newSVpv(...)) void -strftime(fmt, sec, min, hour, mday, mon, year, wday = -1, yday = -1, isdst = -1) +strftime(fmt, sec, min, hour, mday, mon, year, wday = -1, yday = -1, isdst = 0) SV * fmt int sec int min @@ -3603,9 +3603,11 @@ strftime(fmt, sec, min, hour, mday, mon, year, wday = -1, yday = -1, isdst = -1) { PERL_UNUSED_ARG(wday); PERL_UNUSED_ARG(yday); - PERL_UNUSED_ARG(isdst); - SV *sv = sv_strftime_ints(fmt, sec, min, hour, mday, mon, year, 0); + /* -isdst triggers backwards compatibility mode for non-zero + * 'isdst' */ + SV *sv = sv_strftime_ints(fmt, sec, min, hour, mday, mon, year, + -abs(isdst)); if (sv) { sv = sv_2mortal(sv); } diff --git a/ext/POSIX/lib/POSIX.pod b/ext/POSIX/lib/POSIX.pod index d72fc1faa9da..637c09200a23 100644 --- a/ext/POSIX/lib/POSIX.pod +++ b/ext/POSIX/lib/POSIX.pod @@ -1872,7 +1872,7 @@ Returns the string. Synopsis: strftime(fmt, sec, min, hour, mday, mon, year, - wday = -1, yday = -1, isdst = -1) + wday = -1, yday = -1, isdst = 0) The month (C) begins at zero, I, January is 0, not 1. The @@ -1880,7 +1880,11 @@ year (C) is given in years since 1900, I, the year 1995 is 95; the year 2001 is 101. Consult your system's C manpage for details about these and the other arguments. -The C, C, and C parameters are all ignored. +The C and C parameters are both ignored. Their values are +always determinable from the other parameters. + +C should be C<1> or C<0>, depending on whether or not daylight +savings time is in effect for the given time or not. If you want your code to be portable, your format (C) argument should use only the conversion specifiers defined by the ANSI C @@ -1895,11 +1899,10 @@ The C specifier is notoriously unportable since the names of timezones are non-standard. Sticking to the numeric specifiers is the safest route. -The given arguments are made consistent as though by calling -C before calling your system's C function, -except that the C value is not affected, so that the returned -value will always be as if the locale doesn't have daylight savings -time. +The arguments, except for C, are made consistent as though by +calling C before calling your system's C function. +To get correct results, you must set C to be the proper value. +When omitted, the function assumes daylight savings is not in effect. The string for Tuesday, December 12, 1995 in the C locale. diff --git a/ext/POSIX/t/time.t b/ext/POSIX/t/time.t index 44cba60654ce..faedc675d4de 100644 --- a/ext/POSIX/t/time.t +++ b/ext/POSIX/t/time.t @@ -9,7 +9,7 @@ use strict; use Config; use POSIX; -use Test::More tests => 26; +use Test::More tests => 27; # For the first go to UTC to avoid DST issues around the world when testing. SUS3 says that # null should get you UTC, but some environments want the explicit names. @@ -205,3 +205,12 @@ SKIP: { is(mktime(CORE::localtime($time)), $time, "mktime()"); is(mktime(POSIX::localtime($time)), $time, "mktime()"); } + +SKIP: { + skip "'%s' not implemented in strftime", 1 if $^O eq "VMS" + || $^O eq "MSWin32"; + # Somewhat arbitrarily, put in 60 seconds of slack; if this fails, it + # will likely be off by 1 hour + ok(abs(POSIX::strftime('%s', localtime) - time) < 60, + 'GH #22351; pr: GH #22369'); +} diff --git a/locale.c b/locale.c index 6a6dfc38ecab..edd5f81fd0cc 100644 --- a/locale.c +++ b/locale.c @@ -8095,11 +8095,27 @@ The caller assumes ownership of the returned SV with a reference count of 1. C takes a bunch of integer parameters that together completely define a given time. It calculates the S> to pass to -libc strftime(), and calls that function. Setting C to 0 causes the -date to be calculated as if there is no daylight savings time in effect; any -other value causes the possibility of daylight savings time to be considered. -A positive value is a hint to the function that daylight savings is likely to -be in effect for the passed-in time. +libc strftime(), and calls that function. + +The value of C is used as follows: + +=over + +=item 0 + +No daylight savings time is in effect + +=item E0 + +Check if daylight savings time is in effect, and adjust the results +accordingly. + +=item E0 + +This value is reserved for internal use by the L module for backwards +compatibility purposes. + +=back The caller assumes ownership of the returned SV with a reference count of 1. @@ -8166,8 +8182,15 @@ Perl_sv_strftime_ints(pTHX_ SV * fmt, int sec, int min, int hour, const char * locale = "C"; #endif + /* A negative 'isdst' triggers backwards compatibility mode for + * POSIX::strftime(), in which 0 is always passed to ints_to_tm() so that + * the possibility of daylight savings time is never considered, But, a 1 + * is eventually passed to libc strftime() so that it returns the results + * it always has for a non-zero 'isdst'. See GH #22351 */ struct tm mytm; - ints_to_tm(&mytm, locale, sec, min, hour, mday, mon, year, isdst); + ints_to_tm(&mytm, locale, sec, min, hour, mday, mon, year, + MAX(0, isdst)); + mytm.tm_isdst = MIN(1, abs(isdst)); return sv_strftime_common(fmt, locale, &mytm); }