This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi Paul,

On Mon, 29 Oct 2018 00:05:12 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> Albert ARIBAUD (3ADEV) wrote:
> 
> >     __tzset ();
> > +  /* record errno from __tzset() but do not fail now.  */
> > +  errno_from_tzset = errno;  
> 
> Why not fail now? If tzset fails, mktime is pointless.

I haven't changed the behavior of mktime with respect to tzset
failures, i.e., before this patch, mktime did not bail out when tzset
set errno. In fact, mktime was unable to find out reliably whether
__tzset had modified errno.

Since that was the behavior prior to this patch, and since this patch
was not intended to fix this behavior, I intentionally kept it
unchanged, and checked that it was. In fact, I initially did force
mktime to bail out on __tzset failures, and that caused a lot of tests
to fail in 'make check'.

> Also, remember this code is shared with Gnulib, and POSIX does not specify what 
> tzset does to errno. So you can't rely on tzset setting errno, at least not in 
> the Gnulib usage of this code. And can you even rely on glibc __tzset doing it? 
> It's not part of tzset's spec, surely.

No, it is not, which I believe is why mktime does not bail on __tzset
setting errno (and may even possibly overwrite errno after __tzset). 

> I thought the idea was to define a glibc-specific tzset variant that returned an 
> error number, or something like that. I don't see that here.

Then I must have misunderstood your reply on Fri, 26 Oct 2018 17:10:26
-0700 where I thought you acknowledged that making all functions
called from mktime return a failure status was too extensive and
error-prone.

> Also, how about moving the tzset call into the # if defined _LIBC || 
> NEED_MKTIME_WORKING branch? tzset shouldn't be needed otherwise.

Provided it does not cause any previously successful 'make check'
test to suddenly fail, I am fine with this.

> > +  if (result == -1 && errno == 0)
> > +    return result;> +  errno = 0;
> > +  else if (errno != 0)
> > +    return -1;
> > +  else if (errno_from_tzset != 0)
> > +    __set_errno(errno_from_tzset);
> > +  else
> > +    __set_errno(errno_before_mktime);
> > +  return result;  
> 
> Sorry, but this is all too complicated. Remind me again why we can't rely on 
> __mktime_internal to set errno on failure.

We can, or more exactly, we can, now that the patch sets errno to
EOVERFLOW when returning a failure -1.

But we cannot rely on errno being zero on entry into mktime. It
is, therefore, possible that errno is nonzero when mktime starts, so
mktime may
mistakenly think that an old nonzero errno value was set by
__mktime_internal. 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.

> We don't care what happens to errno when __mktime_internal returns any value 
> other than -1. Does that help?

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.

> I was hoping that mktime itself could look something like the following, which 
> would be a lot simpler. Of course __tzset_with_result would have to be written 
> (both in the Gnulib and the glibc case; the Gnulib version is easy), and 
> __mktime_internal would have to do the right thing with errno, but this is the idea.
> 
> time_t
> mktime (struct tm *tp)
> {
>    int r = __tzset_with_result ();
>    if (r < 0)
>      return r;

This causes a lot of (so far successful) make check tests to result in
failure.

> # if defined _LIBC || NEED_MKTIME_WORKING
>    static mktime_offset_t localtime_offset;
>    return __mktime_internal (tp, __localtime_r, &localtime_offset);
> # else
> #  undef mktime
>    return mktime (tp);
> # endif
> }

I'll run a check with the errno backup removed and without the bail on
__tzset failure. Difference from v1 will only be where errno is set to
EOVERFLOW, moved from mktime (in v1) to __mktime_internal (in v3).

Cordialement,
Albert ARIBAUD
3ADEV


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]