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

[Albert ARIBAUD <albert dot aribaud at 3adev dot fr>]

On Tue, 30 Oct 2018 12:18:50 +0100, "Albert ARIBAUD (3ADEV)"
<albert.aribaud@3adev.fr> 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