[v4] Ensure mktime sets errno on error (bug 23789)
Commit Message
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
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
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
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.)
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
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.
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
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
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.
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
@@ -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
new file mode 100644
@@ -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"
@@ -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