On 11/11/20 8:20 AM, Bruno Haible wrote:
It works fine on Alpine Linux 3.7 (32-bit, 64-bit) and 3.9 (64-bit).
On Alpine Linux 3.10 and 3.12 (64-bit) it fails:
../../gltests/test-parse-datetime.c:448: assertion 'result.tv_sec == 1 * 60 * 60 + 2 *
60 + 3 && result.tv_nsec == 123456789' failed
Aborted
So, to me it looks like a regression between Alpine Linux 3.9 and 3.10.
It's arguably a bug in the test case, since Alpine uses musl libc which does not
support time zone abbreviations longer than 6 bytes, whereas the test case uses
an time zone abbreviation of 2000 bytes (to test a bug in an old Gnulib version
when running on GNU/Linux). POSIX does not define behavior if you go over the limit.
I worked around the problem by changing the test case to not go over the limit
as determined by sysconf (_SC_TZNAME_MAX), in the first attached patch. Plus I
refactored and/or slightly improved the Gnulib overflow checking while I was in
the neighborhood (last two attached patches).
Arguably this is a quality-of-implementation issue here, since Alpine and/or
musl goes beserk with long timezone abbreviations whereas every other
implementation I know of either works or silently substitutes localtime or UTC
(which is good enough for this test case). But I'll leave that issue to the
Alpine and/or musl libc folks.
I'll cc this to the musl bug reporting list. Although the Gnulib test failure
has been fixed, it may be the symptom of a more-severe bug in musl. For those
new to the problem, this thread starts here:
https://lists.gnu.org/r/bug-gnulib/2020-11/msg00039.html
>From 4c9a3c65e279977af4e345748ba73ab0441dc04a Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Wed, 11 Nov 2020 19:08:27 -0800
Subject: [PATCH 1/3] parse-datetime-tests: port to Alpine Linux 3.12.1
* tests/test-parse-datetime.c: Include errno.h for errno,
and unistd.h for _SC_TZNAME_MAX and sysconf.
(main): In the outlandishly-long time zone abbreviation test,
do not exceed TZNAME_MAX as this has undefined behavior,
and on Alpine Linux 3.12.1 it makes the test fail.
---
ChangeLog | 9 +++++++++
tests/test-parse-datetime.c | 16 +++++++++++++---
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index a5999557b..e1828df64 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2020-11-11 Paul Eggert <egg...@cs.ucla.edu>
+
+ parse-datetime-tests: port to Alpine Linux 3.12.1
+ * tests/test-parse-datetime.c: Include errno.h for errno,
+ and unistd.h for _SC_TZNAME_MAX and sysconf.
+ (main): In the outlandishly-long time zone abbreviation test,
+ do not exceed TZNAME_MAX as this has undefined behavior,
+ and on Alpine Linux 3.12.1 it makes the test fail.
+
2020-11-09 Pádraig Brady <p...@draigbrady.com>
mgetgroups: avoid warning with clang
diff --git a/tests/test-parse-datetime.c b/tests/test-parse-datetime.c
index 920c9ae84..187e7c703 100644
--- a/tests/test-parse-datetime.c
+++ b/tests/test-parse-datetime.c
@@ -20,9 +20,11 @@
#include "parse-datetime.h"
+#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <unistd.h>
#include "macros.h"
@@ -435,13 +437,21 @@ main (int argc _GL_UNUSED, char **argv)
/* Outlandishly-long time zone abbreviations should not cause problems. */
{
static char const bufprefix[] = "TZ=\"";
- enum { tzname_len = 2000 };
+ long int tzname_max = -1;
+ errno = 0;
+#ifdef _SC_TZNAME_MAX
+ tzname_max = sysconf (_SC_TZNAME_MAX);
+#endif
+ enum { tzname_alloc = 2000 };
+ if (tzname_max < 0)
+ tzname_max = errno ? 6 : tzname_alloc;
+ int tzname_len = tzname_alloc < tzname_max ? tzname_alloc : tzname_max;
static char const bufsuffix[] = "0\" 1970-01-01 01:02:03.123456789";
- enum { bufsize = sizeof bufprefix - 1 + tzname_len + sizeof bufsuffix };
+ enum { bufsize = sizeof bufprefix - 1 + tzname_alloc + sizeof bufsuffix };
char buf[bufsize];
memcpy (buf, bufprefix, sizeof bufprefix - 1);
memset (buf + sizeof bufprefix - 1, 'X', tzname_len);
- strcpy (buf + bufsize - sizeof bufsuffix, bufsuffix);
+ strcpy (buf + sizeof bufprefix - 1 + tzname_len, bufsuffix);
ASSERT (parse_datetime (&result, buf, &now));
LOG (buf, now, result);
ASSERT (result.tv_sec == 1 * 60 * 60 + 2 * 60 + 3
--
2.25.1
>From 00ffb79c529942eab5c81568808bd317c753213a Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Wed, 11 Nov 2020 19:16:23 -0800
Subject: [PATCH 2/3] parse-datetime: streamline overflow checking
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
When parse-datetime.y’s overflow code was written, INT_ADD_WRAPV
did not work for unsigned destinations, and since time_t might
be unsigned that meant it did not work for time_t destinations.
This limitation of INT_ADD_WRAPV has been fixed, so we can
now streamline parse-datetime.y a bit.
* lib/parse-datetime.y: Do not include limits.h, as LONG_MAX
has not been used for a while.
(yylex, parse_datetime2): Assume C99 declarations after statements.
(yyles): Use INT_SUBTRACT_WRAPV instead of an explicit comparison
to TYPE_MINIMUM.
(parse_datetime2): No need for time_overflow now that
INT_ADD_WRAPV works for unsigned results.
---
ChangeLog | 14 ++++++++++++++
lib/parse-datetime.y | 41 ++++++++++++++++-------------------------
2 files changed, 30 insertions(+), 25 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index e1828df64..530c9a661 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,19 @@
2020-11-11 Paul Eggert <egg...@cs.ucla.edu>
+ parse-datetime: streamline overflow checking
+ When parse-datetime.y’s overflow code was written, INT_ADD_WRAPV
+ did not work for unsigned destinations, and since time_t might
+ be unsigned that meant it did not work for time_t destinations.
+ This limitation of INT_ADD_WRAPV has been fixed, so we can
+ now streamline parse-datetime.y a bit.
+ * lib/parse-datetime.y: Do not include limits.h, as LONG_MAX
+ has not been used for a while.
+ (yylex, parse_datetime2): Assume C99 declarations after statements.
+ (yyles): Use INT_SUBTRACT_WRAPV instead of an explicit comparison
+ to TYPE_MINIMUM.
+ (parse_datetime2): No need for time_overflow now that
+ INT_ADD_WRAPV works for unsigned results.
+
parse-datetime-tests: port to Alpine Linux 3.12.1
* tests/test-parse-datetime.c: Include errno.h for errno,
and unistd.h for _SC_TZNAME_MAX and sysconf.
diff --git a/lib/parse-datetime.y b/lib/parse-datetime.y
index 0c6246742..44ae90350 100644
--- a/lib/parse-datetime.y
+++ b/lib/parse-datetime.y
@@ -27,7 +27,7 @@
Modified by Paul Eggert <egg...@twinsun.com> in 1999 to do the
right thing about local DST. Also modified by Paul Eggert
<egg...@cs.ucla.edu> in 2004 to support nanosecond-resolution
- timestamps, in 2004 to support TZ strings in dates, and in 2017 to
+ timestamps, in 2004 to support TZ strings in dates, and in 2017 and 2020 to
check for integer overflow and to support longer-than-'long'
'time_t' and 'tv_nsec'. */
@@ -63,7 +63,6 @@
#include <inttypes.h>
#include <c-ctype.h>
-#include <limits.h>
#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
@@ -1410,13 +1409,12 @@ yylex (union YYSTYPE *lvalp, parser_control *pc)
if (c_isdigit (c) || c == '-' || c == '+')
{
- char const *p;
+ char const *p = pc->input;
int sign;
- intmax_t value = 0;
if (c == '-' || c == '+')
{
sign = c == '-' ? -1 : 1;
- while (c = *++pc->input, c_isspace (c))
+ while (c = *(pc->input = ++p), c_isspace (c))
continue;
if (! c_isdigit (c))
/* skip the '-' sign */
@@ -1424,8 +1422,8 @@ yylex (union YYSTYPE *lvalp, parser_control *pc)
}
else
sign = 0;
- p = pc->input;
+ time_t value = 0;
do
{
if (INT_MULTIPLY_WRAPV (value, 10, &value))
@@ -1438,17 +1436,12 @@ yylex (union YYSTYPE *lvalp, parser_control *pc)
if ((c == '.' || c == ',') && c_isdigit (p[1]))
{
- time_t s;
- int ns;
+ time_t s = value;
int digits;
- if (time_overflow (value))
- return '?';
- s = value;
-
/* Accumulate fraction, to ns precision. */
p++;
- ns = *p++ - '0';
+ int ns = *p++ - '0';
for (digits = 2; digits <= LOG10_BILLION; digits++)
{
ns *= 10;
@@ -1472,9 +1465,8 @@ yylex (union YYSTYPE *lvalp, parser_control *pc)
negative. */
if (sign < 0 && ns)
{
- if (s == TYPE_MINIMUM (time_t))
+ if (INT_SUBTRACT_WRAPV (s, 1, &s))
return '?';
- s--;
ns = BILLION - ns;
}
@@ -1857,11 +1849,9 @@ parse_datetime2 (struct timespec *result, char const *p,
int quarter;
for (quarter = 1; quarter <= 3; quarter++)
{
- intmax_t iprobe;
- if (INT_ADD_WRAPV (Start, quarter * (90 * 24 * 60 * 60), &iprobe)
- || time_overflow (iprobe))
+ time_t probe;
+ if (INT_ADD_WRAPV (Start, quarter * (90 * 24 * 60 * 60), &probe))
break;
- time_t probe = iprobe;
struct tm probe_tm;
if (localtime_rz (tz, &probe, &probe_tm) && probe_tm.tm_zone
&& probe_tm.tm_isdst != pc.local_time_zone_table[0].value)
@@ -2237,7 +2227,6 @@ parse_datetime2 (struct timespec *result, char const *p,
so this block must follow others that clobber Start. */
if (pc.zones_seen)
{
- intmax_t delta = pc.time_zone, t1;
bool overflow = false;
#ifdef HAVE_TM_GMTOFF
long int utcoff = tm.tm_gmtoff;
@@ -2248,9 +2237,11 @@ parse_datetime2 (struct timespec *result, char const *p,
? tm_diff (&tm, &gmt)
: (overflow = true, 0));
#endif
- overflow |= INT_SUBTRACT_WRAPV (delta, utcoff, &delta);
+ intmax_t delta;
+ overflow |= INT_SUBTRACT_WRAPV (pc.time_zone, utcoff, &delta);
+ time_t t1;
overflow |= INT_SUBTRACT_WRAPV (Start, delta, &t1);
- if (overflow || time_overflow (t1))
+ if (overflow)
{
if (pc.parse_datetime_debug)
dbg_printf (_("error: timezone %d caused time_t overflow\n"),
@@ -2281,14 +2272,14 @@ parse_datetime2 (struct timespec *result, char const *p,
intmax_t sum_ns = orig_ns + pc.rel.ns;
int normalized_ns = (sum_ns % BILLION + BILLION) % BILLION;
int d4 = (sum_ns - normalized_ns) / BILLION;
- intmax_t d1, t1, d2, t2, t3, t4;
+ intmax_t d1, t1, d2, t2, t3;
+ time_t t4;
if (INT_MULTIPLY_WRAPV (pc.rel.hour, 60 * 60, &d1)
|| INT_ADD_WRAPV (Start, d1, &t1)
|| INT_MULTIPLY_WRAPV (pc.rel.minutes, 60, &d2)
|| INT_ADD_WRAPV (t1, d2, &t2)
|| INT_ADD_WRAPV (t2, pc.rel.seconds, &t3)
- || INT_ADD_WRAPV (t3, d4, &t4)
- || time_overflow (t4))
+ || INT_ADD_WRAPV (t3, d4, &t4))
{
if (pc.parse_datetime_debug)
dbg_printf (_("error: adding relative time caused an "
--
2.25.1
>From e2739ba6310893be93d01a23cbfed8d8dfb08966 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Wed, 11 Nov 2020 19:20:42 -0800
Subject: [PATCH 3/3] time_rz: simplify CVE-2017-7476 fix
* lib/time_rz.c: Do not include limits.h; I think it was included
under the mistaken impression that limits.h defines SIZE_MAX.
(SIZE_MAX): Remove.
(save_abbr): Put string length into a ptrdiff_t variable,
so that the size comparison works naturally. This
fixes CVE-2017-7476 in a cleaner way.
---
ChangeLog | 8 ++++++++
lib/time_rz.c | 15 ++-------------
2 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 530c9a661..fe298605e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,13 @@
2020-11-11 Paul Eggert <egg...@cs.ucla.edu>
+ time_rz: simplify CVE-2017-7476 fix
+ * lib/time_rz.c: Do not include limits.h; I think it was included
+ under the mistaken impression that limits.h defines SIZE_MAX.
+ (SIZE_MAX): Remove.
+ (save_abbr): Put string length into a ptrdiff_t variable,
+ so that the size comparison works naturally. This
+ fixes CVE-2017-7476 in a cleaner way.
+
parse-datetime: streamline overflow checking
When parse-datetime.y’s overflow code was written, INT_ADD_WRAPV
did not work for unsigned destinations, and since time_t might
diff --git a/lib/time_rz.c b/lib/time_rz.c
index c58e6831b..a33b8078b 100644
--- a/lib/time_rz.c
+++ b/lib/time_rz.c
@@ -27,7 +27,6 @@
#include <time.h>
#include <errno.h>
-#include <limits.h>
#include <stdbool.h>
#include <stddef.h>
#include <stdlib.h>
@@ -36,10 +35,6 @@
#include "flexmember.h"
#include "time-internal.h"
-#ifndef SIZE_MAX
-# define SIZE_MAX ((size_t) -1)
-#endif
-
/* The approximate size to use for small allocation requests. This is
the largest "small" request for the GNU C library malloc. */
enum { DEFAULT_MXFAST = 64 * sizeof (size_t) / 4 };
@@ -125,14 +120,8 @@ save_abbr (timezone_t tz, struct tm *tm)
{
if (! (*zone_copy || (zone_copy == tz->abbrs && tz->tz_is_set)))
{
- size_t zone_size = strlen (zone) + 1;
- size_t zone_used = zone_copy - tz->abbrs;
- if (SIZE_MAX - zone_used < zone_size)
- {
- errno = ENOMEM;
- return false;
- }
- if (zone_used + zone_size < ABBR_SIZE_MIN)
+ ptrdiff_t zone_size = strlen (zone) + 1;
+ if (zone_size < tz->abbrs + ABBR_SIZE_MIN - zone_copy)
extend_abbrs (zone_copy, zone, zone_size);
else
{
--
2.25.1
