From owner-svn-src-head@freebsd.org Tue May 7 17:44:06 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id F0005158EBCC; Tue, 7 May 2019 17:44:05 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 4571E6D1FB; Tue, 7 May 2019 17:44:04 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 7FE883DB491; Wed, 8 May 2019 03:44:01 +1000 (AEST) Date: Wed, 8 May 2019 03:44:00 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: John Baldwin cc: Bruce Evans , Mark Johnston , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r347063 - head/sys/kern In-Reply-To: Message-ID: <20190508025608.B1874@besplex.bde.org> References: <201905032126.x43LQilu092655@repo.freebsd.org> <335d828e-ac61-bc59-bac3-f80f27b951c7@FreeBSD.org> <20190506184502.GA35464@raichu> <52484f6b-fdae-565b-6c03-37a63d56ad30@FreeBSD.org> <20190508001838.B1127@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=D+Q3ErZj c=1 sm=1 tr=0 cx=a_idp_d a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=ZeAthX8GMysWMYlmZyQA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-Rspamd-Queue-Id: 4571E6D1FB X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.92 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.92)[-0.918,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 May 2019 17:44:06 -0000 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