On 17/02/16(Wed) 20:38, Stefan Kempf wrote: > Martin Pieuchot wrote: > > On 13/02/16(Sat) 18:51, Stefan Kempf wrote: > > > Some thoughts about this: > > > > > > If this particular type of undefined behavior is really a concern: maybe > > > looking for bounds/overflow checks that are incorrect besides undefined > > > behavior first is a better approach. A good way of fixing those will > > > be found, which could then be applied to the "just undefined behavior" > > > cases. > > > > > > Details below. > > > > > > Michael McConville wrote: > > > > time_second is a time_t, which we define as int64_t. The other operands > > > > used are of type uint32_t. Therefore, these checks get promoted to > > > > int64_t and the overflow being tested is undefined because it uses > > > > signed arithmetic. > > > > > > > > I think that the below diff fixes the overflow check. However, I'm > > > > pretty tired at the moment and this is hard to do right, so review is > > > > appreciated. > > > > > > If you know that lt->ia6t_[pv]time will stay an uint32_t forever, > > > then isn't checking for (time_second > INT64_MAX - lt->ia6t_vltime) > > > enough? The right side of the comparison will always be > 0 then. > > > If we somehow had time_second < 0, then your sanity check would still > > > work without checking for time_second > 0. > > > Don't think we ever have time_second < 0 though, since it's the > > > seconds since the Epoch. > > > > Does your reasoning also apply to time_uptime? Because it might make > > sense to first convert this code to use time_uptime first to avoid leaps > > that might be triggered by clock_settime(2) or settimeofday(2) for example. > > I think it holds. time_uptime and time_second are both updated in > tc_windup(), and as far as I can tell after a quick read of > the code, time_uptime is only added to, as is reasonable to expect :-) > > > Such work as been done in NetBSD as far as I know. > > The netinet/in6.c of NetBSD uses time_uptime. Some other things I noted > while looking at NetBSD below. Can't judge how these findings > apply to our tree though. > > But mentioning lifetimes and expire times, your time_yptime suggestion > sounds reasonable to me in this context. > > It looks like NetBSD removed the SIOCSIFALIFETIME_IN6 ioctl a long time > ago, along with the overflow checks, saying that this ioctl could never > have worked: > http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/netinet6/in6.c?rev=1.132&content-type=text/x-cvsweb-markup&only_with_tag=MAIN
Indeed we should remove it as well.
