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

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Albert ARIBAUD (3ADEV)
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

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
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

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

Albert ARIBAUD (3ADEV)
On Tue, 30 Oct 2018 12:18:50 +0100, "Albert ARIBAUD (3ADEV)"
<[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Albert ARIBAUD (3ADEV)
Hi all,

On Wed, 31 Oct 2018 16:56:02 +0100, Albert ARIBAUD
<[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Joseph Myers
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
[hidden email]
Reply | Threaded
Open this post in threaded view
|

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

Albert ARIBAUD (3ADEV)
Hi Joseph,

On Fri, 2 Nov 2018 16:48:25 +0000, Joseph Myers
<[hidden email]> 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
[hidden email] address.

This means I cannot update Bugzilla properly unless my account gets
given the required permissions, right?

Cordialement,
Albert ARIBAUD
3ADEV
Reply | Threaded
Open this post in threaded view
|

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

Joseph Myers
On Sat, 3 Nov 2018, Albert ARIBAUD wrote:

> I don't have an @gcc.gnu.org or @sourceware.org address; I use my
> [hidden email] 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
[hidden email] / [hidden email] account in Bugzilla.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

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

Paul Eggert
In reply to this post by Albert ARIBAUD (3ADEV)
[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.

0001-mktime-fix-EOVERFLOW-bug-glibc.patch (3K) Download Attachment
0001-mktime-fix-EOVERFLOW-bug-gnulib.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Paul Eggert
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.

0001-mktime-fix-EOVERFLOW-bug.patch (3K) Download Attachment
0002-mktime-new-test-for-mktime-failure.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Albert ARIBAUD (3ADEV)
In reply to this post by Joseph Myers
Hi Joseph,

On Sat, 3 Nov 2018 00:16:04 +0000, Joseph Myers
<[hidden email]> wrote :

> On Sat, 3 Nov 2018, Albert ARIBAUD wrote:
>
> > I don't have an @gcc.gnu.org or @sourceware.org address; I use my
> > [hidden email] 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
> [hidden email] / [hidden email] account in Bugzilla.

I'd rather not multiply accounts if I can avoid it. How do I have the
permissions added to [hidden email]?

Cordialement,
Albert ARIBAUD
3ADEV
Reply | Threaded
Open this post in threaded view
|

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

Albert ARIBAUD (3ADEV)
In reply to this post by Paul Eggert
Hi Paul,

On Sun, 4 Nov 2018 23:54:59 -0800, Paul Eggert <[hidden email]>
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
Reply | Threaded
Open this post in threaded view
|

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

Albert ARIBAUD (3ADEV)
Hi Paul,

On Tue, 6 Nov 2018 06:43:06 +0100, Albert ARIBAUD
<[hidden email]> wrote :

> Hi Paul,
>
> On Sun, 4 Nov 2018 23:54:59 -0800, Paul Eggert <[hidden email]>
> 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
Reply | Threaded
Open this post in threaded view
|

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

Paul Eggert
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?

Reply | Threaded
Open this post in threaded view
|

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

Albert ARIBAUD (3ADEV)
Hi Paul,

On Tue, 6 Nov 2018 16:28:36 -0800, Paul Eggert <[hidden email]>
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
Reply | Threaded
Open this post in threaded view
|

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

Albert ARIBAUD (3ADEV)
On Wed, 7 Nov 2018 13:39:42 +0100, Albert ARIBAUD
<[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Paul Eggert
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.


0001-mktime-fix-EOVERFLOW-bug.txt (5K) Download Attachment
0002-mktime-new-test-for-mktime-failure.txt (5K) Download Attachment
0003-mktime-simplify-offset-guess.txt (2K) Download Attachment
0004-mktime-make-more-room-for-overflow.txt (4K) Download Attachment
0005-mktime-fix-bug-with-Y2038-DST-transition.txt (2K) Download Attachment
0006-mktime-fix-non-EOVERFLOW-errno-handling.txt (14K) Download Attachment
0007-mktime-DEBUG_MKTIME-cleanup.txt (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Albert ARIBAUD (3ADEV)
In reply to this post by Albert ARIBAUD (3ADEV)
Hi Albert,

On Mon, 5 Nov 2018 21:29:38 +0100, Albert ARIBAUD
<[hidden email]> wrote :

> Hi Joseph,
>
> On Sat, 3 Nov 2018 00:16:04 +0000, Joseph Myers
> <[hidden email]> wrote :
>
> > On Sat, 3 Nov 2018, Albert ARIBAUD wrote:
> >  
> > > I don't have an @gcc.gnu.org or @sourceware.org address; I use my
> > > [hidden email] 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
> > [hidden email] / [hidden email] account in Bugzilla.  
>
> I'd rather not multiply accounts if I can avoid it. How do I have the
> permissions added to [hidden email]?

Ping?
 
Cordialement,
Albert ARIBAUD
3ADEV
Reply | Threaded
Open this post in threaded view
|

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

Albert ARIBAUD (3ADEV)
In reply to this post by Paul Eggert
Hi Paul,

On Fri, 9 Nov 2018 18:00:25 -0800, Paul Eggert <[hidden email]>
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
Reply | Threaded
Open this post in threaded view
|

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

Albert ARIBAUD (3ADEV)
On Wed, 14 Nov 2018 00:04:04 +0100, Albert ARIBAUD
<[hidden email]> wrote :

> If I can get
> the adequate permissions for Bugzilla.

... I'll push the series onto master.

Cordialement,
Albert ARIBAUD
3ADEV
Reply | Threaded
Open this post in threaded view
|

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

Joseph Myers
In reply to this post by 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 [hidden email]?
>
> Ping?

I've added you to the editbugs group.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

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

Albert ARIBAUD (3ADEV)
Hi Joseph,

On Thu, 15 Nov 2018 17:01:38 +0000, Joseph Myers
<[hidden email]> 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 [hidden email]?  
> >
> > Ping?  
>
> I've added you to the editbugs group.

Thanks!

Cordialement,
Albert ARIBAUD
3ADEV