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] Ensure mktime sets errno on error (bug 23789)


Hi Paul,

On Thu, 25 Oct 2018 08:09:43 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 10/24/18 12:32 PM, Albert ARIBAUD (3ADEV) wrote:
> > +  result = __mktime_internal (tp, __localtime_r, &localtime_offset);
> > +  if (result == -1)
> > +    {
> > +      __set_errno(EOVERFLOW);
> > +    }  
> 
> A couple of other points. First, mktime can fail for reasons other than 
> EOVERFLOW; for example, mktime can exhaust memory due to an internal 
> malloc failure. In these cases mktime should set errno to the 
> appropriate error number, not to EOVERFLOW.

I think memory exhaustion, e.g. due to __tz_set() not able to malloc()
old_tz, is intended to be hidden within __tz_set() and not bubble up to
callers such as mktime(), but I may be mistaken, and in any case, no
effort is made to avoid malloc() or realloc() setting errno=ENOMEM.

So, if it should bubble up, then memory exhaustion would manifest
itself by malloc() or realloc() returning NULL and setting errno to
ENOMEM, so __mktime_internal() or mktime() would not need to set errno
again, but it would have to preserve this ENOMEM and not change it
accidentally into EOVERFLOW.

Generally speaking, if we are to bubble up failures while executing
mktime(), then any function involved should preserve errno in case it
calls another function which sets errno on failure.

This calls for some additional changes, for instance:

- mktime() calls __tz_set() then __mktime_internal(), and no check is
  made to see if __tz_set() failed -- if it does then __mktime_internal
  does too, we might lose an ENOMEM.
- but __tz_set() returns void, which means we cannot know if it failed,
  except by looking at errno.
- however, errno may have already been set from a previous, unrelated
  failure by the time mktime() calls __tz_set, and mktime() might
  wrongly take this old errno as a __tz_set() failure.
- To distinguish 'old' vs proper errno from __tz_set(), we would need
  it to return e.g. an int equal to 0 for success and -1 for failure,
  so that mktime() could detect __tz_set() failure and return -1 while
  preserving errno.
- __tz_set() would in turn require __tz_set_internal() to return a
  success/failure indication.

I feel that is a pretty extensive change, and I haven't looked into
__mktime_internal() itself, only __tz_set().

OTOH, we cannot just ignore failures in functions called by mktime()
and __mktime_internal(), and we cannot overwrite errno.

[the following assumes that __mktime_internal has been fixed to set
errno=EOVERFLOW and return -1 on failures as required to fix bug 23789,
*and* may return -1 as a valid value]

A simpler approach to the 'preserve errno values from called functions'
problem would be for mktime() to:
- backup the current errno value locally and reset errno to 0 (*)
- call __tz_set()
- if errno has become non-zero, return -1;
- reset errno to 0 again (*)
- call __mktime_internal()
- if __mktime_internal() has returned -1, return -1 (**)
- if errno has become non-zero, return -1;
- restore errno from the local backup;
- return whatever __mktime_internal() has returned.

(*) errno should not be reset to 0 by any system or library function, as
per the errno documentation, but I take it to actually mean that
non-zero errno values should be preserved by such functions. Here the
errno value on entry will be restored on exit, and thus preserved.

(**) This is so that if __mktime_internal() returned -1 as a valid
value (and thus set errno to 0 despite the rule above), then the -1
value *and* zeroed errno are passed up to the mktime() caller.

Comments welcome.

> Second, a nit: please avoid the curly braces in simple cases like the above.

Ok.

Cordialement,
Albert ARIBAUD
3ADEV


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