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

Message ID 4b54ff2cd98520b5e1bf31da5b3ec0a2713ad3b3.1727784647.git.fweimer@redhat.com (mailing list archive)
State Superseded
Headers
Series mktime tm_isdst compatibility improvements |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed
redhat-pt-bot/TryBot-still_applies warning Patch no longer applies to master

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
  
Paul Eggert Oct. 9, 2024, 7:49 a.m. UTC | #3
On 2024-10-01 15:43, Florian Weimer wrote:
> * Paul Eggert:
> 
>> Why add a tunable for this obscure undocumented implementation detail
>> of mktime?
> 
> 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.

Do you mean "in glibc 2.29", or "since glibc 2.29"? From your 
description, it sounds like you're referring to this glibc commit 
(2022-09-09):

https://github.com/bminor/glibc/commit/83859e1115269cf56d21669361d4ddbe2687831c

and this change appeared in glibc 2.37 (2023-02-01).

I've spent some time looking into what I think is the underlying problem 
here, and sort of have a handle on it. I'll try to spring some more time 
free to fix it. I've found several other bugs in the code while I was in 
the neighborhood, so will probably generate a long string of patches. 
(Still not seeing a need for a tunable, though; these are all bugs 
through and through.)
  
Florian Weimer Oct. 9, 2024, 8:07 a.m. UTC | #4
* Paul Eggert:

> On 2024-10-01 15:43, Florian Weimer wrote:
>> * Paul Eggert:
>> 
>>> Why add a tunable for this obscure undocumented implementation detail
>>> of mktime?
>> 
>> 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.
>
> Do you mean "in glibc 2.29", or "since glibc 2.29"? From your 
> description, it sounds like you're referring to this glibc commit 
> (2022-09-09):
>
> https://github.com/bminor/glibc/commit/83859e1115269cf56d21669361d4ddbe2687831c
>
> and this change appeared in glibc 2.37 (2023-02-01).
>
> I've spent some time looking into what I think is the underlying problem 
> here, and sort of have a handle on it. I'll try to spring some more time 
> free to fix it. I've found several other bugs in the code while I was in 
> the neighborhood, so will probably generate a long string of patches. 
> (Still not seeing a need for a tunable, though; these are all bugs 
> through and through.)

I assumed it was this commit:

commit 8e6fd2bdb21efe2cc1ae7571ff8fb2599db6a05a
Author: Paul Eggert <eggert@cs.ucla.edu>
Date:   Wed Sep 19 13:16:14 2018 -0700

    Merge mktime, timegm from upstream Gnulib

But you are right, it could have been one of the later backports.
  

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 */