Date: Sun, 8 Dec 2013 13:53:30 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Justin Hibbits <jhibbits@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r259080 - in head/sys: dev/iicbus geom/cache geom/journal Message-ID: <20131208123144.U883@besplex.bde.org> In-Reply-To: <201312071955.rB7JtYNH001792@svn.freebsd.org> References: <201312071955.rB7JtYNH001792@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 7 Dec 2013, Justin Hibbits wrote:
> Log:
> Fix some integer signs. These unsigned integers should all be signed.
>
> Found by: clang (powerpc64)
Unsigned integers are indeed used excessively. They should only be used
if values larger than INT_MAX need to be represented, but values larger
than UINT_MAX need not be represented. This is a very small target
(just 1 more bit).
> Modified: head/sys/dev/iicbus/max6690.c
> ==============================================================================
> --- head/sys/dev/iicbus/max6690.c Sat Dec 7 19:39:38 2013 (r259079)
> +++ head/sys/dev/iicbus/max6690.c Sat Dec 7 19:55:34 2013 (r259080)
> @@ -362,7 +362,7 @@ max6690_sensor_sysctl(SYSCTL_HANDLER_ARG
> struct max6690_softc *sc;
> struct max6690_sensor *sens;
> int error;
> - unsigned int temp;
> + int temp;
>
> dev = arg1;
> sc = device_get_softc(dev);
>
This fixes one style bug (non-use of the BSD spelling u_int) but adds
another (int declarations not grouped together).
> Modified: head/sys/geom/cache/g_cache.c
> ==============================================================================
> --- head/sys/geom/cache/g_cache.c Sat Dec 7 19:39:38 2013 (r259079)
> +++ head/sys/geom/cache/g_cache.c Sat Dec 7 19:55:34 2013 (r259080)
> @@ -67,7 +67,7 @@ static u_int g_cache_used_hi = 20;
> static int
> sysctl_handle_pct(SYSCTL_HANDLER_ARGS)
> {
> - u_int val = *(u_int *)arg1;
> + int val;
> int error;
>
> error = sysctl_handle_int(oidp, &val, 0, req);
>
This adds the larger non-style bug that val is used uninitialized.
Compilers should warn about this. This results in stack garbage
being copied out instead of the "old" value.
sysctl_handle_u_int() doesn't exist, so sysctl_handle_int() os often
abused to handle u_int's and worse (sometimes "opaque" types, where
it usually works accidentally if the type is 32 bits, and sometimes
even works accidentally if the type is larger). clang seems to be
complaining about this abuse. Why does it only complain here? Ah,
maybe it is not smart enough to complain about this, but is just
complaining about the (val < 0) range check.
This adds 2 (lexical) style bugs:
- the int declarations are not grouped together
This misses fixing nearby bogus unsigned variables. The cast for these
is more bogus than before. Indeed, the whole function is horrible, and
this change is sort of backwards since it gives a sign mismatch with
the underlying variables:
% static u_int g_cache_used_lo = 5;
% static u_int g_cache_used_hi = 20;
Example of excessive use of unsigned types. These variables only need
to handle percentages between 0 and 100.
% static int
Style bug (missing blank line);
% sysctl_handle_pct(SYSCTL_HANDLER_ARGS)
% {
% u_int val = *(u_int *)arg1;
This is the old version. It has another style bug (initialization in
declaration). The initialization wouldn't have been lost so easily
if it had been in the correct place.
But the type was correct, since it is bug-for-bug constent with the
underlying variables.
% int error;
%
% error = sysctl_handle_int(oidp, &val, 0, req);
sysctl_handle_int() was abused to handle the u_int (arg1 points to
one of the above 2 variables).
% if (error || !req->newptr)
% return (error);
2 style bugs (2 boolean comparisions of non-booleans). Strict KNF doesn't
even use the ! operator to test booleansl; it uses an explicit " == 0"
test for this, though it sometimes doesn't write out "!= 0" for the reverse
test.
% if (val < 0 || val > 100)
% return (EINVAL);
The first test in this is redundant, This is apparently what clang was
complaining about. The warning for that can be fixed by removing the
test. However, I don't like this fix. It depends on the type being
unsigned. The code becomes more fragile, and would break if the type
were changed to the correct one (signed).
% if ((arg1 == &g_cache_used_lo && val > g_cache_used_hi) ||
% (arg1 == &g_cache_used_hi && g_cache_used_lo > val))
% return (EINVAL);
% *(u_int *)arg1 = val;
There are only 2 variables handled by this function, so we use arg1
to set the correct one. However, the previous statement has decided
which of the variables is being set, and we could use that to set it.
Or better, convert arg1 from void * to u_int * up front, and use that
throughout instead of several casts of arg1.
% return (0);
% }
% SYSCTL_PROC(_kern_geom_cache, OID_AUTO, used_lo, CTLTYPE_UINT|CTLFLAG_RW,
Style bugs:
- missing blank line. This is intentional for grouping the function with
its variables, as above, but still ugly.
- missing spaces around binary operator '|'. This bug is too common for
this operator, and for CTLFLAG*. But the spaces are even more needed
for '|' than for '+' or '*', since '|' looks more like an alphanumeric
character.
% &g_cache_used_lo, 0, sysctl_handle_pct, "IU", "");
% SYSCTL_PROC(_kern_geom_cache, OID_AUTO, used_hi, CTLTYPE_UINT|CTLFLAG_RW,
% &g_cache_used_hi, 0, sysctl_handle_pct, "IU", "");
Style bugs:
- non-KNF continuation indent. This bug is too common for SYSCTL*(),
although someone once did a fairly global sweep to fix it for them.
- no description.
This sysctl is over-engineered. A simple SYSCTL_UINT() is enough.
There are many more-critical sysctls that don't have any range checking.
The fix is not to bloat the kernel by adding range checking to them
all. With several thousand sysctls, the range checking code could
easily be over-engineered enough to double the size of an already
massively bloated kernel. OTOH, there may still be runtime errors
from sloppy caclulations using the percentage variables. The
calculations are of the form (x * pct / 100) and not the more robust
but fuzzier (x / 100 * pct). pct will normally be promoted to the
type of x in these expressions (often to 64 bits, and then overflow
is far off). Overflow is only affected by pct being unsigned if x is
int or shorter.
> Modified: head/sys/geom/journal/g_journal.c
> ==============================================================================
> --- head/sys/geom/journal/g_journal.c Sat Dec 7 19:39:38 2013 (r259079)
> +++ head/sys/geom/journal/g_journal.c Sat Dec 7 19:55:34 2013 (r259080)
> @@ -168,7 +168,7 @@ SYSCTL_UINT(_kern_geom_journal_cache, OI
> static int
> g_journal_cache_switch_sysctl(SYSCTL_HANDLER_ARGS)
> {
> - u_int cswitch;
> + int cswitch;
> int error;
>
> cswitch = g_journal_cache_switch;
>
2 style bugs (int variables not grouped; int variables not sorted).
This sysctl is missing most of the style bugs discussed above:
- the transfer variable cswitch is not initialized in its declaration.
Its initialization didn't get deleted.
- the comparisons of the non-booleans are non-boolean.
- the percentage variable is used in the robust way (x / 100 * pct).
- the SYSCTL()s use KNF continuation indentation
- there are spaces around the binary operator '|'
- the SYSCTL()s have descriptions.
This leaves mainly type errors (excessive use of unsigned variables).
There is a different inconsistency: the u_int g_journal_cache_switch
has formatting data "I" instead of "IU". Both will work due to the
range checking and assumptions that the machine is not exotic, but there
is no reason to not to use a consistent format.
Another common style bug/API design error visible here is using
sysctl_handle_int() on generic variables. You have to know that the
variable type is int to use it or u_int, int32_t or uint32_t to abuse
it. The width of the variable is in the sysctl data and is usually
correct. sysctl_handle_opaque() is usable on generic variables but
doesn't work so well (it prevents type checking and has more locking
overheads). When sysctl_handle_int() is used, the width in the sysctl
data should be used to detect size mismatches. It is used, but there
are bugs in the error handling for this (something like ignoring size
mismatches for short i/o's in only 1 direction).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131208123144.U883>