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

  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.

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): Move call to __tzset inside conditional.
---
History:
- 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
  

On Tue, 30 Oct 2018, Albert ARIBAUD (3ADEV) 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.
> 
> The test was run through 'make check' on i686-linux-gnu,
> then the fix was added and 'make check' run again.

I strongly advise testing this sort of thing on both 32-bit *and* 64-bit 
platforms, given the likelihood of differences when time_t is 64-bit.