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

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

Hi Paul,

On Mon, 29 Oct 2018 10:09:01 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 10/29/18 9:00 AM, Albert ARIBAUD wrote:
> > I haven't changed the behavior of mktime with respect to tzset
> > failures  
> 
> The most recent proposal did make *some* change in this area, since 
> mktime inspected what tzset does to errno.

mktime only records what tzset does to errno in order to reproduce it
as faithfully as possible. I would probably get the same result if I
only recorded errno from after __tzset was called. 

> The simplest approach would be to define tzset to never fail. That is, 
> even if the user specifies a TZ value that is invalid, or if memory is 
> exhausted by tzset, tzset does not fail and its errno is irrelevant.
>
> If we want to do something more complicated than that, it must be 
> glibc-specific (since POSIX tzset does not fail), and if so, the 
> glibc-specific tzset variant can return an error indication that mktime 
> can rely on when _LIBC is defined. However, it would be simpler, at 
> least for now, to simply say that tzset never fails and its actions on 
> errno are irrelevant. We can worry about fixing this later.

Defining __tzset so that it never fails is what already happens (before
or after the patch). Indeed, I removed the whole errno handling part of
the patch, keeping only __mktime_internal EOVERFLOW additions, and the
make check tests were unaffected (save the addition of the successful
new time/tst-mktime4.c test).

> > The only way to be sure that a non-zero errno is the
> > result of __mktime_internal is to force errno to 0 before the call.  
> 
> Not if we're in charge of __mktime_internal, which we are. 
> __mktime_internal can use the same rule that mktime proper should use: 
> leave errno alone if it returns -1 successfully, set errno if it returns 
> -1 on failure, and errno's value is unspecified if it returns any value 
> other than -1.
> 
> 
> > I believe we do care that whatever happens to errno before the patch is
> > applied has to happen the same once the patch is applied, except for
> > the specific case that a __mktime_internal failure return of -1 should
> > also set errno to equal EOVERFLOW.  
> 
> No, the idea is more limited than that. If mktime returns any value 
> other than -1, we don't care happens to errno and the patch can alter 
> errno's value in that case. If mktime returns -1 due to a failure, it 
> must set errno to indicate the failure, and this should happen 
> regardless of whether the failure occurs in __mktime_internal or 
> somewhere else, and it should happen regardless of whether the failure 
> is EOVERFLOW or some other failure. Finally, if mktime returns -1 
> successfully, it should leave errno alone.

That's actually __mktime_internal, but apart from that, I've just
checked that implementing exactly this causes no regression -- for some
reason I was convinced it had caused some in a previous attempt.

I'll send out a v3 with just the EOVERFLOW code moved from mktime to
__mktime_internal (this takes care of gmtime too).

Cordialement,
Albert ARIBAUD
3ADEV