| Message ID | e9704996-c39a-d4cb-2a62-b0342924cdc5@cs.ucla.edu |
|---|---|
| State | New, archived |
| Headers | show |
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
From b634d4e4025768787e14604e08284e5a3ac0da6e Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Sun, 4 Nov 2018 23:49:03 -0800 Subject: [PATCH 2/2] mktime: new test for mktime failure Based on a test suggested by Albert Aribaud in: https://www.sourceware.org/ml/libc-alpha/2018-10/msg00662.html * time/Makefile (tests): Add bug-mktime4. * time/bug-mktime4.c: New file. --- ChangeLog | 6 +++ time/Makefile | 2 +- time/bug-mktime4.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 time/bug-mktime4.c diff --git a/ChangeLog b/ChangeLog index 231a69b65a..c5b806a54f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ 2018-11-04 Paul Eggert <eggert@cs.ucla.edu> + mktime: new test for mktime failure + Based on a test suggested by Albert Aribaud in: + https://www.sourceware.org/ml/libc-alpha/2018-10/msg00662.html + * time/Makefile (tests): Add bug-mktime4. + * time/bug-mktime4.c: New file. + mktime: fix EOVERFLOW bug [BZ#23789] * time/mktime.c [!_LIBC && !DEBUG_MKTIME]: 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..9c076eb623 --- /dev/null +++ b/time/bug-mktime4.c @@ -0,0 +1,92 @@ +#include <time.h> +#include <errno.h> +#include <limits.h> +#include <stdbool.h> +#include <stdio.h> +#include <string.h> + +static bool +equal_tm (struct tm const *t, struct tm const *u) +{ + return (t->tm_sec == u->tm_sec && t->tm_min == u->tm_min + && t->tm_hour == u->tm_hour && t->tm_mday == u->tm_mday + && t->tm_mon == u->tm_mon && t->tm_year == u->tm_year + && t->tm_wday == u->tm_wday && t->tm_yday == u->tm_yday + && t->tm_isdst == u->tm_isdst && t->tm_gmtoff == u->tm_gmtoff + && t->tm_zone == u->tm_zone); +} + +static int +do_test (void) +{ + /* Calculate minimum time_t value. This would be simpler with C11, + which has _Generic, but we cannot assume C11. It would also + be simpler with intprops.h, which has TYPE_MINIMUM, but it's + better not to use glibc internals. */ + time_t time_t_min = -1; + time_t_min = (0 < time_t_min ? 0 + : sizeof time_t_min == sizeof (long int) ? LONG_MIN + : sizeof time_t_min == sizeof (long long int) ? LLONG_MIN + : 1); + if (time_t_min == 1) + { + printf ("unknown time type\n"); + return 1; + } + time_t ymin = time_t_min / 60 / 60 / 24 / 366; + bool mktime_should_fail = ymin == 0 || INT_MIN + 1900 < ymin + 1970; + + struct tm tm0 = { .tm_year = INT_MIN, .tm_mday = 1, .tm_wday = -1 }; + struct tm tm = tm0; + errno = 0; + time_t t = mktime (&tm); + long long int llt = t; + bool mktime_failed = tm.tm_wday == tm0.tm_wday; + + if (mktime_failed) + { + if (! mktime_should_fail) + { + printf ("mktime failed but should have succeeded\n"); + return 1; + } + if (errno == 0) + { + printf ("mktime failed without setting errno"); + return 1; + } + if (t != (time_t) -1) + { + printf ("mktime returned %lld but did not set tm_wday\n", llt); + return 1; + } + if (! equal_tm (&tm, &tm0)) + { + printf ("mktime (P) failed but modified *P\n"); + return 1; + } + } + else + { + if (mktime_should_fail) + { + printf ("mktime succeeded but should have failed\n"); + return 1; + } + struct tm *lt = localtime (&t); + if (lt == NULL) + { + printf ("mktime returned a value rejected by localtime\n"); + return 1; + } + if (! equal_tm (lt, &tm)) + { + printf ("mktime result does not match localtime result\n"); + return 1; + } + } + return 0; +} + +#define TIMEOUT 1000 +#include "support/test-driver.c" -- 2.19.1