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.
In general, I'm not sure if rewriting these checks using *_MAX constants
is the best way to do it, because we'd have to review all of them once the
types of variables involved in the check change.
Clang and newer gcc versions have __builtin_add_overflow, but it's
compiler specific.
One final note, if we really care about that overflow check being
performed in in6_control:
(So this is about the overflow check here itself, not your diff).
The assignments:
ia6->ia6_lifetime.ia6t_expire =
time_second + ia6->ia6_lifetime.ia6t_vltime;
ia6->ia6_lifetime.ia6t_preferred =
time_second + ia6->ia6_lifetime.ia6t_pltime;
are done after the overflow checks. time_second is updated by
hardclock(), which is triggered by the timer interrupt, AFAIK.
If this code runs with interrupts enabled, time_second can yield
a different value for every read access. So the computation might
still overflow in this case.
Practically this should only overflow now when somebody sets the clock
far into the future.
I suppose this check comes from times where time_t was 32 bit.
> Index: netinet6/in6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/in6.c,v
> retrieving revision 1.183
> diff -u -p -r1.183 in6.c
> --- netinet6/in6.c 21 Jan 2016 11:23:48 -0000 1.183
> +++ netinet6/in6.c 12 Feb 2016 19:44:10 -0000
> @@ -70,6 +70,7 @@
> #include <sys/socket.h>
> #include <sys/socketvar.h>
> #include <sys/sockio.h>
> +#include <sys/stdint.h>
> #include <sys/mbuf.h>
> #include <sys/systm.h>
> #include <sys/time.h>
> @@ -348,11 +349,13 @@ in6_control(struct socket *so, u_long cm
> /* sanity for overflow - beware unsigned */
> lt = &ifr->ifr_ifru.ifru_lifetime;
> if (lt->ia6t_vltime != ND6_INFINITE_LIFETIME
> - && lt->ia6t_vltime + time_second < time_second) {
> + && (time_second > 0
> + && time_second > INT64_MAX - lt->ia6t_vltime)) {
> return EINVAL;
> }
> if (lt->ia6t_pltime != ND6_INFINITE_LIFETIME
> - && lt->ia6t_pltime + time_second < time_second) {
> + && (time_second > 0
> + && time_second > INT64_MAX - lt->ia6t_pltime)) {
> return EINVAL;
> }
> break;
>
