time: Use struct alloc_buffer in __tzfile_read

The computation of tzspec_len is moved in front of the total_size
computation, so that the allocation size computation and the
suballocations are next to each other.  Also add an assert that
tzspec_len is positive when it is actually used later.

2 files changed, 55 insertions, 49 deletions

diff --git a/ChangeLog b/ChangeLog
index 82604298de..bc1b17ffa7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2019-02-03  Florian Weimer  <fweimer@redhat.com>
+
+	* time/tzfile.c (__tzfile_read): Use struct alloc_buffer and its
+	implicit overflow checks.
+
 2019-02-03  Aurelien Jarno  <aurelien@aurel32.net>
 
 	* stdlib/isomac.c: Include <unistd.h>.
diff --git a/time/tzfile.c b/time/tzfile.c
index a07e7c5037..e107b33a82 100644
--- a/time/tzfile.c
+++ b/time/tzfile.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <sys/stat.h>
 #include <stdint.h>
+#include <alloc_buffer.h>
 
 #include <timezone/tzfile.h>
 
@@ -105,12 +106,8 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
   struct tzhead tzhead;
   size_t chars;
   size_t i;
-  size_t total_size;
-  size_t types_idx;
-  size_t leaps_idx;
   int was_using_tzfile = __use_tzfile;
   int trans_width = 4;
-  size_t tzspec_len;
   char *new = NULL;
 
   _Static_assert (sizeof (__time64_t) == 8,
@@ -215,32 +212,9 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
       goto read_again;
     }
 
-  if (__builtin_expect (num_transitions
-			> ((SIZE_MAX - (__alignof__ (struct ttinfo) - 1))
-			   / (sizeof (__time64_t) + 1)), 0))
-    goto lose;
-  total_size = num_transitions * (sizeof (__time64_t) + 1);
-  total_size = ((total_size + __alignof__ (struct ttinfo) - 1)
-		& ~(__alignof__ (struct ttinfo) - 1));
-  types_idx = total_size;
-  if (__builtin_expect (num_types
-			> (SIZE_MAX - total_size) / sizeof (struct ttinfo), 0))
-    goto lose;
-  total_size += num_types * sizeof (struct ttinfo);
-  if (__glibc_unlikely (chars > SIZE_MAX - total_size))
-    goto lose;
-  total_size += chars;
-  if (__builtin_expect (__alignof__ (struct leap) - 1
-			> SIZE_MAX - total_size, 0))
-    goto lose;
-  total_size = ((total_size + __alignof__ (struct leap) - 1)
-		& ~(__alignof__ (struct leap) - 1));
-  leaps_idx = total_size;
-  if (__builtin_expect (num_leaps
-			> (SIZE_MAX - total_size) / sizeof (struct leap), 0))
-    goto lose;
-  total_size += num_leaps * sizeof (struct leap);
-  tzspec_len = 0;
+  /* Compute the size of the POSIX time zone specification in the
+     file.  */
+  size_t tzspec_len;
   if (trans_width == 8)
     {
       off_t rem = st.st_size - __ftello (f);
@@ -262,30 +236,56 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
       if (__glibc_unlikely (tzspec_len == 0 || tzspec_len - 1 < num_isgmt))
 	goto lose;
       tzspec_len -= num_isgmt + 1;
-      if (__glibc_unlikely (tzspec_len == 0
-			    || SIZE_MAX - total_size < tzspec_len))
+      if (tzspec_len == 0)
 	goto lose;
     }
-  if (__glibc_unlikely (SIZE_MAX - total_size - tzspec_len < extra))
-    goto lose;
-
-  /* Allocate enough memory including the extra block requested by the
-     caller.  */
-  transitions = malloc (total_size + tzspec_len + extra);
-  if (transitions == NULL)
-    goto lose;
-
-  type_idxs = (unsigned char *) transitions + (num_transitions
-					       * sizeof (__time64_t));
-  types = (struct ttinfo *) ((char *) transitions + types_idx);
-  zone_names = (char *) types + num_types * sizeof (struct ttinfo);
-  leaps = (struct leap *) ((char *) transitions + leaps_idx);
+  else
+    tzspec_len = 0;
+
+  /* The file is parsed into a single heap allocation, comprising of
+     the following arrays:
+
+     __time64_t transitions[num_transitions];
+     struct ttinfo types[num_types];
+     unsigned char type_idxs[num_types];
+     char zone_names[chars];
+     struct leap leaps[num_leaps];
+     char extra_array[extra]; // Stored into *pextras if requested.
+     char tzspec[tzspec_len];
+
+     The piece-wise allocations from buf below verify that no
+     overflow/wraparound occurred in these computations.  */
+  struct alloc_buffer buf;
+  {
+    size_t total_size = num_transitions * (sizeof (__time64_t) + 1);
+    total_size = ((total_size + __alignof__ (struct ttinfo) - 1)
+		  & ~(__alignof__ (struct ttinfo) - 1));
+    total_size += num_types * sizeof (struct ttinfo) + chars;
+    total_size = ((total_size + __alignof__ (struct leap) - 1)
+		  & ~(__alignof__ (struct leap) - 1));
+    total_size += num_leaps * sizeof (struct leap) + tzspec_len + extra;
+
+    transitions = malloc (total_size);
+    if (transitions == NULL)
+      goto lose;
+    buf = alloc_buffer_create (transitions, total_size);
+  }
+
+  /* The address of the first allocation is already stored in the
+     pointer transitions.  */
+  (void) alloc_buffer_alloc_array (&buf, __time64_t, num_transitions);
+  type_idxs = alloc_buffer_alloc_array (&buf, unsigned char, num_transitions);
+  types = alloc_buffer_alloc_array (&buf, struct ttinfo, num_types);
+  zone_names = alloc_buffer_alloc_array (&buf, char, chars);
+  leaps = alloc_buffer_alloc_array (&buf, struct leap, num_leaps);
+  if (extra > 0)
+    *extrap = alloc_buffer_alloc_array (&buf, char, extra);
   if (trans_width == 8)
-    tzspec = (char *) leaps + num_leaps * sizeof (struct leap) + extra;
+    tzspec = alloc_buffer_alloc_array (&buf, char, tzspec_len);
   else
     tzspec = NULL;
-  if (extra > 0)
-    *extrap = (char *) &leaps[num_leaps];
+  if (alloc_buffer_has_failed (&buf))
+    goto lose;
 
   if (__builtin_expect (trans_width == 8, 1))
     {
@@ -390,6 +390,7 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
   /* Read the POSIX TZ-style information if possible.  */
   if (tzspec != NULL)
     {
+      assert (tzspec_len > 0);
       /* Skip over the newline first.  */
       if (__getc_unlocked (f) != '\n'
 	  || (__fread_unlocked (tzspec, 1, tzspec_len - 1, f)