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

[Albert ARIBAUD]

On Wed, 7 Nov 2018 13:39:42 +0100, Albert ARIBAUD
<address@hidden> wrote :

> So it seems to me that we're really "just" reaching a maximum of probes
> after which __mktime_internal, while not failing at computing candidate
> times, could not find a perfect match in less than the 6 rounds it
> allows itself.
> 
> I am instrumenting the code further to unravel the for probe loop
> logic; anyone to whom this rings a bell is welcome to comment.

The loop is acting weird. For all six iteration, at the loop body start,
gt was equal to -67768038462257713 and t, t1 and t2 were all equal to
-2147483648 -- no evolution during all six iterations, but never t==gt,
so the loop used up its remaining_probes and returned -1.

This leads me to two conclusions:

1. If the for-loop reaches remaining_probes==0, then it really should
   set errno = EOVERFLOW before returning -1, because remaining_probes
   is only decremented in the else clause inside the for-loop, and that
   only happens (or should only happen) when there were no failures so
   far, so if we fail then, we have to set errno.

2. It is not normal that t, gt, t1 and t2 remain the same for all six
   iterations of the for-loop. That should be investigated and fixed.

Regarding point 2, The -2147483648 value of t smells of 32-bit signed
saturation, and indeed, ranged_convert gets passed a pointer to t and
saturates it between mktime_min and mktime_max, which are defined to be
the shortest extrema between long_int and time_t.

Now, I don't know why ranged_convert alters an argument which should be
a pure imput. In fact, I don't know why it does not just copy this
argument into a local time_t. Any known reason?

Cordialement,
Albert ARIBAUD
3ADEV