This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[PATCH v2] Ensure mktime sets errno on error (bug 23789)
- From: "Albert ARIBAUD (3ADEV)" <albert dot aribaud at 3adev dot fr>
- To: libc-alpha at sourceware dot org
- Cc: "Albert ARIBAUD (3ADEV)" <albert dot aribaud at 3adev dot fr>
- Date: Sun, 28 Oct 2018 23:52:06 +0100
- Subject: [PATCH v2] Ensure mktime sets errno on error (bug 23789)
Posix mandates that mktime set errno to EOVERFLOW on error, but the
glibc mktime wasn't doing it so far.
Make __mktime_internal set errno=EOVERFLOW on failures to convert, and
make mktime handle errno=EOVERFLOW from __mktime_internal.
Also, add a test to prevent future regressions.
The test was run through 'make check' on i686-linux-gnu,
then the fix was added and 'make check' run again.
* time/Makefile: Add bug-mktime4.
* time/bug-mktime4.c: New file.
* time/mktime.c
(__mktime_internal): Set errno to EOVERFLOW on error.
(mktime): Check for errors from __tzset() or from
__mktime_internal().
---
History:
- 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, so the "save/restore errno"
approach was preferred.
- 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 | 28 ++++++++++++++++++++++++++++
time/mktime.c | 38 ++++++++++++++++++++++++++++++++++----
3 files changed, 63 insertions(+), 5 deletions(-)
create mode 100644 time/bug-mktime4.c
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..dd1e0c76bf
--- /dev/null
+++ b/time/bug-mktime4.c
@@ -0,0 +1,28 @@
+#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;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/time/mktime.c b/time/mktime.c
index 00f0dec6b4..36b35824ff 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,18 +529,41 @@ __mktime_internal (struct tm *tp,
time_t
mktime (struct tm *tp)
{
+ time_t result;
+ /* When __mktime_internal() returns -1, we need to know it it has set
+ * errno (real error) or not (just returning valid time_t value -1),
+ * so we beed to clear errno before calling __mktime_internal().
+ * But we also need to preserve errno if __mktime_internal() does not
+ * modify it, so we need to back up its current value.
+ * Ditto for __tzset(). */
+ int errno_from_tzset;
+ int errno_before_mktime = errno;
+
/* 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. */
+ errno = 0;
__tzset ();
+ /* record errno from __tzset() but do not fail now. */
+ errno_from_tzset = errno;
+ errno = 0;
# if defined _LIBC || NEED_MKTIME_WORKING
static mktime_offset_t localtime_offset;
- return __mktime_internal (tp, __localtime_r, &localtime_offset);
+ result = __mktime_internal (tp, __localtime_r, &localtime_offset);
# else
# undef mktime
- return mktime (tp);
+ result = mktime (tp);
# endif
+ if (result == -1 && errno == 0)
+ return result;
+ else if (errno != 0)
+ return -1;
+ else if (errno_from_tzset != 0)
+ __set_errno(errno_from_tzset);
+ else
+ __set_errno(errno_before_mktime);
+ return result;
}
#endif /* _LIBC || NEED_MKTIME_WORKING || NEED_MKTIME_WINDOWS */
--
2.17.1