[v2] 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.
Make __mktime_internal set errno=EOVERFLOW on failures to convert, and
make mktime handle errno=EOVERFLOW from __mktime_internal.
Also, add a test to prevent future regressions.
The test was run through 'make check' on i686-linux-gnu,
then the fix was added and 'make check' run again.
* time/Makefile: Add bug-mktime4.
* time/bug-mktime4.c: New file.
* time/mktime.c
(__mktime_internal): Set errno to EOVERFLOW on error.
(mktime): Check for errors from __tzset() or from
__mktime_internal().
---
History:
- 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, so the "save/restore errno"
approach was preferred.
- 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 | 28 ++++++++++++++++++++++++++++
time/mktime.c | 38 ++++++++++++++++++++++++++++++++++----
3 files changed, 63 insertions(+), 5 deletions(-)
create mode 100644 time/bug-mktime4.c
Comments
Albert ARIBAUD (3ADEV) wrote:
> __tzset ();
> + /* record errno from __tzset() but do not fail now. */
> + errno_from_tzset = errno;
Why not fail now? If tzset fails, mktime is pointless.
Also, remember this code is shared with Gnulib, and POSIX does not specify what
tzset does to errno. So you can't rely on tzset setting errno, at least not in
the Gnulib usage of this code. And can you even rely on glibc __tzset doing it?
It's not part of tzset's spec, surely.
I thought the idea was to define a glibc-specific tzset variant that returned an
error number, or something like that. I don't see that here.
Also, how about moving the tzset call into the # if defined _LIBC ||
NEED_MKTIME_WORKING branch? tzset shouldn't be needed otherwise.
> + if (result == -1 && errno == 0)
> + return result;> + errno = 0;
> + else if (errno != 0)
> + return -1;
> + else if (errno_from_tzset != 0)
> + __set_errno(errno_from_tzset);
> + else
> + __set_errno(errno_before_mktime);
> + return result;
Sorry, but this is all too complicated. Remind me again why we can't rely on
__mktime_internal to set errno on failure.
We don't care what happens to errno when __mktime_internal returns any value
other than -1. Does that help?
I was hoping that mktime itself could look something like the following, which
would be a lot simpler. Of course __tzset_with_result would have to be written
(both in the Gnulib and the glibc case; the Gnulib version is easy), and
__mktime_internal would have to do the right thing with errno, but this is the idea.
time_t
mktime (struct tm *tp)
{
int r = __tzset_with_result ();
if (r < 0)
return r;
# if defined _LIBC || NEED_MKTIME_WORKING
static mktime_offset_t localtime_offset;
return __mktime_internal (tp, __localtime_r, &localtime_offset);
# else
# undef mktime
return mktime (tp);
# endif
}
On Okt 28 2018, "Albert ARIBAUD (3ADEV)" <albert.aribaud@3adev.fr> wrote:
> + /* When __mktime_internal() returns -1, we need to know it it has set
> + * errno (real error) or not (just returning valid time_t value -1),
> + * so we beed to clear errno before calling __mktime_internal().
> + * But we also need to preserve errno if __mktime_internal() does not
> + * modify it, so we need to back up its current value.
> + * Ditto for __tzset(). */
Style: GNU comment style. No paren after function name.
Andreas.
On Sun, 28 Oct 2018, Albert ARIBAUD (3ADEV) wrote:
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
New tests should use <support/test-driver.c>, not the old test-skeleton.
> + if (result == -1 && errno == 0)
> + return result;
It is never OK for a library function to set errno to 0 (return with errno
set to 0 when it wasn't 0 on entry); if there is no error you can restore
the value that was set on entry to mktime, or set errno to some spurious
other value despite the lack of an error (maybe not a good idea in this
case, because it prevents callers from distinguishing error and non-error
-1), but not set it to 0 if it wasn't 0 on entry to the library function.
(See ISO C: "The value of errno in the initial thread is zero at program
startup (the initial value of errno in other threads is an indeterminate
value), but is never set to zero by any library function.".)
> + __set_errno(errno_from_tzset);
Missing space before '(', here and elsewhere in this patch.
Hi Paul,
On Mon, 29 Oct 2018 00:05:12 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :
> Albert ARIBAUD (3ADEV) wrote:
>
> > __tzset ();
> > + /* record errno from __tzset() but do not fail now. */
> > + errno_from_tzset = errno;
>
> Why not fail now? If tzset fails, mktime is pointless.
I haven't changed the behavior of mktime with respect to tzset
failures, i.e., before this patch, mktime did not bail out when tzset
set errno. In fact, mktime was unable to find out reliably whether
__tzset had modified errno.
Since that was the behavior prior to this patch, and since this patch
was not intended to fix this behavior, I intentionally kept it
unchanged, and checked that it was. In fact, I initially did force
mktime to bail out on __tzset failures, and that caused a lot of tests
to fail in 'make check'.
> Also, remember this code is shared with Gnulib, and POSIX does not specify what
> tzset does to errno. So you can't rely on tzset setting errno, at least not in
> the Gnulib usage of this code. And can you even rely on glibc __tzset doing it?
> It's not part of tzset's spec, surely.
No, it is not, which I believe is why mktime does not bail on __tzset
setting errno (and may even possibly overwrite errno after __tzset).
> I thought the idea was to define a glibc-specific tzset variant that returned an
> error number, or something like that. I don't see that here.
Then I must have misunderstood your reply on Fri, 26 Oct 2018 17:10:26
-0700 where I thought you acknowledged that making all functions
called from mktime return a failure status was too extensive and
error-prone.
> Also, how about moving the tzset call into the # if defined _LIBC ||
> NEED_MKTIME_WORKING branch? tzset shouldn't be needed otherwise.
Provided it does not cause any previously successful 'make check'
test to suddenly fail, I am fine with this.
> > + if (result == -1 && errno == 0)
> > + return result;> + errno = 0;
> > + else if (errno != 0)
> > + return -1;
> > + else if (errno_from_tzset != 0)
> > + __set_errno(errno_from_tzset);
> > + else
> > + __set_errno(errno_before_mktime);
> > + return result;
>
> Sorry, but this is all too complicated. Remind me again why we can't rely on
> __mktime_internal to set errno on failure.
We can, or more exactly, we can, now that the patch sets errno to
EOVERFLOW when returning a failure -1.
But we cannot rely on errno being zero on entry into mktime. It
is, therefore, possible that errno is nonzero when mktime starts, so
mktime may
mistakenly think that an old nonzero errno value was set by
__mktime_internal. The only way to be sure that a non-zero errno is the
result of __mktime_internal is to force errno to 0 before the call.
> We don't care what happens to errno when __mktime_internal returns any value
> other than -1. Does that help?
I believe we do care that whatever happens to errno before the patch is
applied has to happen the same once the patch is applied, except for
the specific case that a __mktime_internal failure return of -1 should
also set errno to equal EOVERFLOW.
> I was hoping that mktime itself could look something like the following, which
> would be a lot simpler. Of course __tzset_with_result would have to be written
> (both in the Gnulib and the glibc case; the Gnulib version is easy), and
> __mktime_internal would have to do the right thing with errno, but this is the idea.
>
> time_t
> mktime (struct tm *tp)
> {
> int r = __tzset_with_result ();
> if (r < 0)
> return r;
This causes a lot of (so far successful) make check tests to result in
failure.
> # if defined _LIBC || NEED_MKTIME_WORKING
> static mktime_offset_t localtime_offset;
> return __mktime_internal (tp, __localtime_r, &localtime_offset);
> # else
> # undef mktime
> return mktime (tp);
> # endif
> }
I'll run a check with the errno backup removed and without the bail on
__tzset failure. Difference from v1 will only be where errno is set to
EOVERFLOW, moved from mktime (in v1) to __mktime_internal (in v3).
Cordialement,
Albert ARIBAUD
3ADEV
On 10/29/18 9:00 AM, Albert ARIBAUD wrote:
> I haven't changed the behavior of mktime with respect to tzset
> failures
The most recent proposal did make *some* change in this area, since
mktime inspected what tzset does to errno.
The simplest approach would be to define tzset to never fail. That is,
even if the user specifies a TZ value that is invalid, or if memory is
exhausted by tzset, tzset does not fail and its errno is irrelevant.
If we want to do something more complicated than that, it must be
glibc-specific (since POSIX tzset does not fail), and if so, the
glibc-specific tzset variant can return an error indication that mktime
can rely on when _LIBC is defined. However, it would be simpler, at
least for now, to simply say that tzset never fails and its actions on
errno are irrelevant. We can worry about fixing this later.
> The only way to be sure that a non-zero errno is the
> result of __mktime_internal is to force errno to 0 before the call.
Not if we're in charge of __mktime_internal, which we are.
__mktime_internal can use the same rule that mktime proper should use:
leave errno alone if it returns -1 successfully, set errno if it returns
-1 on failure, and errno's value is unspecified if it returns any value
other than -1.
> I believe we do care that whatever happens to errno before the patch is
> applied has to happen the same once the patch is applied, except for
> the specific case that a __mktime_internal failure return of -1 should
> also set errno to equal EOVERFLOW.
No, the idea is more limited than that. If mktime returns any value
other than -1, we don't care happens to errno and the patch can alter
errno's value in that case. If mktime returns -1 due to a failure, it
must set errno to indicate the failure, and this should happen
regardless of whether the failure occurs in __mktime_internal or
somewhere else, and it should happen regardless of whether the failure
is EOVERFLOW or some other failure. Finally, if mktime returns -1
successfully, it should leave errno alone.
Hi Paul,
On Mon, 29 Oct 2018 10:09:01 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :
> On 10/29/18 9:00 AM, Albert ARIBAUD wrote:
> > I haven't changed the behavior of mktime with respect to tzset
> > failures
>
> The most recent proposal did make *some* change in this area, since
> mktime inspected what tzset does to errno.
mktime only records what tzset does to errno in order to reproduce it
as faithfully as possible. I would probably get the same result if I
only recorded errno from after __tzset was called.
> The simplest approach would be to define tzset to never fail. That is,
> even if the user specifies a TZ value that is invalid, or if memory is
> exhausted by tzset, tzset does not fail and its errno is irrelevant.
>
> If we want to do something more complicated than that, it must be
> glibc-specific (since POSIX tzset does not fail), and if so, the
> glibc-specific tzset variant can return an error indication that mktime
> can rely on when _LIBC is defined. However, it would be simpler, at
> least for now, to simply say that tzset never fails and its actions on
> errno are irrelevant. We can worry about fixing this later.
Defining __tzset so that it never fails is what already happens (before
or after the patch). Indeed, I removed the whole errno handling part of
the patch, keeping only __mktime_internal EOVERFLOW additions, and the
make check tests were unaffected (save the addition of the successful
new time/tst-mktime4.c test).
> > The only way to be sure that a non-zero errno is the
> > result of __mktime_internal is to force errno to 0 before the call.
>
> Not if we're in charge of __mktime_internal, which we are.
> __mktime_internal can use the same rule that mktime proper should use:
> leave errno alone if it returns -1 successfully, set errno if it returns
> -1 on failure, and errno's value is unspecified if it returns any value
> other than -1.
>
>
> > I believe we do care that whatever happens to errno before the patch is
> > applied has to happen the same once the patch is applied, except for
> > the specific case that a __mktime_internal failure return of -1 should
> > also set errno to equal EOVERFLOW.
>
> No, the idea is more limited than that. If mktime returns any value
> other than -1, we don't care happens to errno and the patch can alter
> errno's value in that case. If mktime returns -1 due to a failure, it
> must set errno to indicate the failure, and this should happen
> regardless of whether the failure occurs in __mktime_internal or
> somewhere else, and it should happen regardless of whether the failure
> is EOVERFLOW or some other failure. Finally, if mktime returns -1
> successfully, it should leave errno alone.
That's actually __mktime_internal, but apart from that, I've just
checked that implementing exactly this causes no regression -- for some
reason I was convinced it had caused some in a previous attempt.
I'll send out a v3 with just the EOVERFLOW code moved from mktime to
__mktime_internal (this takes care of gmtime too).
Cordialement,
Albert ARIBAUD
3ADEV
Hi Joseph,
On Mon, 29 Oct 2018 15:15:40 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :
> On Sun, 28 Oct 2018, Albert ARIBAUD (3ADEV) wrote:
>
> > +#define TEST_FUNCTION do_test ()
> > +#include "../test-skeleton.c"
>
> New tests should use <support/test-driver.c>, not the old test-skeleton.
Ok.
> > + if (result == -1 && errno == 0)
> > + return result;
>
> It is never OK for a library function to set errno to 0 (return with errno
> set to 0 when it wasn't 0 on entry); if there is no error you can restore
> the value that was set on entry to mktime, or set errno to some spurious
> other value despite the lack of an error (maybe not a good idea in this
> case, because it prevents callers from distinguishing error and non-error
> -1), but not set it to 0 if it wasn't 0 on entry to the library function.
> (See ISO C: "The value of errno in the initial thread is zero at program
> startup (the initial value of errno in other threads is an indeterminate
> value), but is never set to zero by any library function.".)
Noted [but then, if some application code, for some reason, has errno
already set to EOVERFLOW by the time it calls mktime, and if mktime
returns -1 without setting errno (i.e., returns a valid "one second
before the Epoch" time), the application will falsely believe mktime
returned a failure (-1 and errno==EOVERFLOW). OTOH, -1 is not 100%
Posix as a time elapsed since the Epoch, so that's not really an issue].
> > + __set_errno(errno_from_tzset);
>
> Missing space before '(', here and elsewhere in this patch.
Will be "fixed" in v3 as errno handling will be removed.
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,28 @@
+#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;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.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,18 +529,41 @@ __mktime_internal (struct tm *tp,
time_t
mktime (struct tm *tp)
{
+ time_t result;
+ /* When __mktime_internal() returns -1, we need to know it it has set
+ * errno (real error) or not (just returning valid time_t value -1),
+ * so we beed to clear errno before calling __mktime_internal().
+ * But we also need to preserve errno if __mktime_internal() does not
+ * modify it, so we need to back up its current value.
+ * Ditto for __tzset(). */
+ int errno_from_tzset;
+ int errno_before_mktime = errno;
+
/* 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. */
+ errno = 0;
__tzset ();
+ /* record errno from __tzset() but do not fail now. */
+ errno_from_tzset = errno;
+ errno = 0;
# if defined _LIBC || NEED_MKTIME_WORKING
static mktime_offset_t localtime_offset;
- return __mktime_internal (tp, __localtime_r, &localtime_offset);
+ result = __mktime_internal (tp, __localtime_r, &localtime_offset);
# else
# undef mktime
- return mktime (tp);
+ result = mktime (tp);
# endif
+ if (result == -1 && errno == 0)
+ return result;
+ else if (errno != 0)
+ return -1;
+ else if (errno_from_tzset != 0)
+ __set_errno(errno_from_tzset);
+ else
+ __set_errno(errno_before_mktime);
+ return result;
}
#endif /* _LIBC || NEED_MKTIME_WORKING || NEED_MKTIME_WINDOWS */