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

Message ID 20181030111850.5322-1-albert.aribaud@3adev.fr
State New
Headers show
Series
  • [v4] Ensure mktime sets errno on error (bug 23789)
Related show

Commit Message

Albert ARIBAUD (3ADEV) Oct. 30, 2018, 11:18 a.m.
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

-- 
2.17.1

Comments

Albert ARIBAUD (3ADEV) Oct. 31, 2018, 3:56 p.m. | #1
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
Albert ARIBAUD (3ADEV) Nov. 2, 2018, 6:07 a.m. | #2
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
Joseph Myers Nov. 2, 2018, 4:48 p.m. | #3
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
Albert ARIBAUD (3ADEV) Nov. 3, 2018, 12:01 a.m. | #4
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
Joseph Myers Nov. 3, 2018, 12:16 a.m. | #5
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
Paul Eggert Nov. 3, 2018, 2:13 a.m. | #6
[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
Paul Eggert Nov. 5, 2018, 7:54 a.m. | #7
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.
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
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
Albert ARIBAUD (3ADEV) Nov. 5, 2018, 8:29 p.m. | #8
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
Albert ARIBAUD (3ADEV) Nov. 6, 2018, 5:43 a.m. | #9
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
Albert ARIBAUD (3ADEV) Nov. 6, 2018, 8:41 p.m. | #10
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
Paul Eggert Nov. 7, 2018, 12:28 a.m. | #11
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?
Albert ARIBAUD (3ADEV) Nov. 7, 2018, 12:39 p.m. | #12
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
Albert ARIBAUD (3ADEV) Nov. 7, 2018, 2:48 p.m. | #13
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
Paul Eggert Nov. 10, 2018, 2 a.m. | #14
On 11/7/18 6:48 AM, Albert ARIBAUD wrote:

> 1. If the for-loop reaches remaining_probes==0, then it really should

>     set errno = EOVERFLOW before returning -1, because remaining_probes

>     is only decremented in the else clause inside the for-loop, and that

>     only happens (or should only happen) when there were no failures so

>     far, so if we fail then, we have to set errno.


Thanks for the diagnosis. Revised patches attached, which set errno in 
that case as you suggested.


> 2. It is not normal that t, gt, t1 and t2 remain the same for all six

>     iterations of the for-loop. That should be investigated and fixed.


Long ago I came up with weird scenarios involving odd combinations of 
leap seconds and DST transitions all near the maximum convertible time_t 
values that could involve that many iterations. None of these scenarios 
will ever happen, really; the number is that large just to be safe. It 
could be that I overestimated the number, but that's no big deal.


> I don't know why ranged_convert alters an argument which should be

> a pure imput. In fact, I don't know why it does not just copy this

> argument into a local time_t. Any known reason?

Because it communicates back to the caller the nearest long_int value 
that is in range. This value is not obvious because it can depend on 
timezone and leap second information.

After looking at the mktime implementation again I see some other things 
that need fixing. These are mostly for Gnulib (when we can't assume that 
localtime_r fails only due to EOVERFLOW), but there are some code 
cleanups and fixes for very unlikely bugs. Proposed glibc patches attached.
From d7b1a2c6fda647672fd7774fc70cbe0b797af4e5 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 7 Nov 2018 07:52:17 -0800
Subject: [PATCH 1/7] mktime: fix EOVERFLOW bug

[BZ#23789]
* time/mktime.c [!_LIBC && !DEBUG_MKTIME]:
Include libc-config.h, not config.h, for __set_errno.
(guess_time_tm, __mktime_internal): Set errno to EOVERFLOW on overflow.
---
 ChangeLog     |  8 ++++++++
 time/mktime.c | 26 +++++++++++++++++++-------
 2 files changed, 27 insertions(+), 7 deletions(-)

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;
     }
Albert ARIBAUD (3ADEV) Nov. 13, 2018, 10:54 p.m. | #15
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
Albert ARIBAUD (3ADEV) Nov. 13, 2018, 11:04 p.m. | #16
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
Albert ARIBAUD (3ADEV) Nov. 13, 2018, 11:24 p.m. | #17
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
Joseph Myers Nov. 15, 2018, 5:01 p.m. | #18
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
Albert ARIBAUD (3ADEV) Nov. 15, 2018, 9:53 p.m. | #19
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

Patch

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