Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 7 Dec 2013 21:39:52 -0800
From:      Justin Hibbits <jhibbits@freebsd.org>
To:        Bruce Evans <brde@optusnet.com.au>
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:  <CAHSQbTBJ9%2BSNLqkM-FMLwGiGUC%2BJbiCcZEcTkJoBy_kS_33svw@mail.gmail.com>
In-Reply-To: <20131208123144.U883@besplex.bde.org>
References:  <201312071955.rB7JtYNH001792@svn.freebsd.org> <20131208123144.U883@besplex.bde.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Dec 7, 2013 6:54 PM, "Bruce Evans" <brde@optusnet.com.au> wrote:
>
> 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.
>

Sorry, I missed that. I will fix it tonight when I get home. Very much a
pointy hat to me.

> 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.

Only purpose was to fix the clang warning. I will look at this though.

>
> % 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

I will fix all the bugs I introduced tonight or tomorrow. The rest I will
either look at or file for later. Sorry for the breakage.

-Justin



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAHSQbTBJ9%2BSNLqkM-FMLwGiGUC%2BJbiCcZEcTkJoBy_kS_33svw>