This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[PATCH] time: Use struct alloc_buffer in __tzfile_read
- From: Florian Weimer <fweimer at redhat dot com>
- To: libc-alpha at sourceware dot org
- Date: Sat, 02 Feb 2019 23:17:56 +0100
- Subject: [PATCH] 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.
diff --git a/time/tzfile.c b/time/tzfile.c
index 24440a6a9c..72adfbc68e 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,59 @@ __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);
+ }
+
+ 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;
+ 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 +393,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)