Message ID | 20181028225206.4568-1-albert.aribaud@3adev.fr |
---|---|
State | New |
Headers | show |
Series | [v2] Ensure mktime sets errno on error (bug 23789) | expand |
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
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..dd1e0c76bf --- /dev/null +++ b/time/bug-mktime4.c @@ -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" diff --git a/time/mktime.c b/time/mktime.c index 00f0dec6b4..36b35824ff 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,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 */