svn commit: r347063 - head/sys/kern

[Bruce Evans]

On Tue, 7 May 2019, John Baldwin wrote:

> On 5/7/19 8:45 AM, Bruce Evans wrote:
>> On Mon, 6 May 2019, John Baldwin wrote:
>>
>>> On 5/6/19 11:45 AM, Mark Johnston wrote:
>>>> On Mon, May 06, 2019 at 11:07:18AM -0700, John Baldwin wrote:
>>>>> On 5/3/19 2:26 PM, Mark Johnston wrote:
>>>>>> Author: markj
>>>>>> Date: Fri May  3 21:26:44 2019
>>>>>> New Revision: 347063
>>>>>> URL: https://svnweb.freebsd.org/changeset/base/347063
>>>>>>
>>>>>> Log:
>>>>>>   Disallow excessively small times of day in clock_settime(2).
>>
>> This actually disallows negative timespecs in clock_settime(), with
>> collateral disallowment for small positive times.
>
> FWIW, clock_settime already disallows negative times due to the
> existing check in the previous two lines:
>
> 	if (ats->tv_nsec < 0 || ats->tv_nsec >= 1000000000 ||
> 	    ats->tv_sec < 0)
> 		return (EINVAL);
>
> I would probably lean towards not disallowing negative times as I
> said in one of my earlier replies, but given we have that blanket
> check already in place, the current patch doesn't seem to make
> things much worse.

This check was correctly missing in FreeBSD-5.

This check doesn't actually work.  I rememeber another detail in my test
program.  It sets the time to INT_MAX - 2 == INT32_MAX - 2 to get an
overflow 2 seconds later with 32-bit time_t.  This is how I got a file
time of 1901.

The check for large times on the line after the above is broken in another
way.  It has an upper limit of approx. 8000ULL years.  This limit is
unreachable for 32-bit time_t (except for the unsign extension bug for
negative times which is unreachable due to the above check).  So the check
only works to prevent overflow to a negative time with 64-bit time_t.

Other parts of the system are or were more careful about this:
- setitimer() and alarm() used to have a limit of 10**8 seconds to
   prevent overflow of 32-bit time_t when adding this number to the
   current time.  10**8 seconds is 3.16 years, so this works up to about
   2035.  This was broken by silently removing the check for 10**8 in
   itimerfix() as part of implementing POSIX itimers but the man pages
   still document the limit.  This is not allowed by POSIX.  POSIX
   requires times to work up to the limit of a time_t.  Some APIs return
   the residual time after a timeout, so this cannot be implemented by
   clamping the time to a reasonably small value.  POSIX itimers have a
   different implementation that is closer to not needing this limit
   and more broken having it.

   setitimer() is also broken for negative times (doesn't allow them).
   IIRC, POSIX requires this bug.  This just give more work for applications.
   Small negative differences can easily occur as the result of subtracting
   times.  Applications must either check for them after calculating
   differences, or handle the error for them from setimer().  Usually the
   correct handling is to treat negative timeouts as 0.

- sbintimes have 32 bits for the seconds part, so they have similar
   problems to 32-bit time_t.  Some places handle these problems by limiting
   the timeouts to INT32_MAX / 2.  The timeout is sometimes added to the
   current time, but since the current time is the monotonic time it is at
   most a few years so the limit for the timeout could be a bit larger than
   INT32_MAX / 2.

Oops, sbintimes are now used for for setitimer(), and setitimer() is one
of the few places with the INT32_MAX / 2 limit.  This gives the following
bugs for setitimer() and alarm():
- these syscalls still have a limit which is not allowed by POSIX
- the limit is INT32_MAX / 2 seconds (+ maximal usec?), not the documented
   one.

64_bit time_t's mostly just increase the overflow bugs.  Applications can
probe for bugs using INT64_MAX in tv_sec.  This overflows all 32-bit
calculations and most signed 64-bit calculations.  The kernel has to
reduce to 32 bits in seconds for all uses of sbintimes.  The kernel
does do this for nanosleep().

Bruce