| Message ID | 20181030111850.5322-1-albert.aribaud@3adev.fr |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] Ensure mktime sets errno on error (bug 23789) | expand |
On Tue, 30 Oct 2018 12:18:50 +0100, "Albert ARIBAUD (3ADEV)" <albert.aribaud@3adev.fr> wrote : > Posix mandates that mktime set errno to EOVERFLOW > on error, but the glibc mktime wasn't doing it so > far. > > Fix this and add a test to prevent regressions. > The fix also fixes the same issue in timegm. > > Tested with 'make check' on x86_64-linux-gnu and > i686-linux-gnu. > > * time/Makefile: Add bug-mktime4. > * time/bug-mktime4.c: New file. > * time/mktime.c > (__mktime_internal): Set errno to EOVERFLOW on error. > (mktime): Move call to __tzset inside conditional. > --- > History: > - v4 > - no source code change. > - patch run through 'make check' on x86_64-linux-gnu in addition to > i686-linux-gnu. > - v3: > - time/tst-mktime4.c: switch to support/test-driver. > - time/mktime: remove useless errno handling and move call to __tzset > insde conditional. > - v2: > - __mktime_internal: set errno to EOVERFLOW upon failure. > - mktime: detect failure from __tzset and __mktime_internal by clearing > errno before call and checking it after. Final errno is as follows: > - errno set by __mktime_internal if there was one; > - otherwise, 0 if __mktime_internal returned a non-failure -1; > - otherwise, errno set by __tzset if there was one; > - otherwise, value of errno on entry in mktime. > - v1: > - mktime: set errno to EOVERFLOW if __mktime_internal returns -1 > > Notes: > - v1 erroneously took any return value of -1 as a sign of error, regardless > to whether errno was 0 or not; v2 handles the case where __mktime_internal > return -1 as a correct value. > - an alternative design was considered where every function called, > directly or indirectly, from mktime would have been made to return a > failure status but not change errno (and wrappers created to provide > these function's original behavior). The change was too extensive, and > had a high risk of breaking some behavior. > - timegm() automatically benefits from this change too, i.e., it now > reports failures properly with errno=EOVERFLOW. > - __tzset may set errno (e.g. to ENOENT) and then __mktime may overwrite > this with errno=EOVERFLOW (when failing) or errno=0 (when return valid > -1). However, that was already the case also before the patch. If this version is fine with everyone, I'll prepare a candidate commit for master. As this is my first bugzilla related patch, I'll make the commit available for review before I actually push it onto master. Cordialement, Albert ARIBAUD 3ADEV
Hi all, On Wed, 31 Oct 2018 16:56:02 +0100, Albert ARIBAUD <albert.aribaud@3adev.fr> wrote : > If this version is fine with everyone, I'll prepare a candidate commit > for master. As this is my first bugzilla related patch, I'll make > the commit available for review before I actually push it onto > master. From what I read, the only thing I had to do regarding Bugzilla in the patch was mention the BZ number. The final to be committed patch is at branch aaribaud/bugzilla/23789/v4 for review. What should I do regarding Bugzilla itself? Cordialement, Albert ARIBAUD 3ADEV
On Fri, 2 Nov 2018, Albert ARIBAUD wrote:
> What should I do regarding Bugzilla itself?
https://sourceware.org/glibc/wiki/Bugzilla%20Procedures
https://sourceware.org/glibc/wiki/Committer%20checklist#Update_Bugzilla
The general expectation is that *once a fix has been committed and pushed
to master*, the bug should be marked as RESOLVED/FIXED with the target
milestone set to the next mainline release with the fix. It is the
committer's responsibility to do that update promptly after committing.
Committers have @gcc.gnu.org / @sourceware.org addresses and a Bugzilla
account with such an address should automatically have the required
permissions to update milestones. (Milestones should not be set on open
bugs. If a fix needs to be reverted for some reason, the bug should be
reopened and the milestone setting removed.)
Hi Joseph, On Fri, 2 Nov 2018 16:48:25 +0000, Joseph Myers <joseph@codesourcery.com> wrote : > On Fri, 2 Nov 2018, Albert ARIBAUD wrote: > > > What should I do regarding Bugzilla itself? > > https://sourceware.org/glibc/wiki/Bugzilla%20Procedures > https://sourceware.org/glibc/wiki/Committer%20checklist#Update_Bugzilla > > The general expectation is that *once a fix has been committed and pushed > to master*, the bug should be marked as RESOLVED/FIXED with the target > milestone set to the next mainline release with the fix. It is the > committer's responsibility to do that update promptly after committing. > Committers have @gcc.gnu.org / @sourceware.org addresses and a > Bugzilla account with such an address should automatically have the > required permissions to update milestones. (Milestones should not be > set on open bugs. If a fix needs to be reverted for some reason, the > bug should be reopened and the milestone setting removed.) I don't have an @gcc.gnu.org or @sourceware.org address; I use my albert.aribaud@3adev.fr address. This means I cannot update Bugzilla properly unless my account gets given the required permissions, right? Cordialement, Albert ARIBAUD 3ADEV
On Sat, 3 Nov 2018, Albert ARIBAUD wrote: > I don't have an @gcc.gnu.org or @sourceware.org address; I use my > albert.aribaud@3adev.fr address. > > This means I cannot update Bugzilla properly unless my account gets > given the required permissions, right? You can either have the permissions added to that account, or create an aaribaud@gcc.gnu.org / aaribaud@sourceware.org account in Bugzilla.
[cc'ing to bug-gnulib since mktime.c is shared with gnulib] In <https://www.sourceware.org/ml/libc-alpha/2018-10/msg00662.html> 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 c6223def207b0e436d215d0e3577f0703559be42 Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Fri, 2 Nov 2018 18:49:23 -0700 Subject: [PATCH] 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 | 21 +++++++++++++++------ 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/ChangeLog b/ChangeLog index 497f5b721c..aa3150f138 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2018-11-02 Paul Eggert <eggert@cs.ucla.edu> + + 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-02 Florian Weimer <fweimer@redhat.com> * support/shell-container.c (copy_func): Call diff --git a/time/mktime.c b/time/mktime.c index 00f0dec6b4..26f7a27516 100644 --- a/time/mktime.c +++ b/time/mktime.c @@ -39,7 +39,7 @@ */ #if !defined _LIBC && !DEBUG_MKTIME -# include <config.h> +# include <libc-config.h> #endif /* Assume that leap seconds are possible, unless told otherwise. @@ -51,6 +51,7 @@ #include <time.h> +#include <errno.h> #include <limits.h> #include <stdbool.h> #include <stdlib.h> @@ -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; }
Paul Eggert wrote: > 3. Please construct a third patch containing your mktime test case for glibc, > and we then apply that patch to glibc. I looked at that test case and found some issues with it, e.g., it assumed a particular time_t width in some cases and assumed a particular error number in others. Attached is a revised test case that should fix the issues. For convenience I'm also attaching the same glibc code patch again. From 11823e3d7b7aa6126607dbdd9c495f344682ea04 Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Fri, 2 Nov 2018 18:49:23 -0700 Subject: [PATCH 1/2] 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 | 21 +++++++++++++++------ 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/ChangeLog b/ChangeLog index ef268e8337..231a69b65a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2018-11-04 Paul Eggert <eggert@cs.ucla.edu> + + 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-03 Samuel Thibault <samuel.thibault@ens-lyon.org> * sysdeps/mach/hurd/msync.c: New file. diff --git a/time/mktime.c b/time/mktime.c index 00f0dec6b4..26f7a27516 100644 --- a/time/mktime.c +++ b/time/mktime.c @@ -39,7 +39,7 @@ */ #if !defined _LIBC && !DEBUG_MKTIME -# include <config.h> +# include <libc-config.h> #endif /* Assume that leap seconds are possible, unless told otherwise. @@ -51,6 +51,7 @@ #include <time.h> +#include <errno.h> #include <limits.h> #include <stdbool.h> #include <stdlib.h> @@ -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; }
Hi Joseph, On Sat, 3 Nov 2018 00:16:04 +0000, Joseph Myers <joseph@codesourcery.com> wrote : > On Sat, 3 Nov 2018, Albert ARIBAUD wrote: > > > I don't have an @gcc.gnu.org or @sourceware.org address; I use my > > albert.aribaud@3adev.fr address. > > > > This means I cannot update Bugzilla properly unless my account gets > > given the required permissions, right? > > You can either have the permissions added to that account, or create an > aaribaud@gcc.gnu.org / aaribaud@sourceware.org account in Bugzilla. I'd rather not multiply accounts if I can avoid it. How do I have the permissions added to albert.aribaud@3adev.fr? Cordialement, Albert ARIBAUD 3ADEV
Hi Paul, On Sun, 4 Nov 2018 23:54:59 -0800, Paul Eggert <eggert@cs.ucla.edu> wrote : > Paul Eggert wrote: > > 3. Please construct a third patch containing your mktime test case for glibc, > > and we then apply that patch to glibc. > > I looked at that test case and found some issues with it, e.g., it assumed a > particular time_t width in some cases and assumed a particular error number in > others. Attached is a revised test case that should fix the issues. For > convenience I'm also attaching the same glibc code patch again. Apparently, with both your patches applied there are still paths which yield "mktime failed without setting errno" when make check is run for i686-linux-gnu. I'll go through the call path and see where it fails. Cordialement, Albert ARIBAUD 3ADEV
Hi Paul, On Tue, 6 Nov 2018 06:43:06 +0100, Albert ARIBAUD <albert.aribaud@3adev.fr> wrote : > Hi Paul, > > On Sun, 4 Nov 2018 23:54:59 -0800, Paul Eggert <eggert@cs.ucla.edu> > wrote : > > > Paul Eggert wrote: > > > 3. Please construct a third patch containing your mktime test case for glibc, > > > and we then apply that patch to glibc. > > > > I looked at that test case and found some issues with it, e.g., it assumed a > > particular time_t width in some cases and assumed a particular error number in > > others. Attached is a revised test case that should fix the issues. For > > convenience I'm also attaching the same glibc code patch again. > > Apparently, with both your patches applied there are still paths which > yield "mktime failed without setting errno" when make check is run for > i686-linux-gnu. I'll go through the call path and see where it fails. Issue is that __mktime_internal exited through else if (--remaining_probes == 0) return -1; with errno never set. Any idea why? Cordialement, Albert ARIBAUD 3ADEV
On 11/6/18 12:41 PM, Albert ARIBAUD wrote: > Issue is that __mktime_internal exited through > > else if (--remaining_probes == 0) > return -1; > > with errno never set. > > Any idea why? Either localtime_r is failing without setting errno so the bug is in localtime_r, or localtime_r never fails and so never sets errno so the bug is in mktime. Can you check which of these is happening?
Hi Paul, On Tue, 6 Nov 2018 16:28:36 -0800, Paul Eggert <eggert@cs.ucla.edu> wrote : > On 11/6/18 12:41 PM, Albert ARIBAUD wrote: > > Issue is that __mktime_internal exited through > > > > else if (--remaining_probes == 0) > > return -1; > > > > with errno never set. > > > > Any idea why? > > Either localtime_r is failing without setting errno so the bug is in > localtime_r, or localtime_r never fails and so never sets errno so the > bug is in mktime. Can you check which of these is happening? I've instrumented the code while executing time/bug-mktime4.c. The point where a failure result is returned without setting errno is at line 442: else if (--remaining_probes == 0) return -1; If I add a '__set_errno(EOVERFLOW);' under the else clause before the 'return -1;', then the test case runs fine. But I cannot set errno here since I might overwrite a previous value set some time between when mktime was entered and now. So I have had a look at the functions involved in the for loop around these lines: guess_time_tm and range_convert, to see how they handle errno From what I understand, guess_time_tm never fails as such. It may set errno to EOVERFLOW, but that cannot explain the problem, which is the opposite, i.e. failing without setting errno. range_convert calls convert_time, which calls convert, which point to localtime_r, which calls __tz_convert. Of the three functions which __tz_convert calls, that is, __offtime, __tz_compute, and __tzfile_compute, none fails without setting errno. (although I noticed that __tzfile_compute may call __tzstring which might set errno, but __tzfile_compute returns void, so there's no way for its caller to find out errno was set. But again, it is not the type of issue we have here, which is errno *not* being set on failure.) So it seems to me that we're really "just" reaching a maximum of probes after which __mktime_internal, while not failing at computing candidate times, could not find a perfect match in less than the 6 rounds it allows itself. I am instrumenting the code further to unravel the for probe loop logic; anyone to whom this rings a bell is welcome to comment. Cordialement, Albert ARIBAUD 3ADEV
On Wed, 7 Nov 2018 13:39:42 +0100, Albert ARIBAUD <albert.aribaud@3adev.fr> wrote : > So it seems to me that we're really "just" reaching a maximum of probes > after which __mktime_internal, while not failing at computing candidate > times, could not find a perfect match in less than the 6 rounds it > allows itself. > > I am instrumenting the code further to unravel the for probe loop > logic; anyone to whom this rings a bell is welcome to comment. The loop is acting weird. For all six iteration, at the loop body start, gt was equal to -67768038462257713 and t, t1 and t2 were all equal to -2147483648 -- no evolution during all six iterations, but never t==gt, so the loop used up its remaining_probes and returned -1. This leads me to two conclusions: 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. 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. Regarding point 2, The -2147483648 value of t smells of 32-bit signed saturation, and indeed, ranged_convert gets passed a pointer to t and saturates it between mktime_min and mktime_max, which are defined to be the shortest extrema between long_int and time_t. Now, 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? Cordialement, Albert ARIBAUD 3ADEV
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 <eggert@cs.ucla.edu> 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 <eggert@cs.ucla.edu> + + 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 <msebor@redhat.com> * 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 <config.h> +# include <libc-config.h> #endif /* Assume that leap seconds are possible, unless told otherwise. @@ -51,6 +51,7 @@ #include <time.h> +#include <errno.h> #include <limits.h> #include <stdbool.h> #include <stdlib.h> @@ -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; }
Hi Albert, On Mon, 5 Nov 2018 21:29:38 +0100, Albert ARIBAUD <albert.aribaud@3adev.fr> wrote : > Hi Joseph, > > On Sat, 3 Nov 2018 00:16:04 +0000, Joseph Myers > <joseph@codesourcery.com> wrote : > > > On Sat, 3 Nov 2018, Albert ARIBAUD wrote: > > > > > I don't have an @gcc.gnu.org or @sourceware.org address; I use my > > > albert.aribaud@3adev.fr address. > > > > > > This means I cannot update Bugzilla properly unless my account gets > > > given the required permissions, right? > > > > You can either have the permissions added to that account, or create an > > aaribaud@gcc.gnu.org / aaribaud@sourceware.org account in Bugzilla. > > I'd rather not multiply accounts if I can avoid it. How do I have the > permissions added to albert.aribaud@3adev.fr? Ping? Cordialement, Albert ARIBAUD 3ADEV
Hi Paul, On Fri, 9 Nov 2018 18:00:25 -0800, Paul Eggert <eggert@cs.ucla.edu> wrote : > 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. I've applied the series above current master (with the ChangeLog date adapted) and the make check stats are unchanged by the series, which means the added test indeed returns ok with these patches. If I can get the adequate permissions for Bugzilla. Cordialement, Albert ARIBAUD 3ADEV
On Wed, 14 Nov 2018 00:04:04 +0100, Albert ARIBAUD <albert.aribaud@3adev.fr> wrote : > If I can get > the adequate permissions for Bugzilla. ... I'll push the series onto master. Cordialement, Albert ARIBAUD 3ADEV
On Tue, 13 Nov 2018, Albert ARIBAUD wrote: > > I'd rather not multiply accounts if I can avoid it. How do I have the > > permissions added to albert.aribaud@3adev.fr? > > Ping? I've added you to the editbugs group.
Hi Joseph, On Thu, 15 Nov 2018 17:01:38 +0000, Joseph Myers <joseph@codesourcery.com> wrote : > On Tue, 13 Nov 2018, Albert ARIBAUD wrote: > > > > I'd rather not multiply accounts if I can avoid it. How do I have the > > > permissions added to albert.aribaud@3adev.fr? > > > > Ping? > > I've added you to the editbugs group. Thanks! Cordialement, Albert ARIBAUD 3ADEV
diff --git a/time/Makefile b/time/Makefile index ec3e39dcea..743bd99f18 100644 --- a/time/Makefile +++ b/time/Makefile @@ -43,7 +43,7 @@ tests := test_time clocktest tst-posixtz tst-strptime tst_wcsftime \ tst-getdate tst-mktime tst-mktime2 tst-ftime_l tst-strftime \ tst-mktime3 tst-strptime2 bug-asctime bug-asctime_r bug-mktime1 \ tst-strptime3 bug-getdate1 tst-strptime-whitespace tst-ftime \ - tst-tzname tst-y2039 + tst-tzname tst-y2039 bug-mktime4 include ../Rules diff --git a/time/bug-mktime4.c b/time/bug-mktime4.c new file mode 100644 index 0000000000..14d04c669b --- /dev/null +++ b/time/bug-mktime4.c @@ -0,0 +1,27 @@ +#include <time.h> +#include <errno.h> +#include <limits.h> +#include <stdio.h> +#include <string.h> + +static int +do_test (void) +{ + struct tm tm = { .tm_year = INT_MIN, .tm_mon = INT_MIN, .tm_mday = INT_MIN, + .tm_hour = INT_MIN, .tm_min = INT_MIN, .tm_sec = INT_MIN }; + errno = 0; + time_t tt = mktime (&tm); + if (tt != -1) + { + printf ("mktime() should have returned -1, returned %ld\n", (long int) tt); + return 1; + } + if (errno != EOVERFLOW) + { + printf ("mktime() returned -1, errno should be %d (EOVERFLOW) but is %d (%s)\n", EOVERFLOW, errno, strerror(errno)); + return 1; + } + return 0; +} + +#include "support/test-driver.c" diff --git a/time/mktime.c b/time/mktime.c index 00f0dec6b4..2e0c467147 100644 --- a/time/mktime.c +++ b/time/mktime.c @@ -49,6 +49,7 @@ # define LEAP_SECONDS_POSSIBLE 1 #endif +#include <errno.h> #include <time.h> #include <limits.h> @@ -435,7 +436,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. */ @@ -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; + } } *tp = tm; @@ -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
Posix mandates that mktime set errno to EOVERFLOW on error, but the glibc mktime wasn't doing it so far. Fix this and add a test to prevent regressions. The fix also fixes the same issue in timegm. Tested with 'make check' on x86_64-linux-gnu and i686-linux-gnu. * time/Makefile: Add bug-mktime4. * time/bug-mktime4.c: New file. * time/mktime.c (__mktime_internal): Set errno to EOVERFLOW on error. (mktime): Move call to __tzset inside conditional. --- History: - v4 - no source code change. - patch run through 'make check' on x86_64-linux-gnu in addition to i686-linux-gnu. - v3: - time/tst-mktime4.c: switch to support/test-driver. - time/mktime: remove useless errno handling and move call to __tzset insde conditional. - v2: - __mktime_internal: set errno to EOVERFLOW upon failure. - mktime: detect failure from __tzset and __mktime_internal by clearing errno before call and checking it after. Final errno is as follows: - errno set by __mktime_internal if there was one; - otherwise, 0 if __mktime_internal returned a non-failure -1; - otherwise, errno set by __tzset if there was one; - otherwise, value of errno on entry in mktime. - v1: - mktime: set errno to EOVERFLOW if __mktime_internal returns -1 Notes: - v1 erroneously took any return value of -1 as a sign of error, regardless to whether errno was 0 or not; v2 handles the case where __mktime_internal return -1 as a correct value. - an alternative design was considered where every function called, directly or indirectly, from mktime would have been made to return a failure status but not change errno (and wrappers created to provide these function's original behavior). The change was too extensive, and had a high risk of breaking some behavior. - timegm() automatically benefits from this change too, i.e., it now reports failures properly with errno=EOVERFLOW. - __tzset may set errno (e.g. to ENOENT) and then __mktime may overwrite this with errno=EOVERFLOW (when failing) or errno=0 (when return valid -1). However, that was already the case also before the patch. time/Makefile | 2 +- time/bug-mktime4.c | 27 +++++++++++++++++++++++++++ time/mktime.c | 16 +++++++++++----- 3 files changed, 39 insertions(+), 6 deletions(-) create mode 100644 time/bug-mktime4.c