Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix POSIX::strftime() #22369

Merged
merged 1 commit into from
Jul 2, 2024
Merged

Fix POSIX::strftime() #22369

merged 1 commit into from
Jul 2, 2024

Conversation

khwilliamson
Copy link
Contributor

I don't know what to do about testing this. The example furnished in the ticket uses the command line date with the %s format, which I kind of doubt is portable.

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 86a9c18.

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.

jkeenan added a commit to jkeenan/perl5 that referenced this pull request Jul 1, 2024
@jkeenan
Copy link
Contributor

jkeenan commented Jul 1, 2024

For the purpose of getting one unit test for this p.r. and at the same time getting a dev release out, I offer the following patch:

diff --git a/ext/POSIX/t/time.t b/ext/POSIX/t/time.t
index 44cba60654..d06cb739b8 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,18 @@ SKIP: {
     is(mktime(CORE::localtime($time)), $time, "mktime()");
     is(mktime(POSIX::localtime($time)), $time, "mktime()");
 }
+
+SKIP: { # XXX: Improve this test!
+    skip "System 'date' command not tested on all OSes yet", 1
+    unless ($^O eq 'linux' or $^O eq 'freebsd' or $^O eq 'openbsd');
+    my $xdate = `date '+%s'`;
+    chomp $xdate;
+    my $posix_strftime = POSIX::strftime('%s', localtime);
+    my $perl_time = time;
+    ok(
+        ($posix_strftime == $xdate) &&
+        ($posix_strftime == $perl_time),
+        'GH #22351; pr: GH #22369'
+    );
+}
+

Please review.


# 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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misplaced parentheses. You want abs(delta) < 60, not abs(delta < 60).

Apart from that, strftime "%s" returns seconds since 1970-01-01, but perldoc -f time says the time epoch is platform specific. In particular, I would expect this test to fail on Mac OS Classic (do we still support that?). No idea about VMS, DOS, or other "exotic" systems. Might need a $^O guard?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VMS: The time() function returns the value of time in seconds since 00:00:00 UTC, January 1, 1970.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

experiment on Win32 yields the same. Do we support DOS? We can add skips as necessary, But I think this won't overwhelm us with cpan fails

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we support Mac classic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have to skip Windows and VMS, as strftime() doesn't handle %s

This fixes GH Perl#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 86a9c18.

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.
@khwilliamson khwilliamson merged commit 4c99243 into Perl:blead Jul 2, 2024
29 checks passed
@khwilliamson khwilliamson deleted the 22351 branch July 2, 2024 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants