[v2,2/2] Y2038: make __tz_convert compatible with 64-bit-time

Message ID 20180614135116.8767-3-albert.aribaud@3adev.fr
State New, archived
Headers

Commit Message

Albert ARIBAUD June 14, 2018, 1:51 p.m. UTC
  This implies that its callers be 64-bit-time compatible too.
It is done by creating 64-bit-time versions of these and
turning their original 32-bit-time versions into wrappers
(at a slight execution time cost).

The callers affected are:

  * localtime
  * localtime_r
  * ctime
  * ctime_r
  * gmtime
  * gmtime_r

Note that in time/tzfile.c we do not need to check for time_t
overflows anymore as introduced by commit fc79706a323 since we
now use internal_time_t.
---
 include/time.h   | 36 +++++++++++++++++++++++++++++++-----
 time/ctime.c     | 20 +++++++++++++++++---
 time/ctime_r.c   | 20 +++++++++++++++++---
 time/gmtime.c    | 40 +++++++++++++++++++++++++++++++++++-----
 time/localtime.c | 37 +++++++++++++++++++++++++++++++++----
 time/offtime.c   | 12 ++++++------
 time/tzfile.c    | 14 ++++----------
 time/tzset.c     | 30 ++++++++++++------------------
 8 files changed, 155 insertions(+), 54 deletions(-)
  

Comments

Paul Eggert June 14, 2018, 5:07 p.m. UTC | #1
On 06/14/2018 06:51 AM, Albert ARIBAUD (3ADEV) wrote:
> +#if __TIMESIZE == 64
> +# define __ctime64 ctime
> +# define __ctime64_r ctime_r
> +#endif
> +
> +#if __TIMESIZE == 64
> +# define __localtime64 localtime
> +#else
> +extern struct tm *__localtime64 (const __time64_t *__timer);
> +#endif

These don't seem symmetric. Why don't we need an extern declaration for 
__ctime_64 or for __ctime_64_r? If the code is correct, maybe a comment why?

> +#if (__TIMESIZE != 64)
No parentheses here (and similarly elsewhere).

+/* The C Standard says ctime (t) is equivalent to asctime (localtime (t)).
+   In particular, ctime and asctime must yield the same pointer. */
+  return asctime (__localtime64 (t));

Please indent the comment to be like what it was before.

Otherwise looks good; thanks.
  
Albert ARIBAUD June 14, 2018, 5:51 p.m. UTC | #2
Hi Paul,

On Thu, 14 Jun 2018 10:07:18 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 06/14/2018 06:51 AM, Albert ARIBAUD (3ADEV) wrote:
> > +#if __TIMESIZE == 64
> > +# define __ctime64 ctime
> > +# define __ctime64_r ctime_r
> > +#endif
> > +
> > +#if __TIMESIZE == 64
> > +# define __localtime64 localtime
> > +#else
> > +extern struct tm *__localtime64 (const __time64_t *__timer);
> > +#endif  
> 
> These don't seem symmetric. Why don't we need an extern declaration for 
> __ctime_64 or for __ctime_64_r? If the code is correct, maybe a comment why?
 
The asymmetry (that ctime and ctime_r are only declared in time/time.h
whereas localtime, localtime_r, gmtime and gmtime_r are declared both in
include/time.h as well as time/time.h) existed before my patches, and I
do not know the reason.

I felt I should not add declarations for ctime / ctime_r in
include/time as obviously glibc builds fine without these, and my
patches are not about adding 32-bit declarations but about adding
64-bit support.

However, I can add them if requested.

> > +#if (__TIMESIZE != 64)  
> No parentheses here (and similarly elsewhere).

Fixed everywhere.

> +/* The C Standard says ctime (t) is equivalent to asctime (localtime (t)).
> +   In particular, ctime and asctime must yield the same pointer. */
> +  return asctime (__localtime64 (t));
> 
> Please indent the comment to be like what it was before.

Done.

> Otherwise looks good; thanks.

Thanks to you for the reviews!

Cordialement,
Albert ARIBAUD
3ADEV
  
Paul Eggert June 14, 2018, 6:51 p.m. UTC | #3
On 06/14/2018 10:51 AM, Albert ARIBAUD wrote:
> The asymmetry (that ctime and ctime_r are only declared in time/time.h
> whereas localtime, localtime_r, gmtime and gmtime_r are declared both in
> include/time.h as well as time/time.h) existed before my patches, and I
> do not know the reason.

I suspect it's because time/time.h is intended to be public (copied to 
/usr/include/time.h) whereas include/time.h is merely private to glibc. 
Since ctime and ctime_r are not useful internally (they are 
specializations of strftime that are standardized only because of 
inertia: 7th Edition Unix had ctime but not strftime), there is no need 
for private declarations of ctime and ctime_r in include/time.h because 
glibc code never calls these functions.

Given all that, why bother defining __ctime64 and __ctime64_r in 
include/time.h? glibc code never calls these functions so these #defines 
can be localized to time/ctime.c and time/ctime_r.c respectively, with a 
comment explaining why the #defines are local.

Also, isn't glibc a bit busted in this area? ctime_r is not part of 
standard C, so why isn't ctime_r treated like asctime_r, with a weak 
alias? Isn't this a (separate) bug that needs to be fixed, so that the 
resulting weak alias is like the one for localtime_r as far as 64-bit 
times is concerned?
  
Joseph Myers June 14, 2018, 7:40 p.m. UTC | #4
On Thu, 14 Jun 2018, Paul Eggert wrote:

> Also, isn't glibc a bit busted in this area? ctime_r is not part of standard
> C, so why isn't ctime_r treated like asctime_r, with a weak alias? Isn't this
> a (separate) bug that needs to be fixed, so that the resulting weak alias is
> like the one for localtime_r as far as 64-bit times is concerned?

localtime_r is defined in the same file as localtime, so needs to be weak.  
ctime_r is not defined in the same file as ctime, and is not referenced by 
the implementations of any standard C functions, so does not need to be 
weak.  (If it were referenced by a standard C function, the reference 
would have to be to __ctime_r and ctime_r would have to be a weak alias of 
that.)
  
Albert ARIBAUD June 16, 2018, 2:02 p.m. UTC | #5
Hi Joseph,

On Thu, 14 Jun 2018 19:40:28 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :

> On Thu, 14 Jun 2018, Paul Eggert wrote:
> 
> > Also, isn't glibc a bit busted in this area? ctime_r is not part of standard
> > C, so why isn't ctime_r treated like asctime_r, with a weak alias? Isn't this
> > a (separate) bug that needs to be fixed, so that the resulting weak alias is
> > like the one for localtime_r as far as 64-bit times is concerned?  
> 
> localtime_r is defined in the same file as localtime, so needs to be weak.  
> ctime_r is not defined in the same file as ctime, and is not referenced by 
> the implementations of any standard C functions, so does not need to be 
> weak.  (If it were referenced by a standard C function, the reference 
> would have to be to __ctime_r and ctime_r would have to be a weak alias of 
> that.)
> 

Well, I can tell that if I remove the __ctime64 and __ctime64_r defines
and then build glibc for x86_64-linux-gnu, the linker stage fails due to
nis/nis_print.c lines 336 and 338 referring to an undefined ctime().

(OTOH, ctime_r is not referred to at all in glibc)

So at least I need to keep the '#define __ctime64 ctime' line -- I will
add a comment above it indicating that nis/nos_print.c needs it, and I
will remove the __ctime64_r define. 

Cordialement,
Albert ARIBAUD
3ADEV
  
Paul Eggert June 16, 2018, 2:23 p.m. UTC | #6
Albert ARIBAUD wrote:
> if I remove the __ctime64 and __ctime64_r defines
> and then build glibc for x86_64-linux-gnu, the linker stage fails due to
> nis/nis_print.c lines 336 and 338 referring to an undefined ctime().

How about if we fix nis/nis_print.c to use localtime_r and strftime instead of 
ctime? ctime is not thread-safe, and ctime_r is problematic due to the potential 
problem of overrunning its output buffer when given outlandish timestamps, so 
such a fix should be made regardless of the Y2038 changes.
  
Albert ARIBAUD June 16, 2018, 6:20 p.m. UTC | #7
Hi Paul,

On Sat, 16 Jun 2018 07:23:29 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> Albert ARIBAUD wrote:
> > if I remove the __ctime64 and __ctime64_r defines
> > and then build glibc for x86_64-linux-gnu, the linker stage fails due to
> > nis/nis_print.c lines 336 and 338 referring to an undefined ctime().  
> 
> How about if we fix nis/nis_print.c to use localtime_r and strftime instead of 
> ctime? ctime is not thread-safe, and ctime_r is problematic due to the potential 
> problem of overrunning its output buffer when given outlandish timestamps, so 
> such a fix should be made regardless of the Y2038 changes.

I can do this in a further patch if/when that's agreed upon.

Cordialement,
Albert ARIBAUD
3ADEV
  

Patch

diff --git a/include/time.h b/include/time.h
index fea072cbb6..b314317650 100644
--- a/include/time.h
+++ b/include/time.h
@@ -47,7 +47,7 @@  extern void __tzfile_default (const char *std, const char *dst,
 			      long int stdoff, long int dstoff)
   attribute_hidden;
 extern void __tzset_parse_tz (const char *tz) attribute_hidden;
-extern void __tz_compute (time_t timer, struct tm *tm, int use_localtime)
+extern void __tz_compute (__time64_t timer, struct tm *tm, int use_localtime)
   __THROW attribute_hidden;
 
 /* Subroutine of `mktime'.  Return the `time_t' representation of TP and
@@ -57,18 +57,44 @@  extern time_t __mktime_internal (struct tm *__tp,
 				 struct tm *(*__func) (const time_t *,
 						       struct tm *),
 				 time_t *__offset) attribute_hidden;
+#if __TIMESIZE == 64
+# define __ctime64 ctime
+# define __ctime64_r ctime_r
+#endif
+
+#if __TIMESIZE == 64
+# define __localtime64 localtime
+#else
+extern struct tm *__localtime64 (const __time64_t *__timer);
+#endif
+
 extern struct tm *__localtime_r (const time_t *__timer,
 				 struct tm *__tp) attribute_hidden;
 
+#if __TIMESIZE == 64
+# define __localtime64_r __localtime_r
+#else
+extern struct tm *__localtime64_r (const __time64_t *__timer,
+				   struct tm *__tp) attribute_hidden;
+#endif
+
 extern struct tm *__gmtime_r (const time_t *__restrict __timer,
 			      struct tm *__restrict __tp);
 libc_hidden_proto (__gmtime_r)
 
-/* Compute the `struct tm' representation of *T,
+#if __TIMESIZE == 64
+# define __gmtime64 gmtime
+# define __gmtime64_r __gmtime_r
+#else
+extern struct tm *__gmtime64_r (const __time64_t *__restrict __timer,
+			        struct tm *__restrict __tp);
+#endif
+
+/* Compute the `struct tm' representation of T,
    offset OFFSET seconds east of UTC,
    and store year, yday, mon, mday, wday, hour, min, sec into *TP.
    Return nonzero if successful.  */
-extern int __offtime (const time_t *__timer,
+extern int __offtime (const __time64_t __timer,
 		      long int __offset,
 		      struct tm *__tp) attribute_hidden;
 
@@ -77,8 +103,8 @@  extern char *__asctime_r (const struct tm *__tp, char *__buf)
 extern void __tzset (void) attribute_hidden;
 
 /* Prototype for the internal function to get information based on TZ.  */
-extern struct tm *__tz_convert (const time_t *timer, int use_localtime,
-				struct tm *tp) attribute_hidden;
+extern struct tm *__tz_convert (const __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/ctime.c b/time/ctime.c
index 1222614f29..ee98ed902e 100644
--- a/time/ctime.c
+++ b/time/ctime.c
@@ -16,13 +16,27 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #include <time.h>
+#include <errno.h>
 
 /* Return a string as returned by asctime which
    is the representation of *T in that form.  */
+char *
+__ctime64 (const __time64_t *t)
+{
+/* The C Standard says ctime (t) is equivalent to asctime (localtime (t)).
+   In particular, ctime and asctime must yield the same pointer.  */
+  return asctime (__localtime64 (t));
+}
+
+/* Provide a 32-bit wrapper if needed */
+
+#if (__TIMESIZE != 64)
+
 char *
 ctime (const time_t *t)
 {
-  /* The C Standard says ctime (t) is equivalent to asctime (localtime (t)).
-     In particular, ctime and asctime must yield the same pointer.  */
-  return asctime (localtime (t));
+  __time64_t t64 = *t;
+  return __ctime64 (&t64);
 }
+
+#endif
diff --git a/time/ctime_r.c b/time/ctime_r.c
index c111146d76..d042adffce 100644
--- a/time/ctime_r.c
+++ b/time/ctime_r.c
@@ -18,12 +18,26 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #include <time.h>
+#include <errno.h>
 
 /* Return a string as returned by asctime which is the representation
-   of *T in that form.  Reentrant version.  */
+   of *T in that form.  */
 char *
-ctime_r (const time_t *t, char *buf)
+__ctime64_r (const __time64_t *t, char *buf)
 {
   struct tm tm;
-  return __asctime_r (__localtime_r (t, &tm), buf);
+  return __asctime_r (__localtime64_r (t, &tm), buf);
 }
+
+/* Provide a 32-bit wrapper if needed */
+
+#if (__TIMESIZE != 64)
+
+char *
+ctime_r (const time_t *t, char *buf)
+{
+  __time64_t t64 = *t;
+  return __ctime64_r (&t64, buf);
+}
+
+#endif
diff --git a/time/gmtime.c b/time/gmtime.c
index dc33b3e68a..51560a5bab 100644
--- a/time/gmtime.c
+++ b/time/gmtime.c
@@ -17,21 +17,51 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #include <time.h>
+#include <errno.h>
+
+/* Return the `struct tm' representation of 64-bit-time *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);
+}
+
+/* Provide a 32-bit wrapper if needed */
+
+#if (__TIMESIZE != 64)
 
-/* Return the `struct tm' representation of *T in UTC,
-   using *TP to store the result.  */
 struct tm *
 __gmtime_r (const time_t *t, struct tm *tp)
 {
-  return __tz_convert (t, 0, tp);
+  __time64_t t64 = *t;
+  return __gmtime64_r (&t64, tp);
 }
+
+#endif
+
+/* This always works because either __TIMESIZE != 64 and __gmtime_r exists
+   or __TIMESIZE == 64 and the definition of __gmtime64_r above actually
+   defined __gmtime_r.  */
 libc_hidden_def (__gmtime_r)
 weak_alias (__gmtime_r, gmtime_r)
 
+/* Return the `struct tm' representation of 64-bit-time *T in UTC.	*/
+struct tm *
+__gmtime64 (const __time64_t *t)
+{
+  return __tz_convert (*t, 0, &_tmbuf);
+}
+
+/* Provide a 32-bit wrapper if needed */
+
+#if (__TIMESIZE != 64)
 
-/* Return the `struct tm' representation of *T in UTC.	*/
 struct tm *
 gmtime (const time_t *t)
 {
-  return __tz_convert (t, 0, &_tmbuf);
+  __time64_t t64 = *t;
+  return __gmtime64 (&t64);
 }
+
+#endif
diff --git a/time/localtime.c b/time/localtime.c
index 8684a8a971..9794d8c28c 100644
--- a/time/localtime.c
+++ b/time/localtime.c
@@ -17,25 +17,54 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #include <time.h>
+#include <errno.h>
 
 /* The C Standard says that localtime and gmtime return the same pointer.  */
 struct tm _tmbuf;
 
-
 /* Return the `struct tm' representation of *T in local time,
    using *TP to store the result.  */
+struct tm *
+__localtime64_r (const __time64_t *t, struct tm *tp)
+{
+  return __tz_convert (*t, 1, tp);
+}
+
+/* Provide a 32-bit wrapper if needed */
+
+#if (__TIMESIZE != 64)
+
 struct tm *
 __localtime_r (const time_t *t, struct tm *tp)
 {
-  return __tz_convert (t, 1, tp);
+  __time64_t t64 = *t;
+  return __localtime64_r (&t64, tp);
 }
-weak_alias (__localtime_r, localtime_r)
 
+#endif
+
+/* This always works because either __TIMESIZE != 64 and __localtime_r
+   exists or __TIMESIZE == 64 and the definition of __localtime64_r above
+   actually defined __localtime_r.  */
+weak_alias (__localtime_r, localtime_r)
 
 /* Return the `struct tm' representation of *T in local time.  */
+struct tm *
+__localtime64 (const __time64_t *t)
+{
+  return __tz_convert (*t, 1, &_tmbuf);
+}
+
+/* Provide a 32-bit wrapper if needed */
+
+#if (__TIMESIZE != 64)
+
 struct tm *
 localtime (const time_t *t)
 {
-  return __tz_convert (t, 1, &_tmbuf);
+  __time64_t t64 = *t;
+  return __localtime64 (&t64);
 }
 libc_hidden_def (localtime)
+
+#endif
diff --git a/time/offtime.c b/time/offtime.c
index 04c48389fc..ede23c418f 100644
--- a/time/offtime.c
+++ b/time/offtime.c
@@ -21,18 +21,18 @@ 
 #define	SECS_PER_HOUR	(60 * 60)
 #define	SECS_PER_DAY	(SECS_PER_HOUR * 24)
 
-/* Compute the `struct tm' representation of *T,
+/* Compute the `struct tm' representation of T,
    offset OFFSET seconds east of UTC,
    and store year, yday, mon, mday, wday, hour, min, sec into *TP.
    Return nonzero if successful.  */
 int
-__offtime (const time_t *t, long int offset, struct tm *tp)
+__offtime (const __time64_t t, long int offset, struct tm *tp)
 {
-  time_t days, rem, y;
+  __time64_t days, rem, y;
   const unsigned short int *ip;
 
-  days = *t / SECS_PER_DAY;
-  rem = *t % SECS_PER_DAY;
+  days = t / SECS_PER_DAY;
+  rem = t % SECS_PER_DAY;
   rem += offset;
   while (rem < 0)
     {
@@ -60,7 +60,7 @@  __offtime (const time_t *t, long int offset, struct tm *tp)
   while (days < 0 || days >= (__isleap (y) ? 366 : 365))
     {
       /* Guess a corrected year, assuming 365 days per year.  */
-      time_t yg = y + days / 365 - (days % 365 < 0);
+      __time64_t yg = y + days / 365 - (days % 365 < 0);
 
       /* Adjust DAYS and Y to match the guessed year.  */
       days -= ((yg - y) * 365
diff --git a/time/tzfile.c b/time/tzfile.c
index d7e391c3a3..972e3ff5cf 100644
--- a/time/tzfile.c
+++ b/time/tzfile.c
@@ -635,16 +635,10 @@  __tzfile_compute (__time64_t timer, int use_localtime,
 
 	  /* Convert to broken down structure.  If this fails do not
 	     use the string.  */
-	  {
-	    time_t truncated = timer;
-	    if (__glibc_unlikely (truncated != timer
-				  || ! __offtime (&truncated, 0, tp)))
-	      goto use_last;
-	  }
-
-	  /* Use the rules from the TZ string to compute the change.
-	     timer fits into time_t due to the truncation check
-	     above.  */
+	  if (__glibc_unlikely (! __offtime (timer, 0, tp)))
+	    goto use_last;
+
+	  /* Use the rules from the TZ string to compute the change.  */
 	  __tz_compute (timer, tp, 1);
 
 	  /* If tzspec comes from posixrules loaded by __tzfile_default,
diff --git a/time/tzset.c b/time/tzset.c
index a828b9fb75..4e91d9dca6 100644
--- a/time/tzset.c
+++ b/time/tzset.c
@@ -16,7 +16,6 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #include <ctype.h>
-#include <errno.h>
 #include <libc-lock.h>
 #include <stdbool.h>
 #include <stddef.h>
@@ -27,7 +26,7 @@ 
 
 #include <timezone/tzfile.h>
 
-#define SECSPERDAY ((time_t) 86400)
+#define SECSPERDAY ((__time64_t) 86400)
 
 char *__tzname[2] = { (char *) "GMT", (char *) "GMT" };
 int __daylight = 0;
@@ -55,7 +54,7 @@  typedef struct
 
     /* We cache the computed time of change for a
        given year so we don't have to recompute it.  */
-    time_t change;	/* When to change to this zone.  */
+    __time64_t change;	/* When to change to this zone.  */
     int computed_for;	/* Year above is computed for.  */
   } tz_rule;
 
@@ -416,7 +415,7 @@  tzset_internal (int always)
       tz_rules[0].name = tz_rules[1].name = "UTC";
       if (J0 != 0)
 	tz_rules[0].type = tz_rules[1].type = J0;
-      tz_rules[0].change = tz_rules[1].change = (time_t) -1;
+      tz_rules[0].change = tz_rules[1].change = -1;
       update_vars ();
       return;
     }
@@ -424,13 +423,13 @@  tzset_internal (int always)
   __tzset_parse_tz (tz);
 }
 
-/* Figure out the exact time (as a time_t) in YEAR
+/* Figure out the exact time (as a __time64_t) in YEAR
    when the change described by RULE will occur and
    put it in RULE->change, saving YEAR in RULE->computed_for.  */
 static void
 compute_change (tz_rule *rule, int year)
 {
-  time_t t;
+  __time64_t t;
 
   if (year != -1 && rule->computed_for == year)
     /* Operations on times in 2 BC will be slower.  Oh well.  */
@@ -514,9 +513,10 @@  compute_change (tz_rule *rule, int year)
 
 
 /* Figure out the correct timezone for TM and set `__tzname',
-   `__timezone', and `__daylight' accordingly.  */
+   `__timezone', and `__daylight' accordingly.
+   NOTE: this takes a __time64_t value, so passing a time_t value is OK. */
 void
-__tz_compute (time_t timer, struct tm *tm, int use_localtime)
+__tz_compute (__time64_t timer, struct tm *tm, int use_localtime)
 {
   compute_change (&tz_rules[0], 1900 + tm->tm_year);
   compute_change (&tz_rules[1], 1900 + tm->tm_year);
@@ -562,20 +562,14 @@  __tzset (void)
 }
 weak_alias (__tzset, tzset)
 
-/* Return the `struct tm' representation of *TIMER in the local timezone.
+/* Return the `struct tm' representation of TIMER in the local timezone.
    Use local time if USE_LOCALTIME is nonzero, UTC otherwise.  */
 struct tm *
-__tz_convert (const time_t *timer, int use_localtime, struct tm *tp)
+__tz_convert (const __time64_t timer, int use_localtime, struct tm *tp)
 {
   long int leap_correction;
   int leap_extra_secs;
 
-  if (timer == NULL)
-    {
-      __set_errno (EINVAL);
-      return NULL;
-    }
-
   __libc_lock_lock (tzset_lock);
 
   /* Update internal database according to current TZ setting.
@@ -584,14 +578,14 @@  __tz_convert (const time_t *timer, int use_localtime, struct tm *tp)
   tzset_internal (tp == &_tmbuf && use_localtime);
 
   if (__use_tzfile)
-    __tzfile_compute (*timer, use_localtime, &leap_correction,
+    __tzfile_compute (timer, use_localtime, &leap_correction,
 		      &leap_extra_secs, tp);
   else
     {
       if (! __offtime (timer, 0, tp))
 	tp = NULL;
       else
-	__tz_compute (*timer, tp, use_localtime);
+	__tz_compute (timer, tp, use_localtime);
       leap_correction = 0L;
       leap_extra_secs = 0;
     }