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. Such work as been done in NetBSD as far as I know.
