From owner-svn-src-all@freebsd.org Tue Dec 29 12:06:21 2015 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 1C039A5485A; Tue, 29 Dec 2015 12:06:21 +0000 (UTC) (envelope-from chagin.dmitry@gmail.com) Received: from mail-ig0-x231.google.com (mail-ig0-x231.google.com [IPv6:2607:f8b0:4001:c05::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id DF9A71014; Tue, 29 Dec 2015 12:06:20 +0000 (UTC) (envelope-from chagin.dmitry@gmail.com) Received: by mail-ig0-x231.google.com with SMTP id mw1so35192579igb.1; Tue, 29 Dec 2015 04:06:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:date:message-id:subject:from:to:cc:content-type; bh=5OB5aFzBJx6iqzmT5X+1ZzFsireOL8Ezvsc68FAEiBE=; b=sb8dfCaJA4UAZya0zYa/+WaSK5itXJ50Y1SvKMolNTrZCAEalavjIpQjFzTdW2sqaB PnRAYk5ArDm+RkxJXuLQc3WHD+qX5v0Z2wGNQBtANPJlHUE7nSftIKcNxFF5BD7jjA7B 1ODc/eAq8a3u4iJ26vvAkku5/DCHzMpJPMGWlEyMfFjOUkpB0HHgZ+3zXulWDkYFjm0/ Rg28mMhBOBRenMEI3we3ix6FX2TCCLIzeSCLObYKt9U2pxthSDQq87HEUwt+GWdhOvXJ js/P8auqdLEtKIfyY9L/KysHhH10uaeKnlGipEqe0EygOk3uocD2V0bPwnRREUzKg6dB t3Xg== MIME-Version: 1.0 X-Received: by 10.50.142.39 with SMTP id rt7mr7977832igb.29.1451390780253; Tue, 29 Dec 2015 04:06:20 -0800 (PST) Sender: chagin.dmitry@gmail.com Received: by 10.79.82.68 with HTTP; Tue, 29 Dec 2015 04:06:20 -0800 (PST) Date: Tue, 29 Dec 2015 15:06:20 +0300 X-Google-Sender-Auth: cJ0KDaAJOgDagyZTdim3_8Ipxso Message-ID: Subject: Re: svn commit: r292777 - in head: lib/libc/sys sys/kern From: Dmitry Chagin To: Bruce Evans , Konstantin Belousov Cc: Ian Lepore , Dmitry Chagin , src-committers , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.20 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Dec 2015 12:06:21 -0000 2015-12-28 1:35 GMT+03:00 Bruce Evans : > 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.) > > So, my point was: a) for a long time we have broken settimeofday() which does not allow us to set the system time before the Epoch b) as you already mentioned POSIX doesn't require times before the Epoch to work =D1=81) Linux does not allows negative seconds also d) we have settimeofsay(2) that consistent with POSIX and in my understanding phrase "The time is expressed in seconds and microseconds since midnight (0 hour), January 1, 1970." is a strict definition, which prohibits time before the Epoch I do not understand why we should have our own (separate from the rest world) behavior. > 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 =3D ts->tv_sec; > X days =3D secs / SECDAY; > X rsec =3D 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 consistent= ly > 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 =3D 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) =3D=3D 4 && year > 2037)) { /* time_t ove= rflow > */ > X if (ct_debug) > X printf(" =3D 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 >