[PATCH v4] Ensure mktime sets errno on error (bug 23789)

[PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Albert ARIBAUD (3ADEV)]

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

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Albert ARIBAUD]

On Tue, 30 Oct 2018 12:18:50 +0100, "Albert ARIBAUD (3ADEV)"
<albert.aribaud@3adev.fr> wrote :

> 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.

If this version is fine with everyone, I'll prepare a candidate commit
for master. As this is my first bugzilla related patch, I'll make
the commit available for review before I actually push it onto
master.   

Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Albert ARIBAUD]

Hi all,

On Wed, 31 Oct 2018 16:56:02 +0100, Albert ARIBAUD
<albert.aribaud@3adev.fr> wrote :

> If this version is fine with everyone, I'll prepare a candidate commit
> for master. As this is my first bugzilla related patch, I'll make
> the commit available for review before I actually push it onto
> master.   

From what I read, the only thing I had to do regarding Bugzilla in the
patch was mention the BZ number.

The final to be committed patch is at branch aaribaud/bugzilla/23789/v4
for review.

What should I do regarding Bugzilla itself?

Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Joseph Myers]

On Fri, 2 Nov 2018, Albert ARIBAUD wrote:

> What should I do regarding Bugzilla itself?

https://sourceware.org/glibc/wiki/Bugzilla%20Procedures
https://sourceware.org/glibc/wiki/Committer%20checklist#Update_Bugzilla

The general expectation is that *once a fix has been committed and pushed 
to master*, the bug should be marked as RESOLVED/FIXED with the target 
milestone set to the next mainline release with the fix.  It is the 
committer's responsibility to do that update promptly after committing.  
Committers have @gcc.gnu.org / @sourceware.org addresses and a Bugzilla 
account with such an address should automatically have the required 
permissions to update milestones.  (Milestones should not be set on open 
bugs.  If a fix needs to be reverted for some reason, the bug should be 
reopened and the milestone setting removed.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Albert ARIBAUD]

Hi Joseph,

On Fri, 2 Nov 2018 16:48:25 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :

> On Fri, 2 Nov 2018, Albert ARIBAUD wrote:
> 
> > What should I do regarding Bugzilla itself?  
> 
> https://sourceware.org/glibc/wiki/Bugzilla%20Procedures
> https://sourceware.org/glibc/wiki/Committer%20checklist#Update_Bugzilla
> 
> The general expectation is that *once a fix has been committed and pushed 
> to master*, the bug should be marked as RESOLVED/FIXED with the target 
> milestone set to the next mainline release with the fix.  It is the 
> committer's responsibility to do that update promptly after committing.  
> Committers have @gcc.gnu.org / @sourceware.org addresses and a
> Bugzilla account with such an address should automatically have the
> required permissions to update milestones.  (Milestones should not be
> set on open bugs.  If a fix needs to be reverted for some reason, the
> bug should be reopened and the milestone setting removed.)

I don't have an @gcc.gnu.org or @sourceware.org address; I use my
albert.aribaud@3adev.fr address.

This means I cannot update Bugzilla properly unless my account gets
given the required permissions, right?

Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Joseph Myers]

On Sat, 3 Nov 2018, Albert ARIBAUD wrote:

> I don't have an @gcc.gnu.org or @sourceware.org address; I use my
> albert.aribaud@3adev.fr address.
> 
> This means I cannot update Bugzilla properly unless my account gets
> given the required permissions, right?

You can either have the permissions added to that account, or create an 
aaribaud@gcc.gnu.org / aaribaud@sourceware.org account in Bugzilla.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 2575 bytes --]

[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.

[-- Attachment #2: 0001-mktime-fix-EOVERFLOW-bug-glibc.patch --]
[-- Type: text/x-patch, Size: 3732 bytes --]

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


[-- Attachment #3: 0001-mktime-fix-EOVERFLOW-bug-gnulib.patch --]
[-- Type: text/x-patch, Size: 5488 bytes --]

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

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

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.

[-- Attachment #2: 0001-mktime-fix-EOVERFLOW-bug.patch --]
[-- Type: text/x-patch, Size: 3740 bytes --]

From 11823e3d7b7aa6126607dbdd9c495f344682ea04 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 1/2] 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 ef268e8337..231a69b65a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2018-11-04  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-03  Samuel Thibault  <samuel.thibault@ens-lyon.org>
 
 	* sysdeps/mach/hurd/msync.c: New file.
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


[-- Attachment #3: 0002-mktime-new-test-for-mktime-failure.patch --]
[-- Type: text/x-patch, Size: 4156 bytes --]

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

diff --git a/ChangeLog b/ChangeLog
index 231a69b65a..c5b806a54f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -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]:
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..9c076eb623
--- /dev/null
+++ b/time/bug-mktime4.c
@@ -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

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Albert ARIBAUD]

Hi Joseph,

On Sat, 3 Nov 2018 00:16:04 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :

> On Sat, 3 Nov 2018, Albert ARIBAUD wrote:
> 
> > I don't have an @gcc.gnu.org or @sourceware.org address; I use my
> > albert.aribaud@3adev.fr address.
> > 
> > This means I cannot update Bugzilla properly unless my account gets
> > given the required permissions, right?  
> 
> You can either have the permissions added to that account, or create an 
> aaribaud@gcc.gnu.org / aaribaud@sourceware.org account in Bugzilla.

I'd rather not multiply accounts if I can avoid it. How do I have the
permissions added to albert.aribaud@3adev.fr?

Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Albert ARIBAUD]

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

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Albert ARIBAUD]

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

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Paul Eggert]

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?

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Albert ARIBAUD]

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

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Albert ARIBAUD]

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

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

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.


[-- Attachment #2: 0001-mktime-fix-EOVERFLOW-bug.txt --]
[-- Type: text/plain, Size: 4083 bytes --]

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(-)

diff --git a/ChangeLog b/ChangeLog
index e43fd3e987..9f6d1d4edd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -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
diff --git a/time/mktime.c b/time/mktime.c
index 00f0dec6b4..106b4eac26 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,
@@ -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;
     }
 
-- 
2.19.1


[-- Attachment #3: 0002-mktime-new-test-for-mktime-failure.txt --]
[-- Type: text/plain, Size: 4182 bytes --]

From 423a60057ac9b775f964f7686c0f61000e0a4b03 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 7 Nov 2018 07:52:18 -0800
Subject: [PATCH 2/7] mktime: new test for mktime failure

[BZ#23789]
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          |  7 ++++
 time/Makefile      |  2 +-
 time/bug-mktime4.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+), 1 deletion(-)
 create mode 100644 time/bug-mktime4.c

diff --git a/ChangeLog b/ChangeLog
index 9f6d1d4edd..e92ce16df9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2018-11-09  Paul Eggert  <eggert@cs.ucla.edu>
 
+	mktime: new test for mktime failure
+	[BZ#23789]
+	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]:
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..9c076eb623
--- /dev/null
+++ b/time/bug-mktime4.c
@@ -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


[-- Attachment #4: 0003-mktime-simplify-offset-guess.txt --]
[-- Type: text/plain, Size: 1721 bytes --]

From 48c1f7c67e85c9a23f8ae365627b6210426399fb Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 8 Nov 2018 08:26:21 -0800
Subject: [PATCH 3/7] mktime: simplify offset guess

[BZ#23789]
* time/mktime.c (__mktime_internal): Omit excess precision.
---
 ChangeLog     | 4 ++++
 time/mktime.c | 6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index e92ce16df9..b7008a8972 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
 2018-11-09  Paul Eggert  <eggert@cs.ucla.edu>
 
+	mktime: simplify offset guess
+	[BZ#23789]
+	* time/mktime.c (__mktime_internal): Omit excess precision.
+
 	mktime: new test for mktime failure
 	[BZ#23789]
 	Based on a test suggested by Albert Aribaud in:
diff --git a/time/mktime.c b/time/mktime.c
index 106b4eac26..0f905eb8fe 100644
--- a/time/mktime.c
+++ b/time/mktime.c
@@ -355,7 +355,7 @@ __mktime_internal (struct tm *tp,
 		   struct tm *(*convert) (const time_t *, struct tm *),
 		   mktime_offset_t *offset)
 {
-  long_int t, gt, t0, t1, t2, dt;
+  long_int t, gt, t0, t1, t2;
   struct tm tm;
 
   /* The maximum number of probes (calls to CONVERT) should be enough
@@ -502,8 +502,8 @@ __mktime_internal (struct tm *tp,
   /* Set *OFFSET to the low-order bits of T - T0 - NEGATIVE_OFFSET_GUESS.
      This is just a heuristic to speed up the next mktime call, and
      correctness is unaffected if integer overflow occurs here.  */
-  INT_SUBTRACT_WRAPV (t, t0, &dt);
-  INT_SUBTRACT_WRAPV (dt, negative_offset_guess, offset);
+  INT_SUBTRACT_WRAPV (t, t0, offset);
+  INT_SUBTRACT_WRAPV (*offset, negative_offset_guess, offset);
 
   if (LEAP_SECONDS_POSSIBLE && sec_requested != tm.tm_sec)
     {
-- 
2.19.1


[-- Attachment #5: 0004-mktime-make-more-room-for-overflow.txt --]
[-- Type: text/plain, Size: 3716 bytes --]

From 7678c7757fdf133db5edf17c7b3f6097b76e6ade Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 8 Nov 2018 10:20:50 -0800
Subject: [PATCH 4/7] mktime: make more room for overflow
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

[BZ#23789]
* time/mktime.c (long_int): Now 4⨯ int, not just 3⨯.
This is so that we can add tm_diff results to a previous guess,
which will be useful in a later patch.
---
 ChangeLog     |  6 ++++++
 time/mktime.c | 20 +++++++++++---------
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index b7008a8972..82cdd795ab 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2018-11-09  Paul Eggert  <eggert@cs.ucla.edu>
 
+	mktime: make more room for overflow
+	[BZ#23789]
+	* time/mktime.c (long_int): Now 4⨯ int, not just 3⨯.
+	This is so that we can add tm_diff results to a previous guess,
+	which will be useful in a later patch.
+
 	mktime: simplify offset guess
 	[BZ#23789]
 	* time/mktime.c (__mktime_internal): Omit excess precision.
diff --git a/time/mktime.c b/time/mktime.c
index 0f905eb8fe..ffbb5ea171 100644
--- a/time/mktime.c
+++ b/time/mktime.c
@@ -120,11 +120,12 @@ my_tzset (void)
 #if defined _LIBC || NEED_MKTIME_WORKING || NEED_MKTIME_INTERNAL
 
 /* A signed type that can represent an integer number of years
-   multiplied by three times the number of seconds in a year.  It is
+   multiplied by four times the number of seconds in a year.  It is
    needed when converting a tm_year value times the number of seconds
-   in a year.  The factor of three comes because these products need
+   in a year.  The factor of four comes because these products need
    to be subtracted from each other, and sometimes with an offset
-   added to them, without worrying about overflow.
+   added to them, and then with another timestamp added, without
+   worrying about overflow.
 
    Much of the code uses long_int to represent time_t values, to
    lessen the hassle of dealing with platforms where time_t is
@@ -132,12 +133,12 @@ my_tzset (void)
    time_t values that mktime can generate even on platforms where
    time_t is excessively wide.  */
 
-#if INT_MAX <= LONG_MAX / 3 / 366 / 24 / 60 / 60
+#if INT_MAX <= LONG_MAX / 4 / 366 / 24 / 60 / 60
 typedef long int long_int;
 #else
 typedef long long int long_int;
 #endif
-verify (INT_MAX <= TYPE_MAXIMUM (long_int) / 3 / 366 / 24 / 60 / 60);
+verify (INT_MAX <= TYPE_MAXIMUM (long_int) / 4 / 366 / 24 / 60 / 60);
 
 /* Shift A right by B bits portably, by dividing A by 2**B and
    truncating towards minus infinity.  B should be in the range 0 <= B
@@ -211,9 +212,10 @@ isdst_differ (int a, int b)
    were not adjusted between the timestamps.
 
    The YEAR values uses the same numbering as TP->tm_year.  Values
-   need not be in the usual range.  However, YEAR1 must not overflow
-   when multiplied by three times the number of seconds in a year, and
-   likewise for YDAY1 and three times the number of seconds in a day.  */
+   need not be in the usual range.  However, YEAR1 - YEAR0 must not
+   overflow even when multiplied by three times the number of seconds
+   in a year, and likewise for YDAY1 - YDAY0 and three times the
+   number of seconds in a day.  */
 
 static long_int
 ydhms_diff (long_int year1, long_int yday1, int hour1, int min1, int sec1,
@@ -403,7 +405,7 @@ __mktime_internal (struct tm *tp,
   if (LEAP_SECONDS_POSSIBLE)
     {
       /* Handle out-of-range seconds specially,
-	 since ydhms_tm_diff assumes every minute has 60 seconds.  */
+	 since ydhms_diff assumes every minute has 60 seconds.  */
       if (sec < 0)
 	sec = 0;
       if (59 < sec)
-- 
2.19.1


[-- Attachment #6: 0005-mktime-fix-bug-with-Y2038-DST-transition.txt --]
[-- Type: text/plain, Size: 1846 bytes --]

From 19a49e00d5fab2ac24d36900dc810407fd05d12a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 8 Nov 2018 10:30:36 -0800
Subject: [PATCH 5/7] mktime: fix bug with Y2038 DST transition
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

[BZ#23789]
* time/mktime.c (ranged_convert): On 32-bit platforms, don’t
mishandle a DST transition that jumps over the Y2038 boundary.
No such DST transitions are known so this is only a theoretical
bug, but we might as well do things right.
---
 ChangeLog     | 7 +++++++
 time/mktime.c | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 82cdd795ab..939ca140ef 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2018-11-09  Paul Eggert  <eggert@cs.ucla.edu>
 
+	mktime: fix bug with Y2038 DST transition
+	[BZ#23789]
+	* time/mktime.c (ranged_convert): On 32-bit platforms, don’t
+	mishandle a DST transition that jumps over the Y2038 boundary.
+	No such DST transitions are known so this is only a theoretical
+	bug, but we might as well do things right.
+
 	mktime: make more room for overflow
 	[BZ#23789]
 	* time/mktime.c (long_int): Now 4⨯ int, not just 3⨯.
diff --git a/time/mktime.c b/time/mktime.c
index ffbb5ea171..6d5b8cf838 100644
--- a/time/mktime.c
+++ b/time/mktime.c
@@ -323,7 +323,7 @@ ranged_convert (struct tm *(*convert) (const time_t *, struct tm *),
       while (true)
 	{
 	  long_int mid = long_int_avg (ok, bad);
-	  if (mid != ok && mid != bad)
+	  if (mid == ok || mid == bad)
 	    break;
 	  r = convert_time (convert, mid, tp);
 	  if (r)
@@ -332,6 +332,8 @@ ranged_convert (struct tm *(*convert) (const time_t *, struct tm *),
 	    bad = mid;
 	}
 
+      *t = ok;
+
       if (!r && ok)
 	{
 	  /* The last conversion attempt failed;
-- 
2.19.1


[-- Attachment #7: 0006-mktime-fix-non-EOVERFLOW-errno-handling.txt --]
[-- Type: text/plain, Size: 11032 bytes --]

From 67ece14d68a5b086f2b4a04d27b0d48f3f47bea3 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 9 Nov 2018 17:33:24 -0800
Subject: [PATCH 6/7] mktime: fix non-EOVERFLOW errno handling

[BZ#23789]
mktime was not properly reporting failures when the underlying
localtime_r fails with errno != EOVERFLOW; it incorrectly treated
them like EOVERFLOW failures, and set errno to EOVERFLOW.
The problem could happen on non-glibc platforms, with Gnulib.
* time/mktime.c (guess_time_tm): Remove, replacing with ...
(tm_diff): ... this simpler function, which does not change errno.
All callers changed to deal with errno themselves.
(ranged_convert, __mktime_internal): Return failure immediately if
the underlying function reports any failure other than EOVERFLOW.
(__mktime_internal): Set errno to EOVERFLOW if the spring-forward
gap code fails.
---
 ChangeLog     |  14 ++++
 time/mktime.c | 197 +++++++++++++++++++++++++-------------------------
 2 files changed, 113 insertions(+), 98 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 939ca140ef..9f62ab865e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,19 @@
 2018-11-09  Paul Eggert  <eggert@cs.ucla.edu>
 
+	mktime: fix non-EOVERFLOW errno handling
+	[BZ#23789]
+	mktime was not properly reporting failures when the underlying
+	localtime_r fails with errno != EOVERFLOW; it incorrectly treated
+	them like EOVERFLOW failures, and set errno to EOVERFLOW.
+	The problem could happen on non-glibc platforms, with Gnulib.
+	* time/mktime.c (guess_time_tm): Remove, replacing with ...
+	(tm_diff): ... this simpler function, which does not change errno.
+	All callers changed to deal with errno themselves.
+	(ranged_convert, __mktime_internal): Return failure immediately if
+	the underlying function reports any failure other than EOVERFLOW.
+	(__mktime_internal): Set errno to EOVERFLOW if the spring-forward
+	gap code fails.
+
 	mktime: fix bug with Y2038 DST transition
 	[BZ#23789]
 	* time/mktime.c (ranged_convert): On 32-bit platforms, don’t
diff --git a/time/mktime.c b/time/mktime.c
index 6d5b8cf838..dc83985bbd 100644
--- a/time/mktime.c
+++ b/time/mktime.c
@@ -250,45 +250,25 @@ long_int_avg (long_int a, long_int b)
   return shr (a, 1) + shr (b, 1) + ((a | b) & 1);
 }
 
-/* Return a time_t value corresponding to (YEAR-YDAY HOUR:MIN:SEC),
-   assuming that T corresponds to *TP and that no clock adjustments
-   occurred between *TP and the desired time.
-   Although T and the returned value are of type long_int,
-   they represent time_t values and must be in time_t range.
-   If TP is null, return a value not equal to T; this avoids false matches.
+/* Return a long_int value corresponding to (YEAR-YDAY HOUR:MIN:SEC)
+   minus *TP seconds, assuming no clock adjustments occurred between
+   the two timestamps.
+
    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 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.  */
+   *TP should be in the usual range.  */
 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)
+tm_diff (long_int year, long_int yday, int hour, int min, int sec,
+	 struct tm const *tp)
 {
-  if (tp)
-    {
-      long_int result;
-      long_int d = ydhms_diff (year, yday, hour, min, sec,
-			       tp->tm_year, tp->tm_yday,
-			       tp->tm_hour, tp->tm_min, tp->tm_sec);
-      if (! INT_ADD_WRAPV (t, d, &result))
-	return result;
-      __set_errno (EOVERFLOW);
-    }
-
-  /* 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
-     confuse the spring-forward gap detector.  */
-  return (t < long_int_avg (mktime_min, mktime_max)
-	  ? (t <= mktime_min + 1 ? t + 1 : mktime_min)
-	  : (mktime_max - 1 <= t ? t - 1 : mktime_max));
+  return ydhms_diff (year, yday, hour, min, sec,
+		     tp->tm_year, tp->tm_yday,
+		     tp->tm_hour, tp->tm_min, tp->tm_sec);
 }
 
 /* Use CONVERT to convert T to a struct tm value in *TM.  T must be in
-   range for time_t.  Return TM if successful, NULL if T is out of
-   range for CONVERT.  */
+   range for time_t.  Return TM if successful, NULL (setting errno) on
+   failure.  */
 static struct tm *
 convert_time (struct tm *(*convert) (const time_t *, struct tm *),
 	      long_int t, struct tm *tm)
@@ -300,49 +280,48 @@ convert_time (struct tm *(*convert) (const time_t *, struct tm *),
 /* Use CONVERT to convert *T to a broken down time in *TP.
    If *T is out of range for conversion, adjust it so that
    it is the nearest in-range value and then convert that.
-   A value is in range if it fits in both time_t and long_int.  */
+   A value is in range if it fits in both time_t and long_int.
+   Return TP on success, NULL (setting errno) on failure.  */
 static struct tm *
 ranged_convert (struct tm *(*convert) (const time_t *, struct tm *),
 		long_int *t, struct tm *tp)
 {
-  struct tm *r;
-  if (*t < mktime_min)
-    *t = mktime_min;
-  else if (mktime_max < *t)
-    *t = mktime_max;
-  r = convert_time (convert, *t, tp);
-
-  if (!r && *t)
+  long_int t1 = (*t < mktime_min ? mktime_min
+		 : *t <= mktime_max ? *t : mktime_max);
+  struct tm *r = convert_time (convert, t1, tp);
+  if (r)
     {
-      long_int bad = *t;
-      long_int ok = 0;
-
-      /* BAD is a known unconvertible value, and OK is a known good one.
-	 Use binary search to narrow the range between BAD and OK until
-	 they differ by 1.  */
-      while (true)
-	{
-	  long_int mid = long_int_avg (ok, bad);
-	  if (mid == ok || mid == bad)
-	    break;
-	  r = convert_time (convert, mid, tp);
-	  if (r)
-	    ok = mid;
-	  else
-	    bad = mid;
-	}
+      *t = t1;
+      return r;
+    }
+  if (errno != EOVERFLOW)
+    return NULL;
 
-      *t = ok;
+  long_int bad = t1;
+  long_int ok = 0;
+  struct tm oktm; oktm.tm_sec = -1;
 
-      if (!r && ok)
-	{
-	  /* The last conversion attempt failed;
-	     revert to the most recent successful attempt.  */
-	  r = convert_time (convert, ok, tp);
-	}
+  /* BAD is a known out-of-range value, and OK is a known in-range one.
+     Use binary search to narrow the range between BAD and OK until
+     they differ by 1.  */
+  while (true)
+    {
+      long_int mid = long_int_avg (ok, bad);
+      if (mid == ok || mid == bad)
+	break;
+      if (convert_time (convert, mid, tp))
+	ok = mid, oktm = *tp;
+      else if (errno != EOVERFLOW)
+	return NULL;
+      else
+	bad = mid;
     }
 
-  return r;
+  if (oktm.tm_sec < 0)
+    return NULL;
+  *t = ok;
+  *tp = oktm;
+  return tp;
 }
 
 
@@ -359,7 +338,6 @@ __mktime_internal (struct tm *tp,
 		   struct tm *(*convert) (const time_t *, struct tm *),
 		   mktime_offset_t *offset)
 {
-  long_int t, gt, t0, t1, t2;
   struct tm tm;
 
   /* The maximum number of probes (calls to CONVERT) should be enough
@@ -379,7 +357,7 @@ __mktime_internal (struct tm *tp,
   int isdst = tp->tm_isdst;
 
   /* 1 if the previous probe was DST.  */
-  int dst2;
+  int dst2 = 0;
 
   /* Ensure that mon is in range, and set year accordingly.  */
   int mon_remainder = mon % 12;
@@ -418,36 +396,46 @@ __mktime_internal (struct tm *tp,
      time.  */
 
   INT_SUBTRACT_WRAPV (0, off, &negative_offset_guess);
-  t0 = ydhms_diff (year, yday, hour, min, sec,
-		   EPOCH_YEAR - TM_YEAR_BASE, 0, 0, 0, negative_offset_guess);
+  long_int t0 = ydhms_diff (year, yday, hour, min, sec,
+			    EPOCH_YEAR - TM_YEAR_BASE, 0, 0, 0,
+			    negative_offset_guess);
+  long_int t = t0, t1 = t0, t2 = t0;
 
   /* Repeatedly use the error to improve the guess.  */
 
-  for (t = t1 = t2 = t0, dst2 = 0;
-       (gt = guess_time_tm (year, yday, hour, min, sec, t,
-			    ranged_convert (convert, &t, &tm)),
-	t != gt);
-       t1 = t2, t2 = t, t = gt, dst2 = tm.tm_isdst != 0)
-    if (t == t1 && t != t2
-	&& (tm.tm_isdst < 0
-	    || (isdst < 0
-		? dst2 <= (tm.tm_isdst != 0)
-		: (isdst != 0) != (tm.tm_isdst != 0))))
-      /* We can't possibly find a match, as we are oscillating
-	 between two values.  The requested time probably falls
-	 within a spring-forward gap of size GT - T.  Follow the common
-	 practice in this case, which is to return a time that is GT - T
-	 away from the requested time, preferring a time whose
-	 tm_isdst differs from the requested value.  (If no tm_isdst
-	 was requested and only one of the two values has a nonzero
-	 tm_isdst, prefer that value.)  In practice, this is more
-	 useful than returning -1.  */
-      goto offset_found;
-    else if (--remaining_probes == 0)
-      {
-	__set_errno (EOVERFLOW);
+  while (true)
+    {
+      if (! ranged_convert (convert, &t, &tm))
 	return -1;
-      }
+      long_int dt = tm_diff (year, yday, hour, min, sec, &tm);
+      if (dt == 0)
+	break;
+
+      if (t == t1 && t != t2
+	  && (tm.tm_isdst < 0
+	      || (isdst < 0
+		  ? dst2 <= (tm.tm_isdst != 0)
+		  : (isdst != 0) != (tm.tm_isdst != 0))))
+	/* We can't possibly find a match, as we are oscillating
+	   between two values.  The requested time probably falls
+	   within a spring-forward gap of size DT.  Follow the common
+	   practice in this case, which is to return a time that is DT
+	   away from the requested time, preferring a time whose
+	   tm_isdst differs from the requested value.  (If no tm_isdst
+	   was requested and only one of the two values has a nonzero
+	   tm_isdst, prefer that value.)  In practice, this is more
+	   useful than returning -1.  */
+	goto offset_found;
+
+      remaining_probes--;
+      if (remaining_probes == 0)
+	{
+	  __set_errno (EOVERFLOW);
+	  return -1;
+	}
+
+      t1 = t2, t2 = t, t += dt, dst2 = tm.tm_isdst != 0;
+    }
 
   /* We have a match.  Check whether tm.tm_isdst has the requested
      value, if any.  */
@@ -489,17 +477,30 @@ __mktime_internal (struct tm *tp,
 	    if (! INT_ADD_WRAPV (t, delta * direction, &ot))
 	      {
 		struct tm otm;
-		ranged_convert (convert, &ot, &otm);
+		if (! ranged_convert (convert, &ot, &otm))
+		  return -1;
 		if (! isdst_differ (isdst, otm.tm_isdst))
 		  {
 		    /* We found the desired tm_isdst.
 		       Extrapolate back to the desired time.  */
-		    t = guess_time_tm (year, yday, hour, min, sec, ot, &otm);
-		    ranged_convert (convert, &t, &tm);
-		    goto offset_found;
+		    long_int gt = ot + tm_diff (year, yday, hour, min, sec,
+						&otm);
+		    if (mktime_min <= gt && gt <= mktime_max)
+		      {
+			if (convert_time (convert, gt, &tm))
+			  {
+			    t = gt;
+			    goto offset_found;
+			  }
+			if (errno != EOVERFLOW)
+			  return -1;
+		      }
 		  }
 	      }
 	  }
+
+      __set_errno (EOVERFLOW);
+      return -1;
     }
 
  offset_found:
-- 
2.19.1


[-- Attachment #8: 0007-mktime-DEBUG_MKTIME-cleanup.txt --]
[-- Type: text/plain, Size: 6114 bytes --]

From c1d348099f2253e18437e711c4de3d886f9bb8ca Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 9 Nov 2018 17:43:48 -0800
Subject: [PATCH 7/7] mktime: DEBUG_MKTIME cleanup
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The DEBUG_MKTIME code no longer works in glibc or in Gnulib.
And it’s no longer needed now that glibc and Gnulib both have
their own testing mechanisms for mktime.
* time/mktime.c (DEBUG_MKTIME): Remove.  All uses removed.
---
 ChangeLog     |   6 ++
 time/mktime.c | 162 +-------------------------------------------------
 2 files changed, 8 insertions(+), 160 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 9f62ab865e..e039728000 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2018-11-09  Paul Eggert  <eggert@cs.ucla.edu>
 
+	mktime: DEBUG_MKTIME cleanup
+	The DEBUG_MKTIME code no longer works in glibc or in Gnulib.
+	And it’s no longer needed now that glibc and Gnulib both have
+	their own testing mechanisms for mktime.
+	* time/mktime.c (DEBUG_MKTIME): Remove.  All uses removed.
+
 	mktime: fix non-EOVERFLOW errno handling
 	[BZ#23789]
 	mktime was not properly reporting failures when the underlying
diff --git a/time/mktime.c b/time/mktime.c
index dc83985bbd..8faa9bc93d 100644
--- a/time/mktime.c
+++ b/time/mktime.c
@@ -17,12 +17,6 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-/* Define this to 1 to have a standalone program to test this implementation of
-   mktime.  */
-#ifndef DEBUG_MKTIME
-# define DEBUG_MKTIME 0
-#endif
-
 /* The following macros influence what gets defined when this file is compiled:
 
    Macro/expression            Which gnulib module    This compilation unit
@@ -34,11 +28,9 @@
    || NEED_MKTIME_WINDOWS
 
    NEED_MKTIME_INTERNAL        mktime-internal        mktime_internal
-
-   DEBUG_MKTIME                (defined manually)     my_mktime, main
  */
 
-#if !defined _LIBC && !DEBUG_MKTIME
+#ifndef _LIBC
 # include <libc-config.h>
 #endif
 
@@ -60,13 +52,6 @@
 #include <intprops.h>
 #include <verify.h>
 
-#if DEBUG_MKTIME
-# include <stdio.h>
-/* Make it work even if the system's libc has its own mktime routine.  */
-# undef mktime
-# define mktime my_mktime
-#endif /* DEBUG_MKTIME */
-
 #ifndef NEED_MKTIME_INTERNAL
 # define NEED_MKTIME_INTERNAL 0
 #endif
@@ -74,7 +59,7 @@
 # define NEED_MKTIME_WINDOWS 0
 #endif
 #ifndef NEED_MKTIME_WORKING
-# define NEED_MKTIME_WORKING DEBUG_MKTIME
+# define NEED_MKTIME_WORKING 0
 #endif
 
 #include "mktime-internal.h"
@@ -562,146 +547,3 @@ weak_alias (mktime, timelocal)
 libc_hidden_def (mktime)
 libc_hidden_weak (timelocal)
 #endif
-\f
-#if DEBUG_MKTIME
-
-static int
-not_equal_tm (const struct tm *a, const struct tm *b)
-{
-  return ((a->tm_sec ^ b->tm_sec)
-	  | (a->tm_min ^ b->tm_min)
-	  | (a->tm_hour ^ b->tm_hour)
-	  | (a->tm_mday ^ b->tm_mday)
-	  | (a->tm_mon ^ b->tm_mon)
-	  | (a->tm_year ^ b->tm_year)
-	  | (a->tm_yday ^ b->tm_yday)
-	  | isdst_differ (a->tm_isdst, b->tm_isdst));
-}
-
-static void
-print_tm (const struct tm *tp)
-{
-  if (tp)
-    printf ("%04d-%02d-%02d %02d:%02d:%02d yday %03d wday %d isdst %d",
-	    tp->tm_year + TM_YEAR_BASE, tp->tm_mon + 1, tp->tm_mday,
-	    tp->tm_hour, tp->tm_min, tp->tm_sec,
-	    tp->tm_yday, tp->tm_wday, tp->tm_isdst);
-  else
-    printf ("0");
-}
-
-static int
-check_result (time_t tk, struct tm tmk, time_t tl, const struct tm *lt)
-{
-  if (tk != tl || !lt || not_equal_tm (&tmk, lt))
-    {
-      printf ("mktime (");
-      print_tm (lt);
-      printf (")\nyields (");
-      print_tm (&tmk);
-      printf (") == %ld, should be %ld\n", (long int) tk, (long int) tl);
-      return 1;
-    }
-
-  return 0;
-}
-
-int
-main (int argc, char **argv)
-{
-  int status = 0;
-  struct tm tm, tmk, tml;
-  struct tm *lt;
-  time_t tk, tl, tl1;
-  char trailer;
-
-  /* Sanity check, plus call tzset.  */
-  tl = 0;
-  if (! localtime (&tl))
-    {
-      printf ("localtime (0) fails\n");
-      status = 1;
-    }
-
-  if ((argc == 3 || argc == 4)
-      && (sscanf (argv[1], "%d-%d-%d%c",
-		  &tm.tm_year, &tm.tm_mon, &tm.tm_mday, &trailer)
-	  == 3)
-      && (sscanf (argv[2], "%d:%d:%d%c",
-		  &tm.tm_hour, &tm.tm_min, &tm.tm_sec, &trailer)
-	  == 3))
-    {
-      tm.tm_year -= TM_YEAR_BASE;
-      tm.tm_mon--;
-      tm.tm_isdst = argc == 3 ? -1 : atoi (argv[3]);
-      tmk = tm;
-      tl = mktime (&tmk);
-      lt = localtime_r (&tl, &tml);
-      printf ("mktime returns %ld == ", (long int) tl);
-      print_tm (&tmk);
-      printf ("\n");
-      status = check_result (tl, tmk, tl, lt);
-    }
-  else if (argc == 4 || (argc == 5 && strcmp (argv[4], "-") == 0))
-    {
-      time_t from = atol (argv[1]);
-      time_t by = atol (argv[2]);
-      time_t to = atol (argv[3]);
-
-      if (argc == 4)
-	for (tl = from; by < 0 ? to <= tl : tl <= to; tl = tl1)
-	  {
-	    lt = localtime_r (&tl, &tml);
-	    if (lt)
-	      {
-		tmk = tml;
-		tk = mktime (&tmk);
-		status |= check_result (tk, tmk, tl, &tml);
-	      }
-	    else
-	      {
-		printf ("localtime_r (%ld) yields 0\n", (long int) tl);
-		status = 1;
-	      }
-	    tl1 = tl + by;
-	    if ((tl1 < tl) != (by < 0))
-	      break;
-	  }
-      else
-	for (tl = from; by < 0 ? to <= tl : tl <= to; tl = tl1)
-	  {
-	    /* Null benchmark.  */
-	    lt = localtime_r (&tl, &tml);
-	    if (lt)
-	      {
-		tmk = tml;
-		tk = tl;
-		status |= check_result (tk, tmk, tl, &tml);
-	      }
-	    else
-	      {
-		printf ("localtime_r (%ld) yields 0\n", (long int) tl);
-		status = 1;
-	      }
-	    tl1 = tl + by;
-	    if ((tl1 < tl) != (by < 0))
-	      break;
-	  }
-    }
-  else
-    printf ("Usage:\
-\t%s YYYY-MM-DD HH:MM:SS [ISDST] # Test given time.\n\
-\t%s FROM BY TO # Test values FROM, FROM+BY, ..., TO.\n\
-\t%s FROM BY TO - # Do not test those values (for benchmark).\n",
-	    argv[0], argv[0], argv[0]);
-
-  return status;
-}
-
-#endif /* DEBUG_MKTIME */
-\f
-/*
-Local Variables:
-compile-command: "gcc -DDEBUG_MKTIME -I. -Wall -W -O2 -g mktime.c -o mktime"
-End:
-*/
-- 
2.19.1

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Albert ARIBAUD]

Hi Albert,

On Mon, 5 Nov 2018 21:29:38 +0100, Albert ARIBAUD
<albert.aribaud@3adev.fr> wrote :

> Hi Joseph,
> 
> On Sat, 3 Nov 2018 00:16:04 +0000, Joseph Myers
> <joseph@codesourcery.com> wrote :
> 
> > On Sat, 3 Nov 2018, Albert ARIBAUD wrote:
> >   
> > > I don't have an @gcc.gnu.org or @sourceware.org address; I use my
> > > albert.aribaud@3adev.fr address.
> > > 
> > > This means I cannot update Bugzilla properly unless my account gets
> > > given the required permissions, right?    
> > 
> > You can either have the permissions added to that account, or create an 
> > aaribaud@gcc.gnu.org / aaribaud@sourceware.org account in Bugzilla.  
> 
> I'd rather not multiply accounts if I can avoid it. How do I have the
> permissions added to albert.aribaud@3adev.fr?

Ping?
 
Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Albert ARIBAUD]

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

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Albert ARIBAUD]

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

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Joseph Myers]

On Tue, 13 Nov 2018, Albert ARIBAUD wrote:

> > I'd rather not multiply accounts if I can avoid it. How do I have the
> > permissions added to albert.aribaud@3adev.fr?
> 
> Ping?

I've added you to the editbugs group.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v4] Ensure mktime sets errno on error (bug 23789)

[Albert ARIBAUD]

Hi Joseph,

On Thu, 15 Nov 2018 17:01:38 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :

> On Tue, 13 Nov 2018, Albert ARIBAUD wrote:
> 
> > > I'd rather not multiply accounts if I can avoid it. How do I have the
> > > permissions added to albert.aribaud@3adev.fr?  
> > 
> > Ping?  
> 
> I've added you to the editbugs group.

Thanks!

Cordialement,
Albert ARIBAUD
3ADEV