[v4] Ensure mktime sets errno on error (bug 23789)
Commit Message
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.
Comments
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
@@ -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]:
@@ -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
new file mode 100644
@@ -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