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

[Paul Eggert]

[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
Description: Text Data

0001-mktime-fix-EOVERFLOW-bug-gnulib.patch
Description: Text Data