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 These assignments are also there in in6_update_ifa(), but without overflow checks, it seems. This routine is called when doing a SIOCAIFADDR_IN6 ioctl(). NetBSD has them also. if (ia6->ia6_lifetime.ia6t_vltime != ND6_INFINITE_LIFETIME) { ia6->ia6_lifetime.ia6t_expire = time_second + ia6->ia6_lifetime.ia6t_vltime; } else ia6->ia6_lifetime.ia6t_expire = 0; if (ia6->ia6_lifetime.ia6t_pltime != ND6_INFINITE_LIFETIME) { ia6->ia6_lifetime.ia6t_preferred = time_second + ia6->ia6_lifetime.ia6t_pltime;
