From patchwork Sat Nov 3 02:13:50 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Eggert X-Patchwork-Id: 30013 Received: (qmail 126326 invoked by alias); 3 Nov 2018 02:14:01 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 126266 invoked by uid 89); 3 Nov 2018 02:14:00 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS autolearn=ham version=3.3.2 spammy=H*Ad:D*ucla.edu, H*r:sk:zimbra., Overflow, H*RU:sk:zimbra. X-HELO: zimbra.cs.ucla.edu Subject: Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789) To: "Albert ARIBAUD (3ADEV)" References: <20181030111850.5322-1-albert.aribaud@3adev.fr> Cc: libc-alpha@sourceware.org, Gnulib bugs From: Paul Eggert Message-ID: <9947f682-fc46-f2d6-4b07-536c03b9e657@cs.ucla.edu> Date: Fri, 2 Nov 2018 19:13:50 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20181030111850.5322-1-albert.aribaud@3adev.fr> [cc'ing to bug-gnulib since mktime.c is shared with gnulib] In Albert ARIBAUD (3ADEV) wrote: > useful than returning -1. */ > goto offset_found; > else if (--remaining_probes == 0) > - return -1; > + { > + __set_errno (EOVERFLOW); > + return -1; > + } There should be no need to set errno here, since localtime_r or gmtime_r should have already set errno. And setting errno to EOVERFLOW would be a mistake if localtime_r or gmtime_r set errno to some value other than EOVERFLOW. Conversely, guess_time_tm should set errno on overflow error. > /* We have a match. Check whether tm.tm_isdst has the requested > value, if any. */ > @@ -507,7 +511,10 @@ __mktime_internal (struct tm *tp, > if (INT_ADD_WRAPV (t, sec_adjustment, &t) > || ! (mktime_min <= t && t <= mktime_max) > || ! convert_time (convert, t, &tm)) > - return -1; > + { > + __set_errno (EOVERFLOW); > + return -1; > + } Similarly, this should not set errno if ! convert_time (convert, t, &tm) since convert_time should set errno on failure and we shouldn't second-guess it. > @@ -522,13 +529,12 @@ __mktime_internal (struct tm *tp, > time_t > mktime (struct tm *tp) > { > +# if defined _LIBC || NEED_MKTIME_WORKING > + static mktime_offset_t localtime_offset; > /* POSIX.1 8.1.1 requires that whenever mktime() is called, the > time zone names contained in the external variable 'tzname' shall > be set as if the tzset() function had been called. */ > __tzset (); > - > -# if defined _LIBC || NEED_MKTIME_WORKING > - static mktime_offset_t localtime_offset; > return __mktime_internal (tp, __localtime_r, &localtime_offset); > # else > # undef mktime Come to think of it, this part of the change is not needed. The glibc documentation already says that mktime (p) updates *p only if mktime succeeds. So a caller that wants to determine whether a mktime that returned ((time_t) -1) succeeded merely needs to (say) set p->tm_wday = -1 before calling mktime (p), and then test whether p->tm_wday is still negative after mktime returns. So there is no need for mktime to save and restore errno after all. So, I propose that we install the following patches instead: 1. Apply the first attached patch to glibc. 2. Apply the second attached patch to Gnulib, so that its mktime.c stays in sync with glibc. 3. Please construct a third patch containing your mktime test case for glibc, and we then apply that patch to glibc. From 96e52c5786964954e41e3242b5342a9462f6fa78 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Fri, 2 Nov 2018 18:48:57 -0700 Subject: [PATCH] mktime: fix EOVERFLOW bug This fixes a glibc bug I reported here: https://sourceware.org/bugzilla/show_bug.cgi?id=23789 * lib/mktime.c: Sync from glibc, incorporating the following: [!_LIBC && !DEBUG_MKTIME]: Include libc-config.h, not config.h, for __set_errno. (guess_time_tm, __mktime_internal): Set errno to EOVERFLOW on overflow. * m4/mktime.m4 (gl_FUNC_MKTIME_WORKS): Check for EOVERFLOW bug in glibc 2.28 and earlier. * modules/mktime (Depends-on): Add libc-config. --- ChangeLog | 11 +++++++++++ lib/mktime.c | 21 +++++++++++++++------ m4/mktime.m4 | 5 ++++- modules/mktime | 1 + 4 files changed, 31 insertions(+), 7 deletions(-) diff --git a/ChangeLog b/ChangeLog index bbf379b8c..2ce3520fb 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,16 @@ 2018-11-02 Paul Eggert + mktime: fix EOVERFLOW bug + This fixes a glibc bug I reported here: + https://sourceware.org/bugzilla/show_bug.cgi?id=23789 + * lib/mktime.c: Sync from glibc, incorporating the following: + [!_LIBC && !DEBUG_MKTIME]: + Include libc-config.h, not config.h, for __set_errno. + (guess_time_tm, __mktime_internal): Set errno to EOVERFLOW on overflow. + * m4/mktime.m4 (gl_FUNC_MKTIME_WORKS): + Check for EOVERFLOW bug in glibc 2.28 and earlier. + * modules/mktime (Depends-on): Add libc-config. + gnulib-common.m4: port _Noreturn to C++ Problem reported by Akim Demaille in: https://lists.gnu.org/r/bug-bison/2018-10/msg00067.html diff --git a/lib/mktime.c b/lib/mktime.c index 00f0dec6b..26f7a2751 100644 --- a/lib/mktime.c +++ b/lib/mktime.c @@ -39,7 +39,7 @@ */ #if !defined _LIBC && !DEBUG_MKTIME -# include +# include #endif /* Assume that leap seconds are possible, unless told otherwise. @@ -51,6 +51,7 @@ #include +#include #include #include #include @@ -255,8 +256,9 @@ long_int_avg (long_int a, long_int b) If TP is null, return a value not equal to T; this avoids false matches. YEAR and YDAY must not be so large that multiplying them by three times the number of seconds in a year (or day, respectively) would overflow long_int. - If the returned value would be out of range, yield the minimal or - maximal in-range value, except do not yield a value equal to T. */ + If TP is non-null and the returned value would be out of range, set + errno to EOVERFLOW and yield a minimal or maximal in-range value + that is not equal to T. */ static long_int guess_time_tm (long_int year, long_int yday, int hour, int min, int sec, long_int t, const struct tm *tp) @@ -269,9 +271,10 @@ guess_time_tm (long_int year, long_int yday, int hour, int min, int sec, tp->tm_hour, tp->tm_min, tp->tm_sec); if (! INT_ADD_WRAPV (t, d, &result)) return result; + __set_errno (EOVERFLOW); } - /* Overflow occurred one way or another. Return the nearest result + /* An error occurred, probably overflow. Return the nearest result that is actually in range, except don't report a zero difference if the actual difference is nonzero, as that would cause a false match; and don't oscillate between two values, as that would @@ -344,6 +347,8 @@ ranged_convert (struct tm *(*convert) (const time_t *, struct tm *), Use *OFFSET to keep track of a guess at the offset of the result, compared to what the result would be for UTC without leap seconds. If *OFFSET's guess is correct, only one CONVERT call is needed. + If successful, set *TP to the canonicalized struct tm; + otherwise leave *TP alone, return ((time_t) -1) and set errno. This function is external because it is used also by timegm.c. */ time_t __mktime_internal (struct tm *tp, @@ -505,8 +510,12 @@ __mktime_internal (struct tm *tp, sec_adjustment -= sec; sec_adjustment += sec_requested; if (INT_ADD_WRAPV (t, sec_adjustment, &t) - || ! (mktime_min <= t && t <= mktime_max) - || ! convert_time (convert, t, &tm)) + || ! (mktime_min <= t && t <= mktime_max)) + { + __set_errno (EOVERFLOW); + return -1; + } + if (! convert_time (convert, t, &tm)) return -1; } diff --git a/m4/mktime.m4 b/m4/mktime.m4 index 4b3e399be..88f5dc92f 100644 --- a/m4/mktime.m4 +++ b/m4/mktime.m4 @@ -1,4 +1,4 @@ -# serial 30 +# serial 31 dnl Copyright (C) 2002-2003, 2005-2007, 2009-2018 Free Software Foundation, dnl Inc. dnl This file is free software; the Free Software Foundation @@ -43,6 +43,7 @@ AC_DEFUN([gl_FUNC_MKTIME_WORKS], [AC_RUN_IFELSE( [AC_LANG_SOURCE( [[/* Test program from Paul Eggert and Tony Leneis. */ +#include #include #include #include @@ -150,6 +151,8 @@ bigtime_test (int j) == (tm.tm_isdst < 0 ? -1 : 0 < tm.tm_isdst)))) return 0; } + else if (errno != EOVERFLOW) + return 0; return 1; } diff --git a/modules/mktime b/modules/mktime index f08c5b14c..17ed3cd78 100644 --- a/modules/mktime +++ b/modules/mktime @@ -10,6 +10,7 @@ Depends-on: time multiarch intprops [test $REPLACE_MKTIME = 1] +libc-config [test $REPLACE_MKTIME = 1] stdbool [test $REPLACE_MKTIME = 1] time_r [test $REPLACE_MKTIME = 1] verify [test $REPLACE_MKTIME = 1] -- 2.19.1