diff mbox series

[4/4] time: Implement the glibc.time.mktime_dst_adjustment tunable

Message ID 4b54ff2cd98520b5e1bf31da5b3ec0a2713ad3b3.1727784647.git.fweimer@redhat.com
State New
Headers show
Series mktime tm_isdst compatibility improvements | expand

Commit Message

Florian Weimer Oct. 1, 2024, 12:15 p.m. UTC
It helps with upgrades from older glibc versions (prior to 2.29)
which performed DST adjustments in fewer cases.
---
 elf/dl-tunables.list           |   9 ++
 elf/libc_early_init.c          |   3 +
 manual/time.texi               |   3 +-
 manual/tunables.texi           |  12 +++
 time/Makefile                  |   7 +-
 time/mktime.c                  |   8 +-
 time/time-tunables.c           |  29 +++++++
 time/tst-mktime-dst-adjust-0.c | 154 +++++++++++++++++++++++++++++++++
 time/tst-mktime-dst-adjust-1.c |   2 +
 time/tzset.h                   |   6 ++
 10 files changed, 228 insertions(+), 5 deletions(-)
 create mode 100644 time/time-tunables.c
 create mode 100644 time/tst-mktime-dst-adjust-0.c
 create mode 100644 time/tst-mktime-dst-adjust-1.c

Comments

Paul Eggert Oct. 1, 2024, 5:41 p.m. UTC | #1
Why add a tunable for this obscure undocumented implementation detail of 
mktime? The proposed tunable's documentation is so unclear that I didn't 
know what it actually meant; I had to read the source code to find out. 
It's not clear why a sysadmin would want to fiddle with such a tunable.

I suggest that we omit this patch. I.e., just make the change with no 
tunable, as the change is an obvious improvement (in an admittedly 
obscure area). But if we do need the patch for some reason, its doc 
needs improving.
Florian Weimer Oct. 1, 2024, 10:43 p.m. UTC | #2
* Paul Eggert:

> Why add a tunable for this obscure undocumented implementation detail
> of mktime? The proposed tunable's documentation is so unclear that I
> didn't know what it actually meant; I had to read the source code to
> find out. It's not clear why a sysadmin would want to fiddle with such
> a tunable.

It disables the DST override heuristic that was added in glibc 2.29.
The heuristic was added without documenting it.  I don't know how it is
supposed to work, and whether the DST-with-UTC thing is a bug in the
code, or if it works as intended.  It's surprising that the heuristic
performs the DST adjustment for IST, but not for Indian Standard Time
(see the test case).

> I suggest that we omit this patch. I.e., just make the change with no
> tunable, as the change is an obvious improvement (in an admittedly
> obscure area). But if we do need the patch for some reason, its doc
> needs improving.

What part is the improvement?  Not doing the adjustment for UTC?  Or
removing the heuristic altogether?  The tunable does the latter.  The
UTC part is already unconditional.

Thanks,
Florian
diff mbox series

Patch

diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
index 40ac5b3776..af3eb4a37c 100644
--- a/elf/dl-tunables.list
+++ b/elf/dl-tunables.list
@@ -84,6 +84,15 @@  glibc {
     }
   }
 
+  time {
+    mktime_dst_adjustment {
+      type: INT_32
+      minval: 0
+      maxval: 1
+      default: 1
+    }
+  }
+
   elision {
     enable {
       type: INT_32
diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
index 575b837f8f..f99f12b6c8 100644
--- a/elf/libc_early_init.c
+++ b/elf/libc_early_init.c
@@ -23,6 +23,7 @@ 
 #include <lowlevellock.h>
 #include <pthread_early_init.h>
 #include <sys/single_threaded.h>
+#include <time/tzset.h>
 
 #ifdef SHARED
 _Bool __libc_initial;
@@ -46,4 +47,6 @@  __libc_early_init (_Bool initial)
 #if ENABLE_ELISION_SUPPORT
   __lll_elision_init ();
 #endif
+
+  __time_tunable_init ();
 }
diff --git a/manual/time.texi b/manual/time.texi
index c72febc7c6..0a158e742f 100644
--- a/manual/time.texi
+++ b/manual/time.texi
@@ -1324,7 +1324,8 @@  positive.  This forced adjustment only happens if time zone data
 indicates that there is at least one daylight saving transition (at any
 time).  For zones such as @samp{UTC} which never use daylight saving
 time, the adjustment does not happen even if @code{tm_isdst} is
-positive.
+positive.  The @code{glibc.time.mktime_dst_adjustment} tunable
+can be used to disable the adjustment heuristic.  @xref{Time Tunables}.
 
 The @code{mktime} function ignores the specified contents of the
 @code{tm_wday}, @code{tm_yday}, @code{tm_gmtoff}, and @code{tm_zone}
diff --git a/manual/tunables.texi b/manual/tunables.texi
index 0b1b2898c0..e9e6759bf6 100644
--- a/manual/tunables.texi
+++ b/manual/tunables.texi
@@ -76,6 +76,7 @@  glibc.malloc.check: 0 (min: 0, max: 3)
 @menu
 * Tunable names::  The structure of a tunable name
 * Memory Allocation Tunables::  Tunables in the memory allocation subsystem
+* Time Tunables::  Tunables related to time and time zone processing.
 * Dynamic Linking Tunables:: Tunables in the dynamic linking subsystem
 * Elision Tunables::  Tunables in elision subsystem
 * POSIX Thread Tunables:: Tunables in the POSIX thread subsystem
@@ -294,6 +295,17 @@  supported ones.  If provided value is invalid, @code{MAP_HUGETLB} will not
 be used.
 @end deftp
 
+@node Time Tunables
+@section Time Tunables
+@cindex time tunables
+
+@deftp Tunable glibc.time.mktime_dst_adjustment
+If this tunable is @code{1}, the @code{mktime} function uses a heuristic
+if the the @code{tm_isdst} input value disagrees with available time
+zone data.  If the tunable is @code{0}, this heuristic is disabled.
+@xref{Broken-down Time}.
+@end deftp
+
 @node Dynamic Linking Tunables
 @section Dynamic Linking Tunables
 @cindex dynamic linking tunables
diff --git a/time/Makefile b/time/Makefile
index 0cfc4c9d51..ef7922cf35 100644
--- a/time/Makefile
+++ b/time/Makefile
@@ -30,7 +30,7 @@  headers := time.h sys/time.h sys/timeb.h bits/time.h			\
 	   bits/types/time_t.h bits/types/struct_timeb.h
 
 routines := offtime asctime clock ctime ctime_r difftime \
-	    gmtime localtime mktime time		 \
+	    gmtime localtime mktime time time-tunables	 \
 	    gettimeofday settimeofday settimezone	 \
 	    adjtime tzset tzfile getitimer setitimer	 \
 	    stime dysize timegm ftime			 \
@@ -65,6 +65,8 @@  tests := \
   tst-itimer \
   tst-mktime \
   tst-mktime-dst-adjust \
+  tst-mktime-dst-adjust-0 \
+  tst-mktime-dst-adjust-1 \
   tst-mktime2 \
   tst-mktime3 \
   tst-mktime4 \
@@ -147,3 +149,6 @@  CPPFLAGS-tst-tzname.c += -DTZDEFRULES='"$(posixrules-file)"'
 bug-getdate1-ARGS = ${objpfx}bug-getdate1-fmt
 
 tst-tzfile-fault-ENV = GLIBC_TUNABLES=glibc.rtld.enable_secure=1
+
+tst-mktime-dst-adjust-0-ENV = GLIBC_TUNABLES=glibc.time.mktime_dst_adjustment=0
+tst-mktime-dst-adjust-1-ENV = GLIBC_TUNABLES=glibc.time.mktime_dst_adjustment=1
diff --git a/time/mktime.c b/time/mktime.c
index ad371ba57f..0c9ef13ec3 100644
--- a/time/mktime.c
+++ b/time/mktime.c
@@ -435,9 +435,11 @@  __mktime_internal (struct tm *tp, bool local, mktime_offset_t *offset)
      value, if any.  */
   if (isdst_differ (isdst, tm.tm_isdst)
 #if _LIBC
-      /* Only perform the adjustment if the time zone ever uses DST.
-         This skips the adjustment for UTC, for example.  */
-      && __daylight
+      /* The glibc.time.mktime_dst_adjustment tunable disable all such
+         adjustments.  Otherwise, only perform the adjustment if the
+         time zone ever uses DST.  This skips the adjustment for UTC,
+         for example.  */
+      && __mktime_dst_adjustment && __daylight
 #endif
       )
     {
diff --git a/time/time-tunables.c b/time/time-tunables.c
new file mode 100644
index 0000000000..78d28f38e0
--- /dev/null
+++ b/time/time-tunables.c
@@ -0,0 +1,29 @@ 
+/* Tunables for time-related functions.
+   Copyright 2024 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Paul Eggert <eggert@cs.ucla.edu>.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#define TUNABLE_NAMESPACE time
+#include <elf/dl-tunables.h>
+
+bool __mktime_dst_adjustment;
+
+void
+__time_tunable_init (void)
+{
+  __mktime_dst_adjustment = TUNABLE_GET (mktime_dst_adjustment, int32_t, NULL);
+}
diff --git a/time/tst-mktime-dst-adjust-0.c b/time/tst-mktime-dst-adjust-0.c
new file mode 100644
index 0000000000..386e69429f
--- /dev/null
+++ b/time/tst-mktime-dst-adjust-0.c
@@ -0,0 +1,154 @@ 
+/* Test lack of mktime DST adjustment with the adjustment heuristic disabled.
+   Copyright (C) 2024 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <time.h>
+#include <stdlib.h>
+#include <support/check.h>
+
+static int
+do_test (void)
+{
+  TEST_COMPARE (setenv ("TZ", "UTC", 1), 0);
+
+  {
+    struct tm t =
+      {
+        .tm_year = 124,
+        .tm_mon = 9,
+        .tm_mday = 1,
+        .tm_hour = 9,
+        .tm_min = 20,
+        .tm_sec = 53,
+        .tm_isdst = 1,            /* Not actually true.  */
+      };
+    TEST_COMPARE (mktime (&t), 1727774453);
+  }
+
+  /* IST used DST at one point, but no longer does.  */
+  {
+    char *path = realpath ("../timezone/testdata/IST", NULL);
+    TEST_VERIFY (path != NULL);
+    TEST_COMPARE (setenv ("TZ", path, 1), 0);
+    free (path);
+  }
+
+  {
+    struct tm t =
+      {
+        .tm_year = 124,
+        .tm_mon = 9,
+        .tm_mday = 1,
+        .tm_hour = 9,
+        .tm_min = 20,
+        .tm_sec = 53,
+        .tm_isdst = 0,          /* Correct value.  */
+      };
+    TEST_COMPARE (mktime (&t), 1727774453 - (int) (5.5 * 3600));
+    TEST_COMPARE (t.tm_gmtoff, (int) (5.5 * 3600));
+    TEST_COMPARE (t.tm_isdst, 0);
+  }
+
+  /* No adjustment for historic incorrect value.  */
+  {
+    struct tm t =
+      {
+        .tm_year = 124,
+        .tm_mon = 9,
+        .tm_mday = 1,
+        .tm_hour = 9,
+        .tm_min = 20,
+        .tm_sec = 53,
+        .tm_isdst = 1,          /* Incorrect value.  */
+      };
+    TEST_COMPARE (mktime (&t), 1727774453 - (int) (5.5 * 3600));
+    TEST_COMPARE (t.tm_gmtoff, (int) (5.5 * 3600));
+    TEST_COMPARE (t.tm_isdst, 0);
+  }
+
+  /* No adjustment for correct DST.  */
+  {
+    struct tm t =
+      {
+        .tm_year = 42,
+        .tm_mon = 9,
+        .tm_mday = 1,
+        .tm_hour = 9,
+        .tm_min = 20,
+        .tm_sec = 53,
+        .tm_isdst = 1,          /* Correct value, DST was in effect.  */
+      };
+    TEST_COMPARE (mktime (&t), -860015347);
+    TEST_COMPARE (t.tm_gmtoff, (int) (6.5 * 3600));
+    TEST_COMPARE (t.tm_isdst, 1);
+  }
+
+  /* No adjustment for mismatch: DST incorrectly claimed not in effect.  */
+  {
+    struct tm t =
+      {
+        .tm_year = 42,
+        .tm_mon = 9,
+        .tm_mday = 1,
+        .tm_hour = 9,
+        .tm_min = 20,
+        .tm_sec = 53,
+        .tm_isdst = 0,          /* Incorrect value.  */
+      };
+    TEST_COMPARE (mktime (&t), -860015347);
+    TEST_COMPARE (t.tm_gmtoff, (int) (6.5 * 3600));
+    TEST_COMPARE (t.tm_isdst, 1);
+  }
+
+  /* Test using correct standard time.  */
+  {
+    struct tm t =
+      {
+        .tm_year = 42,
+        .tm_mon = 7,
+        .tm_mday = 1,
+        .tm_hour = 9,
+        .tm_min = 20,
+        .tm_sec = 53,
+        .tm_isdst = 0,          /* Correct value, standard time in effect.  */
+      };
+    TEST_COMPARE (mktime (&t), -865282147);
+    TEST_COMPARE (t.tm_gmtoff, (int) (5.5 * 3600));
+    TEST_COMPARE (t.tm_isdst, 0);
+  }
+
+  /* No adjustment: Test using standard time with mismatch.  */
+  {
+    struct tm t =
+      {
+        .tm_year = 42,
+        .tm_mon = 7,
+        .tm_mday = 1,
+        .tm_hour = 9,
+        .tm_min = 20,
+        .tm_sec = 53,
+        .tm_isdst = 1,          /* Incorrect value.  */
+      };
+    TEST_COMPARE (mktime (&t), -865282147);
+    TEST_COMPARE (t.tm_gmtoff, (int) (5.5 * 3600));
+    TEST_COMPARE (t.tm_isdst, 0);
+  }
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/time/tst-mktime-dst-adjust-1.c b/time/tst-mktime-dst-adjust-1.c
new file mode 100644
index 0000000000..137d1e7411
--- /dev/null
+++ b/time/tst-mktime-dst-adjust-1.c
@@ -0,0 +1,2 @@ 
+/* Test with GLIBC_TUNABLES=glibc.time.mktime_dst_adjust=1 (default).  */
+#include "tst-mktime-dst-adjust.c"
diff --git a/time/tzset.h b/time/tzset.h
index 254540aa05..3999c71de6 100644
--- a/time/tzset.h
+++ b/time/tzset.h
@@ -31,4 +31,10 @@  extern void __tzset_unlocked (void) attribute_hidden;
 extern struct tm *__tz_convert (__time64_t timer, int use_localtime,
                                 struct tm *tp) attribute_hidden;
 
+/* True if mktime uses the DST adjustment heuristic based on tm_isdst.  */
+extern bool __mktime_dst_adjustment attribute_hidden;
+
+/* Initializes tunables.  Called from __libc_early_init.  */
+void __time_tunable_init (void) attribute_hidden;
+
 #endif /* _TZSET_H */