time: Use struct alloc_buffer in __tzfile_read

This simplification makes it easier to rearrange the allocations to
avoid alignment gaps in a future gap.

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.

2019-02-02  Florian Weimer  <fweimer@redhat.com>

	* time/tzfile.c (__tzfile_read): Use struct alloc_buffer and its
	implicit overflow checks.

Florian Weimer wrote:
> +  if (alloc_buffer_alloc_array (&buf, __time64_t, num_transitions)
> +      != transitions)
> +    /* Either the start of the allocation moved unexpectedly
> +       (misaligned heap pointer, should not happen), or we had an
> +       overflow.  */
> +    goto lose;

Is this comparison needed? The overflow will be caught later anyway, and the != 
comparison cannot fail.

Otherwise it looks good; thanks.

* Paul Eggert:

> Florian Weimer wrote:
>> +  if (alloc_buffer_alloc_array (&buf, __time64_t, num_transitions)
>> +      != transitions)
>> +    /* Either the start of the allocation moved unexpectedly
>> +       (misaligned heap pointer, should not happen), or we had an
>> +       overflow.  */
>> +    goto lose;
>
> Is this comparison needed? The overflow will be caught later anyway,
> and the != comparison cannot fail.
>
> Otherwise it looks good; thanks.

Okay, I will remove that.  Even if the allocation moves the pointer
(after removal of the alignment code), we will not cause heap corruption
because the deallocation will use the original (non-moved) pointer.

Thanks,
Florian

* Florian Weimer:

> * Paul Eggert:
>
>> Florian Weimer wrote:
>>> +  if (alloc_buffer_alloc_array (&buf, __time64_t, num_transitions)
>>> +      != transitions)
>>> +    /* Either the start of the allocation moved unexpectedly
>>> +       (misaligned heap pointer, should not happen), or we had an
>>> +       overflow.  */
>>> +    goto lose;
>>
>> Is this comparison needed? The overflow will be caught later anyway,
>> and the != comparison cannot fail.
>>
>> Otherwise it looks good; thanks.
>
> Okay, I will remove that.  Even if the allocation moves the pointer
> (after removal of the alignment code), we will not cause heap corruption
> because the deallocation will use the original (non-moved) pointer.

Updated patch below.

Thanks,
Florian

time: Use struct alloc_buffer in __tzfile_read

This simplification makes it easier to rearrange the allocations to
avoid alignment gaps in a future gap.

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.

2019-02-03  Florian Weimer  <fweimer@redhat.com>

	* time/tzfile.c (__tzfile_read): Use struct alloc_buffer and its
	implicit overflow checks.

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);
-  if (trans_width == 8)
-    tzspec = (char *) leaps + num_leaps * sizeof (struct leap) + extra;
   else
-    tzspec = NULL;
+    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 = (char *) &leaps[num_leaps];
+    *extrap = alloc_buffer_alloc_array (&buf, char, extra);
+  if (trans_width == 8)
+    tzspec = alloc_buffer_alloc_array (&buf, char, tzspec_len);
+  else
+    tzspec = NULL;
+  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)

Thanks, looks good.