[v4] Ensure mktime sets errno on error (bug 23789)
Commit Message
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(-)
Comments
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
@@ -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
@@ -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;
}