This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)
- From: Paul Eggert <eggert at cs dot ucla dot edu>
- To: "Albert ARIBAUD (3ADEV)" <albert dot aribaud at 3adev dot fr>
- Cc: libc-alpha at sourceware dot org, Gnulib bugs <bug-gnulib at gnu dot org>
- Date: Fri, 2 Nov 2018 19:13:50 -0700
- Subject: Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)
- References: <20181030111850.5322-1-albert.aribaud@3adev.fr>
[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;
}
--
2.19.1
>From 96e52c5786964954e41e3242b5342a9462f6fa78 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 2 Nov 2018 18:48:57 -0700
Subject: [PATCH] mktime: fix EOVERFLOW bug
This fixes a glibc bug I reported here:
https://sourceware.org/bugzilla/show_bug.cgi?id=23789
* lib/mktime.c: Sync from glibc, incorporating the following:
[!_LIBC && !DEBUG_MKTIME]:
Include libc-config.h, not config.h, for __set_errno.
(guess_time_tm, __mktime_internal): Set errno to EOVERFLOW on overflow.
* m4/mktime.m4 (gl_FUNC_MKTIME_WORKS):
Check for EOVERFLOW bug in glibc 2.28 and earlier.
* modules/mktime (Depends-on): Add libc-config.
---
ChangeLog | 11 +++++++++++
lib/mktime.c | 21 +++++++++++++++------
m4/mktime.m4 | 5 ++++-
modules/mktime | 1 +
4 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index bbf379b8c..2ce3520fb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
2018-11-02 Paul Eggert <eggert@cs.ucla.edu>
+ mktime: fix EOVERFLOW bug
+ This fixes a glibc bug I reported here:
+ https://sourceware.org/bugzilla/show_bug.cgi?id=23789
+ * lib/mktime.c: Sync from glibc, incorporating the following:
+ [!_LIBC && !DEBUG_MKTIME]:
+ Include libc-config.h, not config.h, for __set_errno.
+ (guess_time_tm, __mktime_internal): Set errno to EOVERFLOW on overflow.
+ * m4/mktime.m4 (gl_FUNC_MKTIME_WORKS):
+ Check for EOVERFLOW bug in glibc 2.28 and earlier.
+ * modules/mktime (Depends-on): Add libc-config.
+
gnulib-common.m4: port _Noreturn to C++
Problem reported by Akim Demaille in:
https://lists.gnu.org/r/bug-bison/2018-10/msg00067.html
diff --git a/lib/mktime.c b/lib/mktime.c
index 00f0dec6b..26f7a2751 100644
--- a/lib/mktime.c
+++ b/lib/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;
}
diff --git a/m4/mktime.m4 b/m4/mktime.m4
index 4b3e399be..88f5dc92f 100644
--- a/m4/mktime.m4
+++ b/m4/mktime.m4
@@ -1,4 +1,4 @@
-# serial 30
+# serial 31
dnl Copyright (C) 2002-2003, 2005-2007, 2009-2018 Free Software Foundation,
dnl Inc.
dnl This file is free software; the Free Software Foundation
@@ -43,6 +43,7 @@ AC_DEFUN([gl_FUNC_MKTIME_WORKS],
[AC_RUN_IFELSE(
[AC_LANG_SOURCE(
[[/* Test program from Paul Eggert and Tony Leneis. */
+#include <errno.h>
#include <limits.h>
#include <stdlib.h>
#include <time.h>
@@ -150,6 +151,8 @@ bigtime_test (int j)
== (tm.tm_isdst < 0 ? -1 : 0 < tm.tm_isdst))))
return 0;
}
+ else if (errno != EOVERFLOW)
+ return 0;
return 1;
}
diff --git a/modules/mktime b/modules/mktime
index f08c5b14c..17ed3cd78 100644
--- a/modules/mktime
+++ b/modules/mktime
@@ -10,6 +10,7 @@ Depends-on:
time
multiarch
intprops [test $REPLACE_MKTIME = 1]
+libc-config [test $REPLACE_MKTIME = 1]
stdbool [test $REPLACE_MKTIME = 1]
time_r [test $REPLACE_MKTIME = 1]
verify [test $REPLACE_MKTIME = 1]
--
2.19.1