svn commit: r292777 - in head: lib/libc/sys sys/kern

[Dmitry Chagin]

2015-12-28 1:35 GMT+03:00 Bruce Evans <brde at optusnet.com.au>:

> 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
с) 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 = 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
>