diff mbox series

[2/4] time: Push tzset lock into callers of time functions

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

Commit Message

Florian Weimer Oct. 1, 2024, 12:14 p.m. UTC 
This fixes a harmless data race on the cached offset used
by __mktime_internal.
---
 include/time.h         | 23 -----------------------
 time/gmtime.c          |  8 ++++++--
 time/localtime.c       |  8 ++++++--
 time/mktime-internal.h |  4 +++-
 time/mktime.c          | 36 +++++++++++++++++++++++++++++-------
 time/strftime_l.c      |  1 +
 time/timegm.c          | 11 +++++++++++
 time/tzfile.c          |  1 +
 time/tzset.c           | 20 ++++++++++++--------
 time/tzset.h           | 34 ++++++++++++++++++++++++++++++++++
 10 files changed, 103 insertions(+), 43 deletions(-)
 create mode 100644 time/tzset.h

Comments

Paul Eggert Oct. 1, 2024, 5:26 p.m. UTC | #1 
On 2024-10-01 05:14, Florian Weimer wrote:
>     if (local)
>       return __localtime64_r (&x, tm);
>     else
>       return __gmtime64_r (&x, tm);

Slightly more elegant (and more efficient with gcc -O2 on x86-64 at 
least, as it avoids a conditional branch) is:

    return (local ? __localtime64_r : __gmtime64_r) (&x, tm);
Paul Eggert Oct. 1, 2024, 5:35 p.m. UTC | #2 
On 2024-10-01 05:14, Florian Weimer wrote:
> +#ifdef _LIBC
> +# include <tzset.h>
> +#endif
>   
>   #include <errno.h>
>   #include <limits.h>
> @@ -256,11 +259,15 @@ tm_diff (long_int year, long_int yday, int hour, int min, int sec,
>   static struct tm *
>   convert_time (bool local, long_int t, struct tm *tm)
>   {
> +#ifdef _LIBC
> +  return __tz_convert (t, local, tm);
> +#else
>     __time64_t x = t;
>     if (local)
>       return __localtime64_r (&x, tm);
>     else
>       return __gmtime64_r (&x, tm);
> +#endif

Come to think of it, the !_LIBC code needn't fiddle with types like 
__time64_t or symbols like __localtime64_r because they're not relevant 
in Gnulib.

Please migrate the "#ifdef _LIBC" stuff higher, e.g., something like 
this near the top.:

   #ifdef _LIBC
   # include <tzset.h>
   #else
   static struct tm *
   __tz_convert (time_t t, bool local, struct tm *tm)
   {
      return (local ? localtime_r : gmtime_r) (&t, tm);
   }
   #endif

and then change the rest of the file to use __tz_convert instead of 
convert_time. That way, we can remove convert_time and the glibc code 
will be more uniform about calling __tz_convert.
Alexander Monakov Oct. 1, 2024, 5:37 p.m. UTC | #3 
On Tue, 1 Oct 2024, Paul Eggert wrote:

> On 2024-10-01 05:14, Florian Weimer wrote:
> >     if (local)
> >       return __localtime64_r (&x, tm);
> >     else
> >       return __gmtime64_r (&x, tm);
> 
> Slightly more elegant (and more efficient with gcc -O2 on x86-64 at least, as
> it avoids a conditional branch) is:
> 
>    return (local ? __localtime64_r : __gmtime64_r) (&x, tm);

On the flip side, in PIC for libc.so this needs a PC-relative computation,
which is not as cheap as amd64's rip-relative lea everywhere.

Alexander
Paul Eggert Oct. 1, 2024, 5:39 p.m. UTC | #4 
On 2024-10-01 05:14, Florian Weimer wrote:
> +#ifdef _LIBC
> +__time64_t
> +__mktime64 (struct tm *tp)
> +{
> +  __libc_lock_lock (__tzset_lock);
> +  __tzset_unlocked ();
> +  static mktime_offset_t localtime_offset;
> +  __time64_t result = __mktime_internal (tp, true, &localtime_offset);
> +  __libc_lock_unlock (__tzset_lock);
> +  return result;
> +}
> +#else /* !_LIBC */

Similarly, please don't have two versions of __mktime64. Just have the 
version quoted above without any "#ifdef _LIBC", and put the relevant 
_LIBC vs !_LIBC stuff in an "#ifdef _LIBC" near the top. This can be 
done similarly to how posix/regex_internal.h deals with __libc_lock_lock.

Thanks.
Paul Eggert Oct. 1, 2024, 5:51 p.m. UTC | #5 
On 2024-10-01 10:37, Alexander Monakov wrote:
> On the flip side, in PIC for libc.so this needs a PC-relative computation,
> which is not as cheap as amd64's rip-relative lea everywhere.

Good point. For what it's worth, on x86-64, gcc -fPIC -O2 generates a 
conditional branch no matter which way you code it, but the code quality 
is worse in the more-elegant formulation.

For Gnulib (which typically doesn't use -fPIC) the more-elegant version 
is a performance win; for glibc not so much. I wouldn't favor an "#if 
_LIBC" here, so it's just an elegance-vs-performance issue and it'd be 
fine to ignore my suggestion.
diff mbox series

Patch

diff --git a/include/time.h b/include/time.h
index f599eeed4e..e9c083646a 100644
--- a/include/time.h
+++ b/include/time.h
@@ -49,24 +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, size_t extra,
-			   char **extrap) 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 __tzfile_default (const char *std, const char *dst,
-			      int stdoff, int dstoff)
-  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
@@ -275,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 9f39c36bd8..ad8fc48966 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 77d19ff5e7..f19d552673 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 3e2848c121..93299effc3 100644
--- a/time/mktime-internal.h
+++ b/time/mktime-internal.h
@@ -73,6 +73,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 8196faa49c..669bc46c34 100644
--- a/time/mktime.c
+++ b/time/mktime.c
@@ -42,6 +42,9 @@ 
 #endif
 
 #include <time.h>
+#ifdef _LIBC
+# include <tzset.h>
+#endif
 
 #include <errno.h>
 #include <limits.h>
@@ -256,11 +259,15 @@  tm_diff (long_int year, long_int yday, int hour, int min, int sec,
 static struct tm *
 convert_time (bool local, long_int t, struct tm *tm)
 {
+#ifdef _LIBC
+  return __tz_convert (t, local, tm);
+#else
   __time64_t x = t;
   if (local)
     return __localtime64_r (&x, tm);
   else
     return __gmtime64_r (&x, tm);
+#endif
 }
 
 /* Convert *T to a broken down time in *TP (as if by localtime if
@@ -318,7 +325,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)
 {
@@ -530,7 +539,19 @@  __mktime_internal (struct tm *tp, bool local, mktime_offset_t *offset)
 
 #endif /* _LIBC || NEED_MKTIME_WORKING || NEED_MKTIME_INTERNAL */
 
-#if defined _LIBC || NEED_MKTIME_WORKING || NEED_MKTIME_WINDOWS
+#ifdef _LIBC
+__time64_t
+__mktime64 (struct tm *tp)
+{
+  __libc_lock_lock (__tzset_lock);
+  __tzset_unlocked ();
+  static mktime_offset_t localtime_offset;
+  __time64_t result = __mktime_internal (tp, true, &localtime_offset);
+  __libc_lock_unlock (__tzset_lock);
+  return result;
+}
+#else /* !_LIBC */
+# if defined NEED_MKTIME_WORKING || NEED_MKTIME_WINDOWS
 
 /* Convert *TP to a __time64_t value.  */
 __time64_t
@@ -541,15 +562,16 @@  __mktime64 (struct tm *tp)
      be set as if the tzset() function had been called.  */
   __tzset ();
 
-# if defined _LIBC || NEED_MKTIME_WORKING
+#  ifdef NEED_MKTIME_WORKING
   static mktime_offset_t localtime_offset;
   return __mktime_internal (tp, true, &localtime_offset);
-# else
-#  undef mktime
+#  else
+#   undef mktime
   return mktime (tp);
-# endif
+#  endif
 }
-#endif /* _LIBC || NEED_MKTIME_WORKING || NEED_MKTIME_WINDOWS */
+# endif /* NEED_MKTIME_WORKING || NEED_MKTIME_WINDOWS */
+#endif /* !_LIBC */
 
 #if defined _LIBC && __TIMESIZE != 64
 
diff --git a/time/strftime_l.c b/time/strftime_l.c
index 77adec9050..e5c5d5fc60 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 1f1f66c61b..4dda728ae3 100644
--- a/time/timegm.c
+++ b/time/timegm.c
@@ -24,6 +24,10 @@ 
 #include <time.h>
 #include <errno.h>
 
+#ifdef _LIBC
+# include <tzset.h>
+#endif
+
 #include "mktime-internal.h"
 
 __time64_t
@@ -31,7 +35,14 @@  __timegm64 (struct tm *tmp)
 {
   static mktime_offset_t gmtime_offset;
   tmp->tm_isdst = 0;
+#ifdef _LIBC
+  __libc_lock_lock (__tzset_lock);
+  __time64_t result = __mktime_internal (tmp, false, &gmtime_offset);
+  __libc_lock_unlock (__tzset_lock);
+  return result;
+#else
   return __mktime_internal (tmp, false, &gmtime_offset);
+#endif
 }
 
 #if defined _LIBC && __TIMESIZE != 64
diff --git a/time/tzfile.c b/time/tzfile.c
index 964a9b7f12..5d4a6054df 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 98a7064970..b0a0bcf68f 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.  */
@@ -544,9 +544,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);
 
@@ -557,8 +556,16 @@  __tzset (void)
       __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.
@@ -569,7 +576,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.
@@ -589,8 +595,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..254540aa05
--- /dev/null
+++ b/time/tzset.h
@@ -0,0 +1,34 @@ 
+#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, size_t extra,
+                           char **extrap) 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 __tzfile_default (const char *std, const char *dst,
+                              int stdoff, int dstoff)
+  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 */