From patchwork Sat Nov 10 02:00:25 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Eggert X-Patchwork-Id: 30100 Received: (qmail 65380 invoked by alias); 10 Nov 2018 02:01:08 -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 61876 invoked by uid 89); 10 Nov 2018 02:00:42 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LOTSOFHASH, KAM_SHORT, SPF_PASS autolearn=ham version=3.3.2 spammy=influence, Revised, tml X-HELO: zimbra.cs.ucla.edu Subject: Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789) To: Albert ARIBAUD Cc: libc-alpha@sourceware.org, Gnulib bugs References: <20181030111850.5322-1-albert.aribaud@3adev.fr> <9947f682-fc46-f2d6-4b07-536c03b9e657@cs.ucla.edu> <20181106064306.45eda0c2@athena> <20181106214147.6391743b@athena> <20181107133942.35141f4e@athena> <20181107154804.23271f4f@athena> From: Paul Eggert Openpgp: preference=signencrypt Message-ID: <7607d256-4154-47c5-dc3e-979cad719a5f@cs.ucla.edu> Date: Fri, 9 Nov 2018 18:00:25 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 MIME-Version: 1.0 In-Reply-To: <20181107154804.23271f4f@athena> On 11/7/18 6:48 AM, Albert ARIBAUD wrote: > 1. If the for-loop reaches remaining_probes==0, then it really should > set errno = EOVERFLOW before returning -1, because remaining_probes > is only decremented in the else clause inside the for-loop, and that > only happens (or should only happen) when there were no failures so > far, so if we fail then, we have to set errno. Thanks for the diagnosis. Revised patches attached, which set errno in that case as you suggested. > 2. It is not normal that t, gt, t1 and t2 remain the same for all six > iterations of the for-loop. That should be investigated and fixed. Long ago I came up with weird scenarios involving odd combinations of leap seconds and DST transitions all near the maximum convertible time_t values that could involve that many iterations. None of these scenarios will ever happen, really; the number is that large just to be safe. It could be that I overestimated the number, but that's no big deal. > I don't know why ranged_convert alters an argument which should be > a pure imput. In fact, I don't know why it does not just copy this > argument into a local time_t. Any known reason? Because it communicates back to the caller the nearest long_int value that is in range. This value is not obvious because it can depend on timezone and leap second information. After looking at the mktime implementation again I see some other things that need fixing. These are mostly for Gnulib (when we can't assume that localtime_r fails only due to EOVERFLOW), but there are some code cleanups and fixes for very unlikely bugs. Proposed glibc patches attached. From d7b1a2c6fda647672fd7774fc70cbe0b797af4e5 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Wed, 7 Nov 2018 07:52:17 -0800 Subject: [PATCH 1/7] mktime: fix EOVERFLOW bug [BZ#23789] * time/mktime.c [!_LIBC && !DEBUG_MKTIME]: Include libc-config.h, not config.h, for __set_errno. (guess_time_tm, __mktime_internal): Set errno to EOVERFLOW on overflow. --- ChangeLog | 8 ++++++++ time/mktime.c | 26 +++++++++++++++++++------- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/ChangeLog b/ChangeLog index e43fd3e987..9f6d1d4edd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2018-11-09 Paul Eggert + + mktime: fix EOVERFLOW bug + [BZ#23789] + * time/mktime.c [!_LIBC && !DEBUG_MKTIME]: + Include libc-config.h, not config.h, for __set_errno. + (guess_time_tm, __mktime_internal): Set errno to EOVERFLOW on overflow. + 2018-11-09 Martin Sebor * include/libc-symbols.h (__attribute_copy__): Define macro unless diff --git a/time/mktime.c b/time/mktime.c index 00f0dec6b4..106b4eac26 100644 --- a/time/mktime.c +++ b/time/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, @@ -435,7 +440,10 @@ __mktime_internal (struct tm *tp, useful than returning -1. */ goto offset_found; else if (--remaining_probes == 0) - return -1; + { + __set_errno (EOVERFLOW); + return -1; + } /* We have a match. Check whether tm.tm_isdst has the requested value, if any. */ @@ -505,8 +513,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; }