Date: Mon, 28 Dec 2015 09:35:11 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ian Lepore <ian@freebsd.org> Cc: Dmitry Chagin <dchagin@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r292777 - in head: lib/libc/sys sys/kern Message-ID: <20151228083418.B1014@besplex.bde.org> In-Reply-To: <1451236237.1369.9.camel@freebsd.org> References: <201512271537.tBRFb7nN095297@repo.freebsd.org> <1451236237.1369.9.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 27 Dec 2015, Ian Lepore wrote: > On Sun, 2015-12-27 at 15:37 +0000, Dmitry Chagin wrote: >> Author: dchagin >> Date: Sun Dec 27 15:37:07 2015 >> New Revision: 292777 >> URL: https://svnweb.freebsd.org/changeset/base/292777 >> >> Log: >> Verify that tv_sec value specified in settimeofday() and >> clock_settime() >> (CLOCK_REALTIME case) system calls is non negative. >> This commit hides a kernel panic in atrtc_settime() as the >> clock_ts_to_ct() >> does not properly convert negative tv_sec. >> >> ps. in my opinion clock_ts_to_ct() should be rewritten to properly >> handle >> negative tv_sec values. >> >> Differential Revision: https://reviews.freebsd.org/D4714 >> Reviewed by: kib >> >> MFC after: 1 week > > IMO, this change is completely unacceptable. If there is a bug in > atrtc code, then by all means fix it, but preventing anyone from > setting valid time values on the system because one driver's code can't > handle it is just wrong. I agree. Even (time_t)-1 should be a valid time for input, although it is an error indicator for output. (This API makes correctly using functions like time(1) difficult or impossible (impossible for time(1) specifically. The implementation might reserve (time_t)-1 for representing an invalid time. But nothing requires it to. (POSIX almost requires the reverse. It requires a broken implementation that represents times as seconds since the Epoch. I think POSIX doesn't require times before the Epoch to work. But FreeBSD and the ado time package tries to make them work.) So if the representation of the current time is (time_t)-1, then time(1) can't do anything better than return this value. But this value is also the error value. There is no way to distinguish this value from the error value, since time(1) is not required to set errno.) I think the change also doesn't actually work, especially in the Western hemisphere, but it was written in the Eastern hemisphere. Suppose that the time is the Epoch. This is 0, so it is pefectly valid. Then if the clock is on local time, utc_offset() is added before calling clock_cs_to_ct() and the result is a negative time_t in the Western hemisphere. Similarly in the Eastern hemisphere when you test with with Western settings. The main bug in clock_ts_ct() is due to division being specified as broken in C: X void X clock_ts_to_ct(struct timespec *ts, struct clocktime *ct) X { X int i, year, days; X time_t rsec; /* remainder seconds */ X time_t secs; X X secs = ts->tv_sec; X days = secs / SECDAY; X rsec = secs % SECDAY; Division of negative numbers used to be implementation-defined in C, but C90 or C99 broke this by requiring the broken alternative of rounding towards 0 like most hardware does. The remainder operation is consistently broken. So when secs < 0, this always gives days < 0 and rsec either 0 or < 0. If this causes a panic, then it is from a sanity check detecting the invalid conversion later. A negative value in days breaks the loop logic but seems to give premature exit from the loops instead of many iterations. Another bug here is the type of rsec. This variable is a small integer (< SECDAY = 86400), not a time_t. Code like this is probably common in userland. w(1) uses it, but w(1) only deals with uptimes which should be positive. clock_ct_to_ts() is also buggy: X int X clock_ct_to_ts(struct clocktime *ct, struct timespec *ts) X { X int i, year, days; X ... X /* Sanity checks. */ X if (ct->mon < 1 || ct->mon > 12 || ct->day < 1 || X ct->day > days_in_month(year, ct->mon) || X ct->hour > 23 || ct->min > 59 || ct->sec > 59 || X (sizeof(time_t) == 4 && year > 2037)) { /* time_t overflow */ X if (ct_debug) X printf(" = EINVAL\n"); X return (EINVAL); X } The limit of 2037 is bogus with 64-bit time_t's or even with 32-bit unsigned time_t's. Years before 1970 are insane due to the C bug, and years before ~1906 are insanse due to representability problems, but there is no check for the lower limit of 'year'. There is also no check for the lower limit of ct->hour, ct->min or ct->sec. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20151228083418.B1014>