This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH v4] 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.

Fix this and add a test to prevent regressions.
The fix also fixes the same issue in timegm.

Tested with 'make check' on x86_64-linux-gnu and
i686-linux-gnu.

        * time/Makefile: Add bug-mktime4.
	* time/bug-mktime4.c: New file.
	* time/mktime.c
	(__mktime_internal): Set errno to EOVERFLOW on error.
	(mktime): Move call to __tzset inside conditional.
---
History:
- v4
  - no source code change.
  - patch run through 'make check' on x86_64-linux-gnu in addition to
    i686-linux-gnu.
- v3:
  - time/tst-mktime4.c: switch to support/test-driver.
  - time/mktime: remove useless errno handling and move call to __tzset
    insde conditional.
- 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.
- 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 | 27 +++++++++++++++++++++++++++
 time/mktime.c      | 16 +++++++++++-----
 3 files changed, 39 insertions(+), 6 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..14d04c669b
--- /dev/null
+++ b/time/bug-mktime4.c
@@ -0,0 +1,27 @@
+#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;
+}
+
+#include "support/test-driver.c"
diff --git a/time/mktime.c b/time/mktime.c
index 00f0dec6b4..2e0c467147 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,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
-- 
2.17.1


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]