svn commit: r317208 - head/sys/netinet

[Hans Petter Selasky]

On 04/21/17 10:10, Bruce Evans wrote:
> On Fri, 21 Apr 2017, Michael Tuexen wrote:
>
>>> On 21. Apr 2017, at 05:16, Bruce Evans <brde at optusnet.com.au> wrote:
>>>
>>> On Thu, 20 Apr 2017, Cy Schubert wrote:
>>>
>>> Please trim quotes.
>>>
>>>> In message <201704201919.v3KJJYko052651 at repo.freebsd.org>, Michael
>>>> Tuexen
>>>> write
>>>> s:
>>>
>>> [>> ... 5 lines trimmed]
>>>
>>>>> Log:
>>>>>  Syncoockies can be used in combination with the syncache. If the
>>>>> cache
>>>>>  overflows, syncookies are used.
>>>
>>> [>> ... 16 lines trimmed]
>>>
>>>>> Modified: head/sys/netinet/tcp_syncache.c
>>>>> =============================================================================
>>>>>
>>>>> =
>>>>> --- head/sys/netinet/tcp_syncache.c    Thu Apr 20 19:14:52 2017
>>>>> (r31720
>>>>> 7)
>>>>> +++ head/sys/netinet/tcp_syncache.c    Thu Apr 20 19:19:33 2017
>>>>> (r31720
>>>>> 8)
>>>>> @@ -260,6 +260,7 @@ syncache_init(void)
>>>>>              &V_tcp_syncache.hashbase[i].sch_mtx, 0);
>>>>>         V_tcp_syncache.hashbase[i].sch_length = 0;
>>>>>         V_tcp_syncache.hashbase[i].sch_sc = &V_tcp_syncache;
>>>>> +        V_tcp_syncache.hashbase[i].sch_last_overflow = INT64_MIN;
>>>> ...
>>>> This line produced the following on i386:
>>>>
>>>> /opt/src/svn-current/sys/netinet/tcp_syncache.c:263:50: error: implicit
>>>> conversion from 'long long' to 'time_t' (aka 'int') changes value from
>>>> -9223372036854775808 to 0 [-Werror,-Wconstant-conversion]
>>>>               V_tcp_syncache.hashbase[i].sch_last_overflow = INT64_MIN;
>>>>                                                            ~ ^~~~~~~~~
>>>> ./x86/_stdint.h:91:41: note: expanded from macro 'INT64_MIN'
>>>> #define INT64_MIN       (-0x7fffffffffffffffLL-1)
>>>>                        ~~~~~~~~~~~~~~~~~~~~~^~
>>>>
>>>> Looks like it needs a time_t cast.
>>>
>>> A cast would just break the warning.  INT64_MIN has nothing to do with
>>> time_t.  The expression (time_t)INT64_MIN would be more obviously
>>> garbage
>>> since in the above it is not clear that sch_last_overflow has a type
>>> unrelated to the constant.
>> Fixed in https://svnweb.freebsd.org/changeset/base/317244
>>>
>>> INT64_MIN is also broken if time_t is 64 bits but unsigned.  Then it is
>>> converted to half of plus infinity instead of the intended minus
>>> infinity.
>> The patch assumes that time_t is signed, which is true for FreeBSD on all
>> platforms. Or am I missing a platform?
>
> Only future platforms.
>
> i386 should use time_t = uint32_t to fully support years 2038-2106 instead
> of time_t = int32_t to partially support years 1902-1970
>   (even time 0 (the Epoch) and other early hours in 1970 are not fully
>   supported now and would be broken by unsigned time_t, since subtraction
>   of the timezone offset from 0 gives negative values with signed time_t
>   and overflowing values with unsigned time_t).  (time_t)-1 is special,
>   so the time 1 second before the Epoch cannot work with signed time_t.
>   This value works better with unsigned time_t because it is not in the
>   middle of the range, but times before the Epoch are just unrepresentable
>   with unsigned time_t.
>
> Changing the signedness of time_t would break the ABI less than changing
> its size, but it still causes problems with buggy software which assumes
> that time_t is signed or encodes special values in it.  Negative times
> are at best unspecified by POSIX.  They give a large range of magic out
> of band values below 0, provided nothing assumes that the system is better
> designed than POSIX so supports times before the Epoch.  Even the Standard
> C library is not that bad, except POSIX forces a bad design for time_t so
> mktime() and friends are restricted to times.  If time_t is unsigned,
> then no times before the Epoch can work, and if it is signed then times
> before
> the Epoch are unportable.
>
> netinet could use non-negative times far in the past as out-of-band
> values if there are any.  But it mostly uses monotonic times.  The
> Epoch for monotonic times starts at boot time (modulo other bugs), so
> there are not enough times far enough in the past shortly after booting.
> If time_t is unsigned, there just aren't enough, and if it is signed
> there are only enough by using unportable negative times.

Hi,

Your proposal to change time_t to unsigned type is potentially dangerous 
in combination with NTP, where negative time deltas may occur. Consider 
existing code like this, both in three and outside the tree.

static time_t last;
time_t now = time();
time_t delta

delta = now - last;
if (delta > 0) {
    /* do something */
} else {
    /* ignore */
}

If time_t is now unsigned, then the check above becomes true for alomost 
all values of time_t, except zero, which is wrong!

Can C-compilers assert signedness of a variable?


I propose, utime_t -> unsigned time and stime_t -> signed time.

--HPS