[musl] Re: parse-datetime test failure

[musl] Re: parse-datetime test failure

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]

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

[-- Attachment #2: 0001-parse-datetime-tests-port-to-Alpine-Linux-3.12.1.patch --]
[-- Type: text/x-patch, Size: 2835 bytes --]

From 4c9a3c65e279977af4e345748ba73ab0441dc04a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@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  <eggert@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


[-- Attachment #3: 0002-parse-datetime-streamline-overflow-checking.patch --]
[-- Type: text/x-patch, Size: 7383 bytes --]

From 00ffb79c529942eab5c81568808bd317c753213a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@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  <eggert@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 <eggert@twinsun.com> in 1999 to do the
    right thing about local DST.  Also modified by Paul Eggert
    <eggert@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


[-- Attachment #4: 0003-time_rz-simplify-CVE-2017-7476-fix.patch --]
[-- Type: text/x-patch, Size: 2710 bytes --]

From e2739ba6310893be93d01a23cbfed8d8dfb08966 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@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  <eggert@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

Re: [musl] Re: parse-datetime test failure

[Rich Felker]

On Wed, Nov 11, 2020 at 07:38:00PM -0800, Paul Eggert wrote:
> 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

Thanks. I believe you've just re-discovered a known bug that's fixed
in musl commit 33338ebc853d37c80f0f236cc7a92cb0acc6aace, which will be
included in the upcoming 1.2.2 release.

Rich

Re: [musl] Re: parse-datetime test failure

[Paul Eggert]

On 11/11/20 8:07 PM, Rich Felker wrote:
> Thanks. I believe you've just re-discovered a known bug that's fixed
> in musl commit 33338ebc853d37c80f0f236cc7a92cb0acc6aace, which will be
> included in the upcoming 1.2.2 release.

Yes, thanks, that looks exactly right. It's *so* nice to have bugs fixed before 
I even report them!

Here's a URL for the musl patch, for the record:

https://git.musl-libc.org/cgit/musl/commit/?id=33338ebc853d37c80f0f236cc7a92cb0acc6aace