Use 64-bit time_t for time zone file parsing

This patch is intended as a precursor for time64_t support.  Some 
further conversions of internal interfaces will be required.  It also 
enables future cleanups in the parser itself.

It does not change much, but I believe it is a small step in the right 
direction.

Thanks,
Florian

On Mai 08 2018, Florian Weimer <fweimer@redhat.com> wrote:

> +  if ((BYTE_ORDER != BIG_ENDIAN && trans_width == 4)
> +      || (BYTE_ORDER == BIG_ENDIAN && trans_width == 4))

Redundant condition.

Andreas.

On 05/08/2018 04:05 PM, Andreas Schwab wrote:
> On Mai 08 2018, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> +  if ((BYTE_ORDER != BIG_ENDIAN && trans_width == 4)
>> +      || (BYTE_ORDER == BIG_ENDIAN && trans_width == 4))
> 
> Redundant condition.

Ah.  I'm dropping the check of BYTE_ORDER.

Thanks,
Florian
Subject: [PATCH] time: Use 64-bit time values for time zone parsing
To: libc-alpha@sourceware.org

2018-05-08  Florian Weimer  <fweimer@redhat.com>

	Use 64-bit epoch values in the time zone file parser.
	* include/time.h (internal_time_t): Define.
	(__tzfile_compute): Use it.
	* time/tzfile.c (struct leap): Use internal_time_t for epoch
	member.
	(transitions): Switch to internal_time_t.
	(__tzfile_read): Likewise.  Remove code dealing with 4-byte time_t
	types.
	(__tzfile_compute): Use internal_time_t for timer argument.  Check
	for truncation before calling __offtime.

diff --git a/include/time.h b/include/time.h
index aab26d7768..23d2580528 100644
--- a/include/time.h
+++ b/include/time.h
@@ -26,6 +26,10 @@ extern __typeof (clock_getcpuclockid) __clock_getcpuclockid;
 /* Now define the internal interfaces.  */
 struct tm;
 
+/* time_t variant for representing time zone data, independent of
+   time_t.  */
+typedef __int64_t internal_time_t;
+
 /* Defined in mktime.c.  */
 extern const unsigned short int __mon_yday[2][13] attribute_hidden;
 
@@ -39,7 +43,7 @@ extern int __use_tzfile attribute_hidden;
 
 extern void __tzfile_read (const char *file, size_t extra,
 			   char **extrap) attribute_hidden;
-extern void __tzfile_compute (time_t timer, int use_localtime,
+extern void __tzfile_compute (internal_time_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,
diff --git a/time/tzfile.c b/time/tzfile.c
index 3e39723148..0ffc1a224a 100644
--- a/time/tzfile.c
+++ b/time/tzfile.c
@@ -44,12 +44,12 @@ struct ttinfo
 
 struct leap
   {
-    time_t transition;		/* Time the transition takes effect.  */
+    internal_time_t transition;	/* Time the transition takes effect.  */
     long int change;		/* Seconds of correction to apply.  */
   };
 
 static size_t num_transitions;
-libc_freeres_ptr (static time_t *transitions);
+libc_freeres_ptr (static internal_time_t *transitions);
 static unsigned char *type_idxs;
 static size_t num_types;
 static struct ttinfo *types;
@@ -113,8 +113,8 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
   size_t tzspec_len;
   char *new = NULL;
 
-  if (sizeof (time_t) != 4 && sizeof (time_t) != 8)
-    abort ();
+  _Static_assert (sizeof (internal_time_t) == 8,
+		  "internal_time_t must be eight bytes");
 
   __use_tzfile = 0;
 
@@ -200,9 +200,7 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
   if (__glibc_unlikely (num_isstd > num_types || num_isgmt > num_types))
     goto lose;
 
-  /* For platforms with 64-bit time_t we use the new format if available.  */
-  if (sizeof (time_t) == 8 && trans_width == 4
-      && tzhead.tzh_version[0] != '\0')
+  if (trans_width == 4 && tzhead.tzh_version[0] != '\0')
     {
       /* We use the 8-byte format.  */
       trans_width = 8;
@@ -222,9 +220,9 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
 
   if (__builtin_expect (num_transitions
 			> ((SIZE_MAX - (__alignof__ (struct ttinfo) - 1))
-			   / (sizeof (time_t) + 1)), 0))
+			   / (sizeof (internal_time_t) + 1)), 0))
     goto lose;
-  total_size = num_transitions * (sizeof (time_t) + 1);
+  total_size = num_transitions * (sizeof (internal_time_t) + 1);
   total_size = ((total_size + __alignof__ (struct ttinfo) - 1)
 		& ~(__alignof__ (struct ttinfo) - 1));
   types_idx = total_size;
@@ -246,7 +244,7 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
     goto lose;
   total_size += num_leaps * sizeof (struct leap);
   tzspec_len = 0;
-  if (sizeof (time_t) == 8 && trans_width == 8)
+  if (trans_width == 8)
     {
       off_t rem = st.st_size - __ftello (f);
       if (__builtin_expect (rem < 0
@@ -276,23 +274,23 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
 
   /* Allocate enough memory including the extra block requested by the
      caller.  */
-  transitions = (time_t *) malloc (total_size + tzspec_len + extra);
+  transitions = malloc (total_size + tzspec_len + extra);
   if (transitions == NULL)
     goto lose;
 
   type_idxs = (unsigned char *) transitions + (num_transitions
-					       * sizeof (time_t));
+					       * sizeof (internal_time_t));
   types = (struct ttinfo *) ((char *) transitions + types_idx);
   zone_names = (char *) types + num_types * sizeof (struct ttinfo);
   leaps = (struct leap *) ((char *) transitions + leaps_idx);
-  if (sizeof (time_t) == 8 && trans_width == 8)
+  if (trans_width == 8)
     tzspec = (char *) leaps + num_leaps * sizeof (struct leap) + extra;
   else
     tzspec = NULL;
   if (extra > 0)
     *extrap = (char *) &leaps[num_leaps];
 
-  if (sizeof (time_t) == 4 || __builtin_expect (trans_width == 8, 1))
+  if (__builtin_expect (trans_width == 8, 1))
     {
       if (__builtin_expect (__fread_unlocked (transitions, trans_width + 1,
 					      num_transitions, f)
@@ -315,19 +313,17 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
     if (__glibc_unlikely (type_idxs[i] >= num_types))
       goto lose;
 
-  if ((BYTE_ORDER != BIG_ENDIAN && (sizeof (time_t) == 4 || trans_width == 4))
-      || (BYTE_ORDER == BIG_ENDIAN && sizeof (time_t) == 8
-	  && trans_width == 4))
+  if (trans_width == 4)
     {
       /* Decode the transition times, stored as 4-byte integers in
-	 network (big-endian) byte order.  We work from the end of
-	 the array so as not to clobber the next element to be
-	 processed when sizeof (time_t) > 4.  */
+	 network (big-endian) byte order.  We work from the end of the
+	 array so as not to clobber the next element to be
+	 processed.  */
       i = num_transitions;
       while (i-- > 0)
 	transitions[i] = decode ((char *) transitions + i * 4);
     }
-  else if (BYTE_ORDER != BIG_ENDIAN && sizeof (time_t) == 8)
+  else if (BYTE_ORDER != BIG_ENDIAN)
     {
       /* Decode the transition times, stored as 8-byte integers in
 	 network (big-endian) byte order.  */
@@ -364,10 +360,10 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
       if (__builtin_expect (__fread_unlocked (x, 1, trans_width, f)
 			    != trans_width, 0))
 	goto lose;
-      if (sizeof (time_t) == 4 || trans_width == 4)
-	leaps[i].transition = (time_t) decode (x);
+      if (trans_width == 4)
+	leaps[i].transition = decode (x);
       else
-	leaps[i].transition = (time_t) decode64 (x);
+	leaps[i].transition = decode64 (x);
 
       if (__glibc_unlikely (__fread_unlocked (x, 1, 4, f) != 4))
 	goto lose;
@@ -395,7 +391,7 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
     types[i++].isgmt = 0;
 
   /* Read the POSIX TZ-style information if possible.  */
-  if (sizeof (time_t) == 8 && tzspec != NULL)
+  if (tzspec != NULL)
     {
       /* Skip over the newline first.  */
       if (__getc_unlocked (f) != '\n'
@@ -405,52 +401,6 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
       else
 	tzspec[tzspec_len - 1] = '\0';
     }
-  else if (sizeof (time_t) == 4 && tzhead.tzh_version[0] != '\0')
-    {
-      /* Get the TZ string.  */
-      if (__builtin_expect (__fread_unlocked ((void *) &tzhead,
-					      sizeof (tzhead), 1, f) != 1, 0)
-	  || (memcmp (tzhead.tzh_magic, TZ_MAGIC, sizeof (tzhead.tzh_magic))
-	      != 0))
-	goto lose;
-
-      size_t num_transitions2 = (size_t) decode (tzhead.tzh_timecnt);
-      size_t num_types2 = (size_t) decode (tzhead.tzh_typecnt);
-      size_t chars2 = (size_t) decode (tzhead.tzh_charcnt);
-      size_t num_leaps2 = (size_t) decode (tzhead.tzh_leapcnt);
-      size_t num_isstd2 = (size_t) decode (tzhead.tzh_ttisstdcnt);
-      size_t num_isgmt2 = (size_t) decode (tzhead.tzh_ttisgmtcnt);
-
-      /* Position the stream before the second header.  */
-      size_t to_skip = (num_transitions2 * (8 + 1)
-			+ num_types2 * 6
-			+ chars2
-			+ num_leaps2 * 12
-			+ num_isstd2
-			+ num_isgmt2);
-      off_t off;
-      if (fseek (f, to_skip, SEEK_CUR) != 0
-	  || (off = __ftello (f)) < 0
-	  || st.st_size < off + 2)
-	goto lose;
-
-      tzspec_len = st.st_size - off - 1;
-      if (tzspec_len == 0)
-	goto lose;
-      char *tzstr = malloc (tzspec_len);
-      if (tzstr == NULL)
-	goto lose;
-      if (__getc_unlocked (f) != '\n'
-	  || (__fread_unlocked (tzstr, 1, tzspec_len - 1, f)
-	      != tzspec_len - 1))
-	{
-	  free (tzstr);
-	  goto lose;
-	}
-      tzstr[tzspec_len - 1] = '\0';
-      tzspec = __tzstring (tzstr);
-      free (tzstr);
-    }
 
   /* Don't use an empty TZ string.  */
   if (tzspec != NULL && tzspec[0] == '\0')
@@ -630,7 +580,7 @@ __tzfile_default (const char *std, const char *dst,
 }
 
 void
-__tzfile_compute (time_t timer, int use_localtime,
+__tzfile_compute (internal_time_t timer, int use_localtime,
 		  long int *leap_correct, int *leap_hit,
 		  struct tm *tp)
 {
@@ -685,10 +635,16 @@ __tzfile_compute (time_t timer, int use_localtime,
 
 	  /* Convert to broken down structure.  If this fails do not
 	     use the string.  */
-	  if (__glibc_unlikely (! __offtime (&timer, 0, tp)))
-	    goto use_last;
+	  {
+	    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.  */
+	  /* Use the rules from the TZ string to compute the change.
+	     timer fits into time_t due to the truncation check
+	     above.  */
 	  __tz_compute (timer, tp, 1);
 
 	  /* If tzspec comes from posixrules loaded by __tzfile_default,

On 05/08/2018 07:29 AM, Florian Weimer wrote:
> Ah. I'm dropping the check of BYTE_ORDER. 

Thanks, this looks good to me. One minor suggestion. This line:

       i = (transitions[num_transitions - 1] - timer) / 15778476;

is now trickier than it used to be. Although the code still works since 
it's just computing a guess, this new behavior might be worth a comment, 
something along the lines "If size_t is 32 bits the assignment to I 
could in theory overflow and wrap around, but this is harmless."

On 05/10/2018 04:04 AM, Paul Eggert wrote:
> On 05/08/2018 07:29 AM, Florian Weimer wrote:
>> Ah. I'm dropping the check of BYTE_ORDER. 
> 
> Thanks, this looks good to me. One minor suggestion. This line:
> 
>        i = (transitions[num_transitions - 1] - timer) / 15778476;
> 
> is now trickier than it used to be. Although the code still works since 
> it's just computing a guess, this new behavior might be worth a comment, 
> something along the lines "If size_t is 32 bits the assignment to I 
> could in theory overflow and wrap around, but this is harmless."

Thanks, I added a comment and pushed thechange.

Florian

Does this use of 64-bit time_t fix bug 19738 ("__tzfile_default 32-bit 
time_t overflow")?  If it does, we should add a testcase for that bug then 
resolve it as FIXED in 2.28.

On 05/17/2018 11:06 PM, Joseph Myers wrote:
> Does this use of 64-bit time_t fix bug 19738 ("__tzfile_default 32-bit
> time_t overflow")?  If it does, we should add a testcase for that bug then
> resolve it as FIXED in 2.28.

Looks like it.  Here's a test case which fails before on i386 (assuming 
that the system posixrules file as the required contents) and passes 
afterwards.

I would call the bug fixed because dates where the overflow occurs with 
a 64-bit (internal) time_t are after the expected life-time of the sun, 
where daylight savings time is certainly meaningless.

Thanks,
Florian
Subject: [PATCH] time: Add test case for bug 19738
To: libc-alpha@sourceware.org

2018-05-18  Florian Weimer  <fweimer@redhat.com>

	[BZ #19738]
	* time/tst-posixrules-overflow.c (do_test): New file.
	* time/Makefile (tests): Add it.

diff --git a/time/Makefile b/time/Makefile
index 0db1206820..5454fe37e3 100644
--- a/time/Makefile
+++ b/time/Makefile
@@ -43,7 +43,7 @@ tests	:= test_time clocktest tst-posixtz tst-strptime tst_wcsftime \
 	   tst-getdate tst-mktime tst-mktime2 tst-ftime_l tst-strftime \
 	   tst-mktime3 tst-strptime2 bug-asctime bug-asctime_r bug-mktime1 \
 	   tst-strptime3 bug-getdate1 tst-strptime-whitespace tst-ftime \
-	   tst-tzname
+	   tst-tzname tst-posixrules-overflow
 
 include ../Rules
 
diff --git a/time/tst-posixrules-overflow.c b/time/tst-posixrules-overflow.c
new file mode 100644
index 0000000000..13d0789a93
--- /dev/null
+++ b/time/tst-posixrules-overflow.c
@@ -0,0 +1,43 @@
+/* Check processing of posixrules/TZDEFRULES (bug 19738).
+   Copyright (C) 2018 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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+#include <support/check.h>
+#include <time.h>
+
+static int
+do_test (void)
+{
+  TEST_COMPARE (setenv ("TZ", "STD+8DST", 1), 0);
+  tzset ();
+  time_t epoch = 1467414184;
+  struct tm *tm = localtime (&epoch);
+  TEST_VERIFY_EXIT (tm != NULL);
+  TEST_COMPARE (tm->tm_year, 2016 - 1900);
+  TEST_COMPARE (tm->tm_mon, 7 - 1);
+  TEST_COMPARE (tm->tm_mday, 1);
+  TEST_COMPARE (tm->tm_hour, 16);
+  TEST_COMPARE (tm->tm_min, 3);
+  TEST_COMPARE (tm->tm_sec, 4);
+  TEST_COMPARE (tm->tm_wday, 5);
+  TEST_COMPARE (tm->tm_yday, 183 - 1);
+  TEST_VERIFY (tm->tm_isdst);
+  return 0;
+}
+
+#include <support/test-driver.c>