[08/59] Push tzset lock into callers of time functions

Message ID 20250105055750.1668721-9-eggert@cs.ucla.edu (mailing list archive)
State New
Headers
Series time: sync mktime from Gnulib |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Paul Eggert Jan. 5, 2025, 5:56 a.m. UTC
  This makes sure no other thread changes the time zone data while
mktime is running, which could confuse its search probes.
Likewise for timegm, though admittedly this is overkill unless leap
seconds are being used and the leap seconds table changes (!).

Although this means mktime and timegm will be more of a bottleneck
for other threads, mktime and timegm are called so rarely that
let's hope it's not a significant performance issue.

This patch also fixes a harmless data race on the cached offset
used by __mktime_internal.

Co-authored-by: Florian Weimer <fweimer@redhat.com>

* include/time.h: Move tzset-related declarations out of here ...
* time/tzset.h: ... and into this new file.  All .c files that
use these declarations changed to include the new file.
* time/gmtime.c (__gmtime64_r, __gmtime64):
* time/localtime.c (__localtime64_r, __localtime64):
* time/mktime.c (__mktime64):
* time/timegm.c (__timegm64):
Lock __tzset_lock while calling __tz_convert.
* time/gmtime.c (__timegm64):
Invoke __tzset_unlocked here; it is needed for leap seconds.
* time/mktime-internal.h (__libc_lock_lock, __libc_lock_unlock)
(__tzset_unlocked) [!_LIBC]: New no-op macros.
* time/mktime.c (convert_time, __tz_convert) [_LIBC]:
Remove; now done by glibc/time/tzset.c's __tz_convert.
(__mktime_internal) [!_LIBC]: Double the number of probes.
---
 include/time.h         | 19 -------------------
 time/gmtime.c          |  8 ++++++--
 time/localtime.c       |  8 ++++++--
 time/mktime-internal.h |  7 ++++++-
 time/mktime.c          | 23 +++++++++++++++--------
 time/strftime_l.c      |  1 +
 time/timegm.c          |  9 ++++++++-
 time/tzfile.c          |  1 +
 time/tzset.c           | 21 +++++++++++----------
 time/tzset.h           | 30 ++++++++++++++++++++++++++++++
 10 files changed, 84 insertions(+), 43 deletions(-)
 create mode 100644 time/tzset.h
  

Patch

diff --git a/include/time.h b/include/time.h
index 52bb22abd9..e9c083646a 100644
--- a/include/time.h
+++ b/include/time.h
@@ -49,20 +49,6 @@  extern const unsigned short int __mon_yday[2][13] attribute_hidden;
 /* Defined in localtime.c.  */
 extern struct tm _tmbuf attribute_hidden;
 
-/* Defined in tzset.c.  */
-extern char *__tzstring (const char *string) attribute_hidden;
-
-extern int __use_tzfile attribute_hidden;
-
-extern void __tzfile_read (const char *file) attribute_hidden;
-extern void __tzfile_compute (__time64_t timer, int use_localtime,
-			      long int *leap_correct, int *leap_hit,
-			      struct tm *tp) attribute_hidden;
-extern void __tzset_parse_tz (const char *tz) attribute_hidden;
-extern void __tz_compute (__time64_t timer, struct tm *tm, int use_localtime)
-  __THROW attribute_hidden;
-
-
 #if __TIMESIZE == 64
 # define __itimerspec64 itimerspec
 #else
@@ -271,11 +257,6 @@  extern int __offtime (__time64_t __timer,
 
 extern char *__asctime_r (const struct tm *__tp, char *__buf)
   attribute_hidden;
-extern void __tzset (void) attribute_hidden;
-
-/* Prototype for the internal function to get information based on TZ.  */
-extern struct tm *__tz_convert (__time64_t timer, int use_localtime,
-				struct tm *tp) attribute_hidden;
 
 extern int __nanosleep (const struct timespec *__requested_time,
 			struct timespec *__remaining);
diff --git a/time/gmtime.c b/time/gmtime.c
index ad6e8eb352..9d0af33401 100644
--- a/time/gmtime.c
+++ b/time/gmtime.c
@@ -17,13 +17,17 @@ 
    <https://www.gnu.org/licenses/>.  */
 
 #include <time.h>
+#include <tzset.h>
 
 /* Return the `struct tm' representation of *T in UTC,
    using *TP to store the result.  */
 struct tm *
 __gmtime64_r (const __time64_t *t, struct tm *tp)
 {
-  return __tz_convert (*t, 0, tp);
+  __libc_lock_lock (__tzset_lock);
+  struct tm *result = __tz_convert (*t, 0, tp);
+  __libc_lock_unlock (__tzset_lock);
+  return result;
 }
 
 /* Provide a 32-bit variant if needed.  */
@@ -48,7 +52,7 @@  weak_alias (__gmtime_r, gmtime_r)
 struct tm *
 __gmtime64 (const __time64_t *t)
 {
-  return __tz_convert (*t, 0, &_tmbuf);
+  return __gmtime64_r (t, &_tmbuf);
 }
 
 /* Provide a 32-bit variant if needed.  */
diff --git a/time/localtime.c b/time/localtime.c
index d1967ff11b..7f865a7053 100644
--- a/time/localtime.c
+++ b/time/localtime.c
@@ -17,6 +17,7 @@ 
    <https://www.gnu.org/licenses/>.  */
 
 #include <time.h>
+#include <tzset.h>
 
 /* C89 says that localtime and gmtime return the same pointer.
    Although C99 and later relax this to let localtime and gmtime
@@ -30,7 +31,10 @@  struct tm _tmbuf;
 struct tm *
 __localtime64_r (const __time64_t *t, struct tm *tp)
 {
-  return __tz_convert (*t, 1, tp);
+  __libc_lock_lock (__tzset_lock);
+  struct  tm *result = __tz_convert (*t, 1, tp);
+  __libc_lock_unlock (__tzset_lock);
+  return result;
 }
 
 /* Provide a 32-bit variant if needed.  */
@@ -54,7 +58,7 @@  weak_alias (__localtime_r, localtime_r)
 struct tm *
 __localtime64 (const __time64_t *t)
 {
-  return __tz_convert (*t, 1, &_tmbuf);
+  return __localtime64_r (t, &_tmbuf);
 }
 libc_hidden_def (__localtime64)
 
diff --git a/time/mktime-internal.h b/time/mktime-internal.h
index 1da98b4373..215be914c2 100644
--- a/time/mktime-internal.h
+++ b/time/mktime-internal.h
@@ -19,6 +19,9 @@ 
 
 #ifndef _LIBC
 # include <time.h>
+# define __libc_lock_lock(lock) ((void) 0)
+# define __libc_lock_unlock(lock) ((void) 0)
+# define __tzset_unlocked() tzset ()
 #endif
 
 /* mktime_offset_t is a signed type wide enough to hold a UTC offset
@@ -73,6 +76,8 @@  typedef int mktime_offset_t;
 /* Subroutine of mktime.  Return the time_t representation of TP and
    normalize TP, given that a struct tm * maps to a time_t.  If
    LOCAL, the mapping is performed by localtime_r, otherwise by gmtime_r.
-   Record next guess for localtime-gmtime offset in *OFFSET.  */
+   Record next guess for localtime-gmtime offset in *OFFSET.
+
+   If _LIBC, the caller must lock __tzset_lock.  */
 extern __time64_t __mktime_internal (struct tm *tp, bool local,
                                      mktime_offset_t *offset) attribute_hidden;
diff --git a/time/mktime.c b/time/mktime.c
index 7f7d33492b..3a951b2c8c 100644
--- a/time/mktime.c
+++ b/time/mktime.c
@@ -62,6 +62,9 @@ 
 # define NEED_MKTIME_WORKING 0
 #endif
 
+#ifdef _LIBC
+# include <tzset.h>
+#endif
 #include "mktime-internal.h"
 
 #if !defined _LIBC && (NEED_MKTIME_WORKING || NEED_MKTIME_WINDOWS)
@@ -250,6 +253,7 @@  tm_diff (long_int year, long_int yday, int hour, int min, int sec,
 		     tp->tm_hour, tp->tm_min, tp->tm_sec);
 }
 
+#ifndef _LIBC
 /* Convert T to a struct tm value in *TM.  Use localtime64_r if LOCAL,
    otherwise gmtime64_r.  T must be in range for __time64_t.  Return
    TM if successful, NULL (setting errno) on failure.  */
@@ -262,8 +266,8 @@  convert_time (long_int t, bool local, struct tm *tm)
   else
     return __gmtime64_r (&x, tm);
 }
-/* Call it __tzconvert to sync with other parts of glibc.  */
-#define __tz_convert convert_time
+# define __tz_convert convert_time
+#endif
 
 /* Convert *T to a broken down time in *TP (as if by localtime if
    LOCAL, otherwise as if by gmtime).  If *T is out of range for
@@ -320,7 +324,9 @@  ranged_convert (bool local, long_int *t, struct tm *tp)
    If *OFFSET's guess is correct, only one reverse mapping call is
    needed.  If successful, set *TP to the canonicalized struct tm;
    otherwise leave *TP alone, return ((time_t) -1) and set errno.
-   This function is external because it is used also by timegm.c.  */
+   This function is external because it is used also by timegm.c.
+
+   If _LIBC, the caller must lock __tzset_lock.  */
 __time64_t
 __mktime_internal (struct tm *tp, bool local, mktime_offset_t *offset)
 {
@@ -548,14 +554,15 @@  __mktime_internal (struct tm *tp, bool local, mktime_offset_t *offset)
 __time64_t
 __mktime64 (struct tm *tp)
 {
-  /* POSIX.1 requires mktime to set external variables like 'tzname'
-     as though tzset had been called.  */
-  __tzset ();
-
 # if defined _LIBC || NEED_MKTIME_WORKING
+  __libc_lock_lock (__tzset_lock);
+  __tzset_unlocked ();
   static mktime_offset_t localtime_offset;
-  return __mktime_internal (tp, true, &localtime_offset);
+  __time64_t result = __mktime_internal (tp, true, &localtime_offset);
+  __libc_lock_unlock (__tzset_lock);
+  return result;
 # else
+  __tzset ();
 #  undef mktime
   return mktime (tp);
 # endif
diff --git a/time/strftime_l.c b/time/strftime_l.c
index f51d926b46..584df83d15 100644
--- a/time/strftime_l.c
+++ b/time/strftime_l.c
@@ -33,6 +33,7 @@ 
 # define MULTIBYTE_IS_FORMAT_SAFE 1
 # define STDC_HEADERS 1
 # include "../locale/localeinfo.h"
+# include <tzset.h>
 #endif
 
 #if defined emacs && !defined HAVE_BCOPY
diff --git a/time/timegm.c b/time/timegm.c
index 4c2615b9fa..42115a5d7f 100644
--- a/time/timegm.c
+++ b/time/timegm.c
@@ -24,13 +24,20 @@ 
 #include <time.h>
 #include <errno.h>
 
+#ifdef _LIBC
+# include <tzset.h>
+#endif
 #include "mktime-internal.h"
 
 __time64_t
 __timegm64 (struct tm *tmp)
 {
+  __libc_lock_lock (__tzset_lock);
+  __tzset_unlocked ();
   static mktime_offset_t gmtime_offset;
-  return __mktime_internal (tmp, false, &gmtime_offset);
+  __time64_t result = __mktime_internal (tmp, false, &gmtime_offset);
+  __libc_lock_unlock (__tzset_lock);
+  return result;
 }
 
 #if defined _LIBC && __TIMESIZE != 64
diff --git a/time/tzfile.c b/time/tzfile.c
index bca8b3f4ef..fa2be56580 100644
--- a/time/tzfile.c
+++ b/time/tzfile.c
@@ -27,6 +27,7 @@ 
 #include <stdint.h>
 #include <alloc_buffer.h>
 #include <set-freeres.h>
+#include <tzset.h>
 
 #include <timezone/tzfile.h>
 
diff --git a/time/tzset.c b/time/tzset.c
index 1a9a03a436..a0bf3382ba 100644
--- a/time/tzset.c
+++ b/time/tzset.c
@@ -16,13 +16,13 @@ 
    <https://www.gnu.org/licenses/>.  */
 
 #include <ctype.h>
-#include <libc-lock.h>
 #include <stdbool.h>
 #include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <time.h>
+#include <tzset.h>
 
 #include <timezone/tzfile.h>
 
@@ -37,7 +37,7 @@  weak_alias (__daylight, daylight)
 weak_alias (__timezone, timezone)
 
 /* This locks all the state variables in tzfile.c and this file.  */
-__libc_lock_define_initialized (static, tzset_lock)
+__libc_lock_define_initialized (, __tzset_lock)
 
 /* This structure contains all the information about a
    timezone given in the POSIX standard TZ envariable.  */
@@ -529,10 +529,8 @@  __tz_compute (__time64_t timer, struct tm *tm, int use_localtime)
 #undef tzset
 
 void
-__tzset (void)
+__tzset_unlocked (void)
 {
-  __libc_lock_lock (tzset_lock);
-
   tzset_internal (1);
 
   if (!__use_tzfile)
@@ -541,9 +539,16 @@  __tzset (void)
       __tzname[0] = (char *) tz_rules[0].name;
       __tzname[1] = (char *) tz_rules[1].name;
     }
+}
 
-  __libc_lock_unlock (tzset_lock);
+void
+__tzset (void)
+{
+  __libc_lock_lock (__tzset_lock);
+  __tzset_unlocked ();
+  __libc_lock_unlock (__tzset_lock);
 }
+
 weak_alias (__tzset, tzset)
 
 /* Return the `struct tm' representation of TIMER in the local timezone.
@@ -554,8 +559,6 @@  __tz_convert (__time64_t timer, int use_localtime, struct tm *tp)
   long int leap_correction;
   int leap_extra_secs;
 
-  __libc_lock_lock (tzset_lock);
-
   /* Update internal database according to current TZ setting.
      POSIX.1 8.3.7.2 says that localtime_r is not required to set tzname.
      This is a good idea since this allows at least a bit more parallelism.  */
@@ -574,8 +577,6 @@  __tz_convert (__time64_t timer, int use_localtime, struct tm *tp)
       leap_extra_secs = 0;
     }
 
-  __libc_lock_unlock (tzset_lock);
-
   if (tp)
     {
       if (! use_localtime)
diff --git a/time/tzset.h b/time/tzset.h
new file mode 100644
index 0000000000..fb7984563e
--- /dev/null
+++ b/time/tzset.h
@@ -0,0 +1,30 @@ 
+#ifndef _TZSET_H
+#define _TZSET_H
+
+#include <time.h>
+#include <libc-lock.h>
+
+/* Defined in tzset.c.  */
+extern char *__tzstring (const char *string) attribute_hidden;
+
+extern int __use_tzfile attribute_hidden;
+
+extern void __tzfile_read (const char *file) attribute_hidden;
+extern void __tzfile_compute (__time64_t timer, int use_localtime,
+			      long int *leap_correct, int *leap_hit,
+			      struct tm *tp) attribute_hidden;
+extern void __tzset_parse_tz (const char *tz) attribute_hidden;
+extern void __tz_compute (__time64_t timer, struct tm *tm, int use_localtime)
+  __THROW attribute_hidden;
+
+__libc_lock_define (extern, __tzset_lock attribute_hidden);
+
+extern void __tzset (void) attribute_hidden;
+extern void __tzset_unlocked (void) attribute_hidden;
+
+/* Prototype for the internal function to get information based on TZ.
+   Caller needs to lock __tzset_lock.  */
+extern struct tm *__tz_convert (__time64_t timer, int use_localtime,
+				struct tm *tp) attribute_hidden;
+
+#endif /* _TZSET_H */