diff mbox

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

Message ID 20181030111850.5322-1-albert.aribaud@3adev.fr
State New
Headers show

Commit Message

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

Comments

Albert ARIBAUD Oct. 31, 2018, 3:56 p.m. UTC | #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 Nov. 2, 2018, 6:07 a.m. UTC | #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. UTC | #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.)
Albert ARIBAUD Nov. 3, 2018, 12:01 a.m. UTC | #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. UTC | #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.
Albert ARIBAUD Nov. 5, 2018, 8:29 p.m. UTC | #6
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 Nov. 13, 2018, 10:54 p.m. UTC | #7
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
Joseph Myers Nov. 15, 2018, 5:01 p.m. UTC | #8
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.
Albert ARIBAUD Nov. 15, 2018, 9:53 p.m. UTC | #9
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
diff mbox

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