Re: parse-datetime test failure

Submitted by Paul Eggert on Nov. 12, 2020, 3:38 a.m.

Details

Message ID e08b126d-fb6a-8b6b-676c-c5815da86f80@cs.ucla.edu
State New
Series "Re: parse-datetime test failure"
Headers show

Commit Message

Paul Eggert Nov. 12, 2020, 3:38 a.m.
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

Patch hide | download patch | download mbox

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


Comments

Rich Felker Nov. 12, 2020, 4:07 a.m.
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
Paul Eggert Nov. 12, 2020, 4:10 a.m.
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