[v3] Y2038: make __mktime_internal compatibles with __time64_t

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

Commit Message

Albert ARIBAUD Oct. 17, 2018, 9:31 a.m. UTC
  From: Paul Eggert <eggert@cs.ucla.edu>

This implies also making its callers 64-bit-time compatible
(these are mktime/localtime and timegm) and providing wrappers
for 32-bit-time userland to call.

This patch was tested by running 'make check' on branch
master then applying this patch and its two predecessors and
running 'make check' again, and checking that both 'make check'
yield identical results. This was done on x86_64-linux-gnu and
i686-linux-gnu.

* include/time.h (__mktime64): Add prototype.
* include/time.h (__localtime64): Likewise.
* include/time.h (fits_in_time_t): New static function.
* time/mktime.c (__mktime64): New function.
* time/timegm.c (__timegm64): Likewise.
* time/mktime.c (mktime) [__TIMESIZE]: New wrapper function.
* time/timegm.c (timegm) [__TIMESIZE]: Likewise.
---
 include/time.h | 33 ++++++++++++++++++----
 time/mktime.c  | 76 +++++++++++++++++++++++++++++++++-----------------
 time/timegm.c  | 21 ++++++++++++--
 3 files changed, 96 insertions(+), 34 deletions(-)
  

Comments

Albert ARIBAUD Oct. 17, 2018, 9:33 a.m. UTC | #1
On Wed, 17 Oct 2018 11:31:26 +0200, "Albert ARIBAUD (3ADEV)"
<albert.aribaud@3adev.fr> wrote :

> From: Paul Eggert <eggert@cs.ucla.edu>

I set Paul as the author because this patch is the one Paul suggested
in https://lists.gnu.org/archive/html/bug-gnulib/2018-09/msg00005.html
only adapted to glibc.

Cordialement,
Albert ARIBAUD
3ADEV
  
Albert ARIBAUD Oct. 17, 2018, 9:35 a.m. UTC | #2
On Wed, 17 Oct 2018 11:31:26 +0200, "Albert ARIBAUD (3ADEV)"
<albert.aribaud@3adev.fr> wrote :

... and of course I only notice the erroneous plural in the subject
now. :(

I will fix it in the next iteration of patch along with any other
changes requested in this thread.

Cordialement,
Albert ARIBAUD
3ADEV
  
Paul Eggert Oct. 18, 2018, 1:44 a.m. UTC | #3
This patch looks OK to me, modulo the spelling fix.

In reviewing this patch I noticed a longstanding bug in mktime that 
hasn't been reported. I just now reported it here:

https://sourceware.org/bugzilla/show_bug.cgi?id=23789

Fixing that bug may affect the patch.
  
Albert ARIBAUD Oct. 24, 2018, 11:23 a.m. UTC | #4
Hi Paul,

On Wed, 17 Oct 2018 18:44:05 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> This patch looks OK to me, modulo the spelling fix.
> 
> In reviewing this patch I noticed a longstanding bug in mktime that 
> hasn't been reported. I just now reported it here:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=23789
> 
> Fixing that bug may affect the patch.

Should I go and provide a fix to that bug, then, and resubmit my patch
over the fix?

Apparently the fix 'only' involves ensuring mktime() sets errno to
EOVERFLOW (as per Posix) if its call to __mktime_internal() returned -1.

In the case where neither _LIBC nor NEED_MKTIME_WORKING are defined,
"another" mktime() is called. Do we assume this other mktime will set
errno to EOVERFLOW, or do we do it too in that case?

Also, I assume tests would be needed for both cases where
__mktime_internal() returns -1? If so, I would need struct tm values
which trigger either one, or at least indications on how to find ones.

Cordialement,
Albert ARIBAUD
3ADEV
  
Florian Weimer Oct. 24, 2018, 11:25 a.m. UTC | #5
* Albert ARIBAUD:

> Hi Paul,
>
> On Wed, 17 Oct 2018 18:44:05 -0700, Paul Eggert <eggert@cs.ucla.edu>
> wrote :
>
>> This patch looks OK to me, modulo the spelling fix.
>> 
>> In reviewing this patch I noticed a longstanding bug in mktime that 
>> hasn't been reported. I just now reported it here:
>> 
>> https://sourceware.org/bugzilla/show_bug.cgi?id=23789
>> 
>> Fixing that bug may affect the patch.
>
> Should I go and provide a fix to that bug, then, and resubmit my patch
> over the fix?

I do not have a strong preference here, as long as the bug fix goes into
a separate patch.

(Despite what I just told to H.J., I think in general we should not
force contributors to fix peripherally related bugs as part of the work
they set out to do.)

Thanks,
Florian
  
Albert ARIBAUD Oct. 24, 2018, 11:37 a.m. UTC | #6
Hi Florian,

On Wed, 24 Oct 2018 13:25:59 +0200, Florian Weimer <fweimer@redhat.com>
wrote :

> * Albert ARIBAUD:
> 
> > Hi Paul,
> >
> > On Wed, 17 Oct 2018 18:44:05 -0700, Paul Eggert <eggert@cs.ucla.edu>
> > wrote :
> >  
> >> This patch looks OK to me, modulo the spelling fix.
> >> 
> >> In reviewing this patch I noticed a longstanding bug in mktime that 
> >> hasn't been reported. I just now reported it here:
> >> 
> >> https://sourceware.org/bugzilla/show_bug.cgi?id=23789
> >> 
> >> Fixing that bug may affect the patch.  
> >
> > Should I go and provide a fix to that bug, then, and resubmit my patch
> > over the fix?  
> 
> I do not have a strong preference here, as long as the bug fix goes into
> a separate patch.
> 
> (Despite what I just told to H.J., I think in general we should not
> force contributors to fix peripherally related bugs as part of the work
> they set out to do.)

I agree with this approach in the general case, however I would like to
point out that in this instance I had not read or felt Paul's comment
as forcing me to handle the bug in any way; on the contrary, I have
seized it as an opportunity to get acquainted with the bugzilla part of
the patch committing process as well as with adding tests to glibc,
since Y2038 will quite probably require adding such tests.

> Thanks,
> Florian

Cordialement,
Albert ARIBAUD
3ADEV
  
Albert ARIBAUD Oct. 24, 2018, 3:59 p.m. UTC | #7
On Wed, 24 Oct 2018 13:23:18 +0200, Albert ARIBAUD
<albert.aribaud@3adev.fr> wrote :

> Also, I assume tests would be needed for both cases where
> __mktime_internal() returns -1? If so, I would need struct tm values
> which trigger either one, or at least indications on how to find ones.

Regarding the tests:

There are already four mktime test files used by 'make check':

	time/tst-mktime.c
	time/bug-mktime1.c
	time/tst-mktime2.c
	time/tst-mktime3.c

If I am getting the logic right, I should not extend any of these files,
but rather, I should add a new file, e.g.:

	time/bug-mktime4.c

which would essentially be an adaptation of the test file attached by
Paul to the bugzilla entry:

https://sourceware.org/bugzilla/attachment.cgi?id=11343

BTW: Paul, your program returns 2 when errno is not EOVERFLOW, but 2 is
not supported as an EXIT_* value, only 0 (EXIT_SUCCESS), 1
(EXIT_FAILURE) and 77 (EXIT_UNSUPPORTED). I assume that was an ad hoc
choice to distinguish the "errno is not EOVERFLOW" failure from a
hypothetical "mktime did not return -1" failure. Within the glibc test
framework, I suspect we would return 1 / EXIT_FAILURE for both, and
print the detailed failure reason on stdout.

I am not entirely sure about the 'bug-' vs 'tst-' prefix, though; I
assumed that since we have a BZ id for the problem, it should be
'bug-', but does it work that way?

And one last question: should I not first commit the test (and with make
check on a 32-bit architecture, show that it adds one FAIL) then commit
the fix (and show with the same make check that the FAIL disappears)?

Cordialement,
Albert ARIBAUD
3ADEV
  
Joseph Myers Oct. 24, 2018, 4:20 p.m. UTC | #8
On Wed, 24 Oct 2018, Albert ARIBAUD wrote:

> And one last question: should I not first commit the test (and with make
> check on a 32-bit architecture, show that it adds one FAIL) then commit
> the fix (and show with the same make check that the FAIL disappears)?

No, you should verify the FAIL with the fix not present, but the test is 
best committed at the same time as a fix, to preserve bisectability.
  
Albert ARIBAUD Oct. 24, 2018, 5:09 p.m. UTC | #9
Bonjour Joseph,

Le Wed, 24 Oct 2018 16:20:50 +0000, Joseph Myers
<joseph@codesourcery.com> a écrit :

> On Wed, 24 Oct 2018, Albert ARIBAUD wrote:
> 
> > And one last question: should I not first commit the test (and with make
> > check on a 32-bit architecture, show that it adds one FAIL) then commit
> > the fix (and show with the same make check that the FAIL disappears)?  
> 
> No, you should verify the FAIL with the fix not present, but the test is 
> best committed at the same time as a fix, to preserve bisectability.

Understood. I have already verified the FAIL once the test is in but
not the fix, using i686-linux-gnu.

Cordialement,
Albert ARIBAUD
3ADEV
  
Paul Eggert Oct. 25, 2018, 8:20 p.m. UTC | #10
On 10/24/18 4:23 AM, Albert ARIBAUD wrote:
>
> In the case where neither _LIBC nor NEED_MKTIME_WORKING are defined,
> "another" mktime() is called. Do we assume this other mktime will set
> errno to EOVERFLOW, or do we do it too in that case?

I would, yes. If the other mktime is broken Gnulib can deal with it in a 
different way; you don't need to worry about it.

>
> Also, I assume tests would be needed for both cases where
> __mktime_internal() returns -1? If so, I would need struct tm values
> which trigger either one, or at least indications on how to find ones.

You want outlandish values, where (for example) tm_year is INT_MIN or 
INT_MAX. On typical 64-bit platforms there are no outlandish values, as 
EOVERFLOW isn't possible when int is 32 bits and time_t is 64 bits.
  
Paul Eggert Oct. 25, 2018, 8:22 p.m. UTC | #11
On 10/24/18 8:59 AM, Albert ARIBAUD wrote:
>
> BTW: Paul, your program returns 2 when errno is not EOVERFLOW, but 2 is
> not supported as an EXIT_* value, only 0 (EXIT_SUCCESS), 1
> (EXIT_FAILURE) and 77 (EXIT_UNSUPPORTED). I assume that was an ad hoc
> choice to distinguish the "errno is not EOVERFLOW" failure from a
> hypothetical "mktime did not return -1" failure.

Yes, that's right. The test was originally written without the test 
framework in mind.
  

Patch

diff --git a/include/time.h b/include/time.h
index 7b435c00c2..76d5707170 100644
--- a/include/time.h
+++ b/include/time.h
@@ -3,6 +3,7 @@ 
 
 #ifndef _ISOMAC
 # include <bits/types/locale_t.h>
+# include <stdbool.h>
 
 extern __typeof (strftime_l) __strftime_l;
 libc_hidden_proto (__strftime_l)
@@ -16,6 +17,18 @@  libc_hidden_proto (localtime)
 libc_hidden_proto (strftime)
 libc_hidden_proto (strptime)
 
+#if __TIMESIZE == 64
+# define __timegm64 timegm
+# define __mktime64 mktime
+# define __timelocal64 timelocal
+#else
+extern __time64_t __timegm64 (struct tm *__tp) __THROW;
+extern __time64_t __mktime64 (struct tm *__tp) __THROW;
+/* Another name for `__mktime64'.  */
+extern __time64_t __timelocal64 (struct tm *__tp) __THROW;
+#endif
+
+
 extern __typeof (clock_getres) __clock_getres;
 extern __typeof (clock_gettime) __clock_gettime;
 libc_hidden_proto (__clock_gettime)
@@ -49,13 +62,13 @@  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;
 
-/* Subroutine of `mktime'.  Return the `time_t' representation of TP and
-   normalize TP, given that a `struct tm *' maps to a `time_t' as performed
+/* Subroutine of mktime.  Return the __time64_t representation of TP and
+   normalize TP, given that a struct tm * maps to a __time64_t as performed
    by FUNC.  Record next guess for localtime-gmtime offset in *OFFSET.  */
-extern time_t __mktime_internal (struct tm *__tp,
-				 struct tm *(*__func) (const time_t *,
-						       struct tm *),
-				 long int *__offset) attribute_hidden;
+extern __time64_t __mktime_internal (struct tm *__tp,
+				     struct tm *(*__func) (const __time64_t *,
+							   struct tm *),
+				     long int *__offset) attribute_hidden;
 
 /* nis/nis_print.c needs ctime, so even if ctime is not declared here,
    we define __ctime64 as ctime so that nis/nis_print.c can get linked
@@ -141,5 +154,13 @@  extern double __difftime (time_t time1, time_t time0);
    actual clock ID.  */
 #define CLOCK_IDFIELD_SIZE	3
 
+/* Check whether a time64_t value fits in a time_t.  */
+static inline bool
+fits_in_time_t (__time64_t t64)
+{
+  time_t t = t64;
+  return t == t64;
+}
+
 #endif
 #endif
diff --git a/time/mktime.c b/time/mktime.c
index 00f0dec6b4..533e05d6ab 100644
--- a/time/mktime.c
+++ b/time/mktime.c
@@ -51,6 +51,7 @@ 
 
 #include <time.h>
 
+#include <errno.h>
 #include <limits.h>
 #include <stdbool.h>
 #include <stdlib.h>
@@ -125,11 +126,11 @@  my_tzset (void)
    to be subtracted from each other, and sometimes with an offset
    added to them, without worrying about overflow.
 
-   Much of the code uses long_int to represent time_t values, to
-   lessen the hassle of dealing with platforms where time_t is
-   unsigned, and because long_int should suffice to represent all
-   time_t values that mktime can generate even on platforms where
-   time_t is excessively wide.  */
+   Much of the code uses long_int to represent __time64_t values, to
+   lessen the hassle of dealing with Gnulib-using platforms where
+   __time64_t is time_t and time_t is unsigned, and because long_int
+   should suffice to represent all __time64_t values that mktime can
+   generate even on platforms where __time64_t is excessively wide.  */
 
 #if INT_MAX <= LONG_MAX / 3 / 366 / 24 / 60 / 60
 typedef long int long_int;
@@ -157,16 +158,17 @@  shr (long_int a, int b)
 	  : a / (one << b) - (a % (one << b) < 0));
 }
 
-/* Bounds for the intersection of time_t and long_int.  */
+/* Bounds for the intersection of __time64_t and long_int.  */
 
 static long_int const mktime_min
-  = ((TYPE_SIGNED (time_t) && TYPE_MINIMUM (time_t) < TYPE_MINIMUM (long_int))
-     ? TYPE_MINIMUM (long_int) : TYPE_MINIMUM (time_t));
+  = ((TYPE_SIGNED (__time64_t)
+      && TYPE_MINIMUM (__time64_t) < TYPE_MINIMUM (long_int))
+     ? TYPE_MINIMUM (long_int) : TYPE_MINIMUM (__time64_t));
 static long_int const mktime_max
-  = (TYPE_MAXIMUM (long_int) < TYPE_MAXIMUM (time_t)
-     ? TYPE_MAXIMUM (long_int) : TYPE_MAXIMUM (time_t));
+  = (TYPE_MAXIMUM (long_int) < TYPE_MAXIMUM (__time64_t)
+     ? TYPE_MAXIMUM (long_int) : TYPE_MAXIMUM (__time64_t));
 
-verify (TYPE_IS_INTEGER (time_t));
+verify (TYPE_IS_INTEGER (__time64_t));
 
 #define EPOCH_YEAR 1970
 #define TM_YEAR_BASE 1900
@@ -247,11 +249,11 @@  long_int_avg (long_int a, long_int b)
   return shr (a, 1) + shr (b, 1) + ((a | b) & 1);
 }
 
-/* Return a time_t value corresponding to (YEAR-YDAY HOUR:MIN:SEC),
+/* Return a __time64_t value corresponding to (YEAR-YDAY HOUR:MIN:SEC),
    assuming that T corresponds to *TP and that no clock adjustments
    occurred between *TP and the desired time.
    Although T and the returned value are of type long_int,
-   they represent time_t values and must be in time_t range.
+   they represent __time64_t values and must be in __time64_t range.
    If TP is null, return a value not equal to T; this avoids false matches.
    YEAR and YDAY must not be so large that multiplying them by three times the
    number of seconds in a year (or day, respectively) would overflow long_int.
@@ -282,22 +284,22 @@  guess_time_tm (long_int year, long_int yday, int hour, int min, int sec,
 }
 
 /* Use CONVERT to convert T to a struct tm value in *TM.  T must be in
-   range for time_t.  Return TM if successful, NULL if T is out of
+   range for __time64_t.  Return TM if successful, NULL if T is out of
    range for CONVERT.  */
 static struct tm *
-convert_time (struct tm *(*convert) (const time_t *, struct tm *),
+convert_time (struct tm *(*convert) (const __time64_t *, struct tm *),
 	      long_int t, struct tm *tm)
 {
-  time_t x = t;
+  __time64_t x = t;
   return convert (&x, tm);
 }
 
 /* Use CONVERT to convert *T to a broken down time in *TP.
    If *T is out of range for conversion, adjust it so that
    it is the nearest in-range value and then convert that.
-   A value is in range if it fits in both time_t and long_int.  */
+   A value is in range if it fits in both __time64_t and long_int.  */
 static struct tm *
-ranged_convert (struct tm *(*convert) (const time_t *, struct tm *),
+ranged_convert (struct tm *(*convert) (const __time64_t *, struct tm *),
 		long_int *t, struct tm *tp)
 {
   struct tm *r;
@@ -339,15 +341,15 @@  ranged_convert (struct tm *(*convert) (const time_t *, struct tm *),
 }
 
 
-/* Convert *TP to a time_t value, inverting
+/* Convert *TP to a __time64_t value, inverting
    the monotonic and mostly-unit-linear conversion function CONVERT.
    Use *OFFSET to keep track of a guess at the offset of the result,
    compared to what the result would be for UTC without leap seconds.
    If *OFFSET's guess is correct, only one CONVERT call is needed.
    This function is external because it is used also by timegm.c.  */
-time_t
+__time64_t
 __mktime_internal (struct tm *tp,
-		   struct tm *(*convert) (const time_t *, struct tm *),
+		   struct tm *(*convert) (const __time64_t *, struct tm *),
 		   mktime_offset_t *offset)
 {
   long_int t, gt, t0, t1, t2, dt;
@@ -518,9 +520,9 @@  __mktime_internal (struct tm *tp,
 
 #if defined _LIBC || NEED_MKTIME_WORKING || NEED_MKTIME_WINDOWS
 
-/* Convert *TP to a time_t value.  */
-time_t
-mktime (struct tm *tp)
+/* Convert *TP to a __time64_t value.  */
+__time64_t
+__mktime64 (struct tm *tp)
 {
   /* POSIX.1 8.1.1 requires that whenever mktime() is called, the
      time zone names contained in the external variable 'tzname' shall
@@ -529,7 +531,7 @@  mktime (struct tm *tp)
 
 # if defined _LIBC || NEED_MKTIME_WORKING
   static mktime_offset_t localtime_offset;
-  return __mktime_internal (tp, __localtime_r, &localtime_offset);
+  return __mktime_internal (tp, __localtime64_r, &localtime_offset);
 # else
 #  undef mktime
   return mktime (tp);
@@ -537,6 +539,28 @@  mktime (struct tm *tp)
 }
 #endif /* _LIBC || NEED_MKTIME_WORKING || NEED_MKTIME_WINDOWS */
 
+#ifdef weak_alias
+weak_alias (__mktime64, __timelocal64)
+#endif
+
+#ifdef _LIBC
+libc_hidden_def (__mktime64)
+libc_hidden_weak (__timelocal64)
+#endif
+
+#if __TIMESIZE != 64
+
+/* The 32-bit-time wrapper.  */
+time_t
+mktime (struct tm *tp)
+{
+  __time64_t t64 = __mktime64 (tp);
+  if (fits_in_time_t (t64))
+    return t64;
+  __set_errno (EOVERFLOW);
+  return -1;
+}
+
 #ifdef weak_alias
 weak_alias (mktime, timelocal)
 #endif
@@ -545,6 +569,8 @@  weak_alias (mktime, timelocal)
 libc_hidden_def (mktime)
 libc_hidden_weak (timelocal)
 #endif
+
+#endif
 
 #if DEBUG_MKTIME
 
diff --git a/time/timegm.c b/time/timegm.c
index 229fff23c6..e7718ee170 100644
--- a/time/timegm.c
+++ b/time/timegm.c
@@ -24,11 +24,26 @@ 
 #include <time.h>
 
 #include "mktime-internal.h"
+#include <errno.h>
+
+__time64_t
+__timegm64 (struct tm *tmp)
+{
+  static long int gmtime_offset;
+  tmp->tm_isdst = 0;
+  return __mktime_internal (tmp, __gmtime64_r, &gmtime_offset);
+}
+
+#if __TIMESIZE != 64
 
 time_t
 timegm (struct tm *tmp)
 {
-  static mktime_offset_t gmtime_offset;
-  tmp->tm_isdst = 0;
-  return __mktime_internal (tmp, __gmtime_r, &gmtime_offset);
+  __time64_t t64 = __timegm64 (tmp);
+  if (fits_in_time_t (t64))
+    return t64;
+  __set_errno (EOVERFLOW);
+  return -1;
 }
+
+#endif