This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] Ensure mktime sets errno on error (bug 23789)
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