Ensure mktime sets errno on error (bug 23789)

  Posix mandates that mktime set errno to EOVERFLOW
on error, that it, upon retuning -1, but the glibc
mktime does not so far on 32-bit architectures.

Fix this and add a test to prevent regressions.

The test was run through 'make check' on i686-linux-gnu,
then the fix was added and 'make check' run again.

	* time/mktime.c
	(mktime): Set errno to EOVERFLOW on error.
	* time/bug-mktime4.c: New file.
        * time/Makefile: Add bug-mktime4.
---
 time/Makefile      |  2 +-
 time/bug-mktime4.c | 28 ++++++++++++++++++++++++++++
 time/mktime.c      | 11 +++++++++--
 3 files changed, 38 insertions(+), 3 deletions(-)
 create mode 100644 time/bug-mktime4.c
  

On Wed, Oct 24, 2018 at 3:32 PM Albert ARIBAUD (3ADEV)
<albert.aribaud@3adev.fr> wrote:
>
> Posix mandates that mktime set errno to EOVERFLOW
> on error, that it, upon retuning -1, but the glibc
> mktime does not so far on 32-bit architectures.
...
>  # 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)
> +    {
> +      __set_errno(EOVERFLOW);
> +    }
> +  return result;

Why do you make this change in the `mktime` wrapper instead of
`__mktime_internal`?

zw
  

Hi Zack,

On Wed, 24 Oct 2018 20:04:11 -0400, Zack Weinberg <zackw@panix.com>
wrote :

> On Wed, Oct 24, 2018 at 3:32 PM Albert ARIBAUD (3ADEV)
> <albert.aribaud@3adev.fr> wrote:
> >
> > Posix mandates that mktime set errno to EOVERFLOW
> > on error, that it, upon retuning -1, but the glibc
> > mktime does not so far on 32-bit architectures.  
> ...
> >  # 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)
> > +    {
> > +      __set_errno(EOVERFLOW);
> > +    }
> > +  return result;  
> 
> Why do you make this change in the `mktime` wrapper instead of
> `__mktime_internal`?

The change is about errno, which is part of the public interface, so I
considered it was more logical to put it in the wrapper which
implements the public interface than in an internal function.

> zw

Cordialement,
Albert ARIBAUD
3ADEV
  

On Okt 25 2018, Albert ARIBAUD <albert.aribaud@3adev.fr> wrote:

> The change is about errno, which is part of the public interface, so I
> considered it was more logical to put it in the wrapper which
> implements the public interface than in an internal function.

I think timegm should get the same treatment for consistency.

Andreas.
  

Albert ARIBAUD (3ADEV) wrote:
> +  if (result == -1)
> +    {
> +      __set_errno(EOVERFLOW);
> +    }

This patch is not correct since -1 is a valid time_t value, and a result of -1 
does not necessarily indicate time_t overflow.
  

Hi Paul,

On Thu, 25 Oct 2018 01:30:45 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> Albert ARIBAUD (3ADEV) wrote:
> > +  if (result == -1)
> > +    {
> > +      __set_errno(EOVERFLOW);
> > +    }  
> 
> This patch is not correct since -1 is a valid time_t value, and a result of -1 
> does not necessarily indicate time_t overflow.

While a do agree that -1 is a valid time_t value in general terms, in
the specific case of the mktime() return values, -1 is the value
returned on error, as per the manual page and as per Posix:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/mktime.html

Cordialement,
Albert ARIBAUD
3ADEV
  

On Thu, Oct 25, 2018 at 8:59 AM Albert ARIBAUD <albert.aribaud@3adev.fr> wrote:
> >
> > This patch is not correct since -1 is a valid time_t value, and a result of -1
> > does not necessarily indicate time_t overflow.
>
> While a do agree that -1 is a valid time_t value in general terms, in
> the specific case of the mktime() return values, -1 is the value
> returned on error

Yes, but it is *also* a value that can be returned as the result of a
*successful* call to mktime.  Specifically, I believe this program is
required to print

    tv=-1 errno=0

on a system conforming to POSIX, when invoked with the time zone set to UTC:

#include <stdio.h>
#include <time.h>
#include <errno.h>

int main(void)
{
  struct tm tm;
  tm.tm_sec   = 59;
  tm.tm_min   = 59;
  tm.tm_hour  = 23;
  tm.tm_mday  = 31;
  tm.tm_mon   = 11;
  tm.tm_year  = 69;
  tm.tm_wday  = 0;
  tm.tm_yday  = 0;
  tm.tm_isdst = 0;

  errno = 0;
  time_t tv = mktime(&tm);
  int err = errno;

  printf("tv=%lld errno=%d\n", (long long)tv, err);
  return 0;
}

This is similar to how strtol can return LONG_MAX either because the
input string was exactly the decimal representation of LONG_MAX, or
because the input string was the representation of a number too large
for "long"; errno is required *not* to be set to ERANGE in the first
case.

zw
  

On Thu, Oct 25, 2018 at 12:34 AM Albert ARIBAUD <albert.aribaud@3adev.fr> wrote:
> > > +  result = __mktime_internal (tp, __localtime_r, &localtime_offset);
...
> > > +  if (result == -1)
> > > +    {
> > > +      __set_errno(EOVERFLOW);
> > > +    }
> > > +  return result;
> >
> > Why do you make this change in the `mktime` wrapper instead of
> > `__mktime_internal`?
>
> The change is about errno, which is part of the public interface, so I
> considered it was more logical to put it in the wrapper which
> implements the public interface than in an internal function.

The proper question to ask is whether the other callers of
__mktime_internal should also set errno according to the same rules.
In this case, the only other caller (within glibc; I haven't checked
gnulib) is timegm. timegm is not part of any standard, but I agree
with Andreas that it should have the same public semantics as mktime,
for consistency's sake.

Also, as Paul pointed out, the wrapper can't tell whether
__mktime_internal returned -1 due to a true overflow, or because the
result of the conversion is correctly (time_t)-1.  __mktime_internal
itself, on the other hand, knows which case it's in and can set errno
only when appropriate.

zw
  

Hi Zack,

On Thu, 25 Oct 2018 09:37:08 -0400, Zack Weinberg <zackw@panix.com>
wrote :

> On Thu, Oct 25, 2018 at 8:59 AM Albert ARIBAUD <albert.aribaud@3adev.fr> wrote:
> > >
> > > This patch is not correct since -1 is a valid time_t value, and a result of -1
> > > does not necessarily indicate time_t overflow.  
> >
> > While a do agree that -1 is a valid time_t value in general terms, in
> > the specific case of the mktime() return values, -1 is the value
> > returned on error  
> 
> Yes, but it is *also* a value that can be returned as the result of a
> *successful* call to mktime.  Specifically, I believe this program is
> required to print
> 
>     tv=-1 errno=0
> 
> on a system conforming to POSIX, when invoked with the time zone set to UTC:
> 
> #include <stdio.h>
> #include <time.h>
> #include <errno.h>
> 
> int main(void)
> {
>   struct tm tm;
>   tm.tm_sec   = 59;
>   tm.tm_min   = 59;
>   tm.tm_hour  = 23;
>   tm.tm_mday  = 31;
>   tm.tm_mon   = 11;
>   tm.tm_year  = 69;
>   tm.tm_wday  = 0;
>   tm.tm_yday  = 0;
>   tm.tm_isdst = 0;
> 
>   errno = 0;
>   time_t tv = mktime(&tm);
>   int err = errno;
> 
>   printf("tv=%lld errno=%d\n", (long long)tv, err);
>   return 0;
> }
> 
> This is similar to how strtol can return LONG_MAX either because the
> input string was exactly the decimal representation of LONG_MAX, or
> because the input string was the representation of a number too large
> for "long"; errno is required *not* to be set to ERANGE in the first
> case.

Hmm... Posix defines the value returned by mktime to be "Seconds since
the Epoch" thus: <http://pubs.opengroup.org/onlinepubs/9699919799/>.
According to this definition, a year below 1970 makes the corresponding
seconds since the epoch value undefined, so I wonder whether the struct
tm above is not outside the allowed limits for mktime().

Cordialement,
Albert ARIBAUD
3ADEV
  

On Thu, Oct 25, 2018 at 10:03 AM Albert ARIBAUD <albert.aribaud@3adev.fr> wrote:

> Hmm... Posix defines the value returned by mktime to be "Seconds since
> the Epoch" thus: <http://pubs.opengroup.org/onlinepubs/9699919799/>.
> According to this definition, a year below 1970 makes the corresponding
> seconds since the epoch value undefined, so I wonder whether the struct
> tm above is not outside the allowed limits for mktime().

That may be true per the letter of POSIX, but there are enough
programs that expect to be able to use negative time_t values to work
with dates prior to 1970 that glibc needs to support this usage
anyway.

zw
  

Hi Zack,

On Thu, 25 Oct 2018 10:12:37 -0400, Zack Weinberg <zackw@panix.com>
wrote :

> On Thu, Oct 25, 2018 at 10:03 AM Albert ARIBAUD <albert.aribaud@3adev.fr> wrote:
> 
> > Hmm... Posix defines the value returned by mktime to be "Seconds since
> > the Epoch" thus: <http://pubs.opengroup.org/onlinepubs/9699919799/>.
> > According to this definition, a year below 1970 makes the corresponding
> > seconds since the epoch value undefined, so I wonder whether the struct
> > tm above is not outside the allowed limits for mktime().  
> 
> That may be true per the letter of POSIX, but there are enough
> programs that expect to be able to use negative time_t values to work
> with dates prior to 1970 that glibc needs to support this usage
> anyway.

I was afraid that would be the case. :)
IMO it warrants at least a comment to explain the divergence from
Posix, but apart from that, I can't see a way to keep __mktime_internal
really internal (i.e., not touch errno) without making the code more
complex than it should; so I will move the setting of errno in
__mktime_internal. This will cover the timegm case too.

I don't think I need to add a test for timegm(), though.

> zw

Cordialement,
Albert ARIBAUD
3ADEV
  

* Zack Weinberg:

> On Thu, Oct 25, 2018 at 10:03 AM Albert ARIBAUD <albert.aribaud@3adev.fr> wrote:
>
>> Hmm... Posix defines the value returned by mktime to be "Seconds since
>> the Epoch" thus: <http://pubs.opengroup.org/onlinepubs/9699919799/>.
>> According to this definition, a year below 1970 makes the corresponding
>> seconds since the epoch value undefined, so I wonder whether the struct
>> tm above is not outside the allowed limits for mktime().
>
> That may be true per the letter of POSIX, but there are enough
> programs that expect to be able to use negative time_t values to work
> with dates prior to 1970 that glibc needs to support this usage
> anyway.

There is also this:

| 4.16 Seconds Since the Epoch
|
| A value that approximates the number of seconds that have elapsed
| since the Epoch. A Coordinated Universal Time name (specified in terms
| of seconds (tm_sec), minutes (tm_min), hours (tm_hour), days since
| January 1 of the year (tm_yday), and calendar year minus 1900
| (tm_year)) is related to a time represented as seconds since the
| Epoch, according to the expression below.
|
| If the year is <1970 or the value is negative, the relationship is
| undefined. If the year is >=1970 and the value is non-negative, the
| value is related to a Coordinated Universal Time name according to the
| C-language expression, where tm_sec, tm_min, tm_hour, tm_yday, and
| tm_year are all integer types:
|
|    tm_sec + tm_min*60 + tm_hour*3600 + tm_yday*86400 +
|     (tm_year-70)*31536000 + ((tm_year-69)/4)*86400 -
|     ((tm_year-1)/100)*86400 + ((tm_year+299)/400)*86400

<http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_16>

I think the formula is only valid for the year 1970 and later years.
Running the test program below gives me:

  error: tm_yday: expected 4, got 5
  gmtime-before-1970: errors found for year 1968, exiting

So negative epoch values are clearly an extension over POSIX, but it's a
widely used one, I think.

Thanks,
Florian

#include <err.h>
#include <inttypes.h>
#include <stdbool.h>
#include <stdio.h>
#include <time.h>

static void
check (const char *what, bool *good, int64_t expected, int64_t actual)
{
  if (expected != actual)
    {
      printf ("error: %s: expected %" PRId64 ", got %" PRId64 "\n",
              what, expected, actual);
      *good = false;
    }
}

int
main (void)
{
  int64_t tm_sec = 1;
  int64_t tm_min = 2;
  int64_t tm_hour = 3;
  int64_t tm_yday = 4;
  for (int64_t tm_year = 120; tm_year > -500; --tm_year)
    {
      int64_t epoch = tm_sec + tm_min*60 + tm_hour*3600 + tm_yday*86400 +
        (tm_year-70)*31536000 + ((tm_year-69)/4)*86400 -
        ((tm_year-1)/100)*86400 + ((tm_year+299)/400)*86400;
      time_t time_epoch = epoch;
      struct tm *tm = gmtime (&time_epoch);
      if (tm == NULL)
        err (1, "gmtime");
      bool good = true;
      check ("tm_sec", &good, tm_sec, tm->tm_sec);
      check ("tm_min", &good, tm_min, tm->tm_min);
      check ("tm_hour", &good, tm_hour, tm->tm_hour);
      check ("tm_yday", &good, tm_yday, tm->tm_yday);
      check ("tm_year", &good, tm_year, tm->tm_year);
      if (!good)
        errx (1, "errors found for year %" PRId64 ", exiting",
              1900 + tm_year);
    }
  return 0;
}
  

On 10/25/18 7:39 AM, Florian Weimer wrote:
> negative epoch values are clearly an extension over POSIX, but it's a
> widely used one, I think.

Yes, and every implementation I know of that has signed time_t supports 
this extension (of course some, like glibc, are buggy; but that's the 
intent).

There are a very few implementations with unsigned time_t, and these 
implementations have a similar issue with the positive value ((time_t) 
-1). The mktime code (which is shared with Gnulib) should work in that 
case as well.
  

On 10/24/18 12:32 PM, Albert ARIBAUD (3ADEV) wrote:
> +  result = __mktime_internal (tp, __localtime_r, &localtime_offset);
> +  if (result == -1)
> +    {
> +      __set_errno(EOVERFLOW);
> +    }

A couple of other points. First, mktime can fail for reasons other than 
EOVERFLOW; for example, mktime can exhaust memory due to an internal 
malloc failure. In these cases mktime should set errno to the 
appropriate error number, not to EOVERFLOW.

Second, a nit: please avoid the curly braces in simple cases like the above.
  

On Thu, 25 Oct 2018, Albert ARIBAUD wrote:

> Posix, but apart from that, I can't see a way to keep __mktime_internal
> really internal (i.e., not touch errno) without making the code more
> complex than it should; so I will move the setting of errno in

If you did want to avoid __mktime_internal setting errno (and in this case 
it seems best for it to set errno, since both callers expect such errno 
setting and there may be errors other than EOVERFLOW), you'd change the 
interface, so it e.g. returns an error number and uses a pointer to return 
the time value (for example).

> I don't think I need to add a test for timegm(), though.

Not for this patch.  At some point in the Y2038 series you'll presumably 
add a version of timegm for _TIME_BITS=64, and that will need testing; 
since there are no tests for timegm in the glibc testsuite at all, it 
would be appropriate then to add a test that gets built and run both with 
and without _TIME_BITS=64, so the existing function gets tested as well as 
the new one.
  

Hi Paul,

On Thu, 25 Oct 2018 08:09:43 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 10/24/18 12:32 PM, Albert ARIBAUD (3ADEV) wrote:
> > +  result = __mktime_internal (tp, __localtime_r, &localtime_offset);
> > +  if (result == -1)
> > +    {
> > +      __set_errno(EOVERFLOW);
> > +    }  
> 
> A couple of other points. First, mktime can fail for reasons other than 
> EOVERFLOW; for example, mktime can exhaust memory due to an internal 
> malloc failure. In these cases mktime should set errno to the 
> appropriate error number, not to EOVERFLOW.

I think memory exhaustion, e.g. due to __tz_set() not able to malloc()
old_tz, is intended to be hidden within __tz_set() and not bubble up to
callers such as mktime(), but I may be mistaken, and in any case, no
effort is made to avoid malloc() or realloc() setting errno=ENOMEM.

So, if it should bubble up, then memory exhaustion would manifest
itself by malloc() or realloc() returning NULL and setting errno to
ENOMEM, so __mktime_internal() or mktime() would not need to set errno
again, but it would have to preserve this ENOMEM and not change it
accidentally into EOVERFLOW.

Generally speaking, if we are to bubble up failures while executing
mktime(), then any function involved should preserve errno in case it
calls another function which sets errno on failure.

This calls for some additional changes, for instance:

- mktime() calls __tz_set() then __mktime_internal(), and no check is
  made to see if __tz_set() failed -- if it does then __mktime_internal
  does too, we might lose an ENOMEM.
- but __tz_set() returns void, which means we cannot know if it failed,
  except by looking at errno.
- however, errno may have already been set from a previous, unrelated
  failure by the time mktime() calls __tz_set, and mktime() might
  wrongly take this old errno as a __tz_set() failure.
- To distinguish 'old' vs proper errno from __tz_set(), we would need
  it to return e.g. an int equal to 0 for success and -1 for failure,
  so that mktime() could detect __tz_set() failure and return -1 while
  preserving errno.
- __tz_set() would in turn require __tz_set_internal() to return a
  success/failure indication.

I feel that is a pretty extensive change, and I haven't looked into
__mktime_internal() itself, only __tz_set().

OTOH, we cannot just ignore failures in functions called by mktime()
and __mktime_internal(), and we cannot overwrite errno.

[the following assumes that __mktime_internal has been fixed to set
errno=EOVERFLOW and return -1 on failures as required to fix bug 23789,
*and* may return -1 as a valid value]

A simpler approach to the 'preserve errno values from called functions'
problem would be for mktime() to:
- backup the current errno value locally and reset errno to 0 (*)
- call __tz_set()
- if errno has become non-zero, return -1;
- reset errno to 0 again (*)
- call __mktime_internal()
- if __mktime_internal() has returned -1, return -1 (**)
- if errno has become non-zero, return -1;
- restore errno from the local backup;
- return whatever __mktime_internal() has returned.

(*) errno should not be reset to 0 by any system or library function, as
per the errno documentation, but I take it to actually mean that
non-zero errno values should be preserved by such functions. Here the
errno value on entry will be restored on exit, and thus preserved.

(**) This is so that if __mktime_internal() returned -1 as a valid
value (and thus set errno to 0 despite the rule above), then the -1
value *and* zeroed errno are passed up to the mktime() caller.

Comments welcome.

> Second, a nit: please avoid the curly braces in simple cases like the above.

Ok.

Cordialement,
Albert ARIBAUD
3ADEV
  

On 10/25/18 9:57 AM, Albert ARIBAUD wrote:
> - To distinguish 'old' vs proper errno from __tz_set(), we would need
>    it to return e.g. an int equal to 0 for success and -1 for failure,
>    so that mktime() could detect __tz_set() failure and return -1 while
>    preserving errno.

This sounds like the way to go. The other approach that you mentioned 
(saving and restoring errno a couple of times) would be more 
error-prone, particularly in non-glibc environments (remember, this code 
is shared with Gnulib).
  

Hi Paul,

On Thu, 25 Oct 2018 12:49:49 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 10/25/18 9:57 AM, Albert ARIBAUD wrote:
> > - To distinguish 'old' vs proper errno from __tz_set(), we would need
> >    it to return e.g. an int equal to 0 for success and -1 for failure,
> >    so that mktime() could detect __tz_set() failure and return -1 while
> >    preserving errno.  
> 
> This sounds like the way to go. The other approach that you mentioned 
> (saving and restoring errno a couple of times) would be more 
> error-prone, particularly in non-glibc environments (remember, this code 
> is shared with Gnulib).

[ s/__tz_set/__tzset/g ]

Problem: __tzset is aliased by tzset, which has a public prototype of
'void (*) (void)'.

I'm worried that for some architectures at least, existing userland
object code which calls tzset() might, if run above a glibc featuring
the change above, actually call __tz_set(), bypassing the wrapper and
potentially trashing some register(s) with the return value from
__tzset().

If there is such a risk, then the solution is to keep tzset() an alias
of __tzset(), keep __tzset() a 'void (*) (void)' as it is now, and
introduce another function, e.g. ___tzset(), which would be the 'int
(*) (void)', and replace the call to __tzset() within mktime() et al.
with a call to ___tzset() along with proper return value handling.

Comments/thoughts?

Cordialement,
Albert ARIBAUD
3ADEV
  

On 10/26/18 9:12 AM, Albert ARIBAUD wrote:
> the solution is to keep tzset() an alias
> of __tzset(), keep __tzset() a 'void (*) (void)' as it is now, and
> introduce another function

Yes, that sounds right.
  

Hi Paul,

On Fri, 26 Oct 2018 09:28:40 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 10/26/18 9:12 AM, Albert ARIBAUD wrote:
> > the solution is to keep tzset() an alias
> > of __tzset(), keep __tzset() a 'void (*) (void)' as it is now, and
> > introduce another function  
> 
> Yes, that sounds right.

... and another problem:

Apparently range_convert() does everything to never fail in converting
a long int time into a struct tm.

On the other hand, ranged_convert() much indirectly ends up possibly
calling __offtime(), which sets EOVERFLOW when it returns a failure
status.

In other words, even though range_convert() hides failures from its
caller, it may return with errno modified. Is it allowed behavior to
alter errno when not returning an error condition?

If it is not allowed, then __offtime() may need to be modified so that
it does not set errno when called from range_convert().

Comments?

Cordialement,
Albert ARIBAUD
3ADEV
  

On 10/26/18 2:18 PM, Albert ARIBAUD wrote:
> Is it allowed behavior to
> alter errno when not returning an error condition?

Ordinarily yes. But it would be better if mktime left errno alone when 
-1 does not represent an error, so that the caller can easily 
distinguish valid -1 return values from erroneous ones.
  

Hi Paul,

On Fri, 26 Oct 2018 15:45:29 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 10/26/18 2:18 PM, Albert ARIBAUD wrote:
> > Is it allowed behavior to
> > alter errno when not returning an error condition?  
> 
> Ordinarily yes. But it would be better if mktime left errno alone when 
> -1 does not represent an error, so that the caller can easily 
> distinguish valid -1 return values from erroneous ones.

Ok, so this requirement extends to all functions called by mktime(),
directly or indirectly (and ditto for gmtime).

Cordialement,
Albert ARIBAUD
3ADEV
  

On 10/26/18 4:26 PM, Albert ARIBAUD wrote:
> Ok, so this requirement extends to all functions called by mktime(),
> directly or indirectly (and ditto for gmtime).

Hmm, well, I'm starting to see why you'd rather have mktime save and 
restore errno when there's not an error. That would also be OK, and that 
might lighten the load on the functions mktime calls.
  

Bonjour Paul,

Le Fri, 26 Oct 2018 17:10:26 -0700, Paul Eggert <eggert@cs.ucla.edu> a
écrit :

> On 10/26/18 4:26 PM, Albert ARIBAUD wrote:
> > Ok, so this requirement extends to all functions called by mktime(),
> > directly or indirectly (and ditto for gmtime).  
> 
> Hmm, well, I'm starting to see why you'd rather have mktime save and 
> restore errno when there's not an error. That would also be OK, and that 
> might lighten the load on the functions mktime calls.

Indeed. So, unless someone else disagrees, I will go with the "save
and restore errno" option, with of course adequate comments in the
code to make it clear why it does it and how.

Cordialement,
Albert ARIBAUD
3ADEV