[PATCH] Fix integer truncation in systat -ifstat

[Bruce Evans]

On Thu, 11 Sep 2014, Ryan Stone wrote:

> systat -ifstat currently truncates byte counters down to 32-bit
> integers.  The following fixes the issue, but I'm not very happy with
> it.  u_long is what the rest of our code uses for network counters,
> but that ends up meaning that our counters are 32-bits wide on 32-bit
> platforms.  I could make it uint64_t but that's not very future proof.
> RIght now I'm leaning towards punting on the issue and using u_long as
> there is an awful lot of code that would have to be modified for
> extended byte counters to actually work on all platforms.

Only differences in the counters are used except in 1 place that is
broken in other ways, so overflow is only a large problem starting at
about 40 Gbps.  At only 10 Gbps, 32-bit counters are enough with a
refresh interval of 1 second but not quite enough with the default
interval of 5 seconds (this default is not documented in the man
page.  It seems to only be documented (with a grammar error -- comma
splice) in the status message for mode switches).  5 seconds at
nearly 1.125 GBps exceeds UINT32_MAX.  Packet counter overflow isn't
a problem until about 600 Gbps with the default interval.  32-bit
systems would have other problems supporting 600 GBps interfaces.

> [rstone at rstone-laptop systat]svn diff
> Index: ifstat.c
> ===================================================================
> --- ifstat.c    (revision 271439)
> +++ ifstat.c    (working copy)
> @@ -269,8 +269,8 @@
>        struct  if_stat *ifp = NULL;
>        struct  timeval tv, new_tv, old_tv;
>        double  elapsed = 0.0;
> -       u_int   new_inb, new_outb, old_inb, old_outb = 0;
> -       u_int   new_inp, new_outp, old_inp, old_outp = 0;
> +       u_long  new_inb, new_outb, old_inb, old_outb = 0;
> +       u_long  new_inp, new_outp, old_inp, old_outp = 0;
>
>        SLIST_FOREACH(ifp, &curlist, link) {
>                /*

u_long was technically and practically correct in 1990 when long was
the largest integer type and the kernel counters had type u_long.
Except u_long was too large then (it should actually be long, thus
2*32 bits on 32-bit machines, making it too large and slow to use for
almost anything including these counters then).  Now the counters have
type uint64_t in the kernel, but apparently not many applications kept
up with this change (I think netstat did).

DIfferences between these counters are assigned to struct member
variables like if_in_curtraffic.  These haven't kept up with the change
either, but they were correct in 1990 since they have type u_long.

The place that is broken in other ways:

% 		/* Display interface if it's received some traffic. */
% 		if (new_inb > 0 && old_inb == 0) {
% 			ifp->display = 1;
% 			needsort = 1;
% 		}

The bugs here are very minor:
- "it's" expands to "it is", so it is a grammar and/or semantics error
- in the long term, old_inb always overflows if the interface is used at
   all.  Sometimes it overflows to precisely 0.  This breaks the logic
   for detecting the first activity on the interface.  But the result of
   misdetcting non-first activity as first seems to be harmless -- just
   sort and redisplay.

Bruce