smatch: misreported 'mask and shift to zero'

smatch: misreported 'mask and shift to zero'

[Toomas Soome]

hi!

The smatch check with illumos code has warning:

/code/illumos-gate/usr/src/tools/proto/root_sparc-nd/opt/onbld/bin/sparc/smatch: /code/illumos-gate/usr/src/common/bignum/bignumimpl.c:1410 big_mul_add_vec() warn: mask and shift to zero

The code itself is perfectly legal:

http://src.illumos.org/source/xref/illumos-gate/usr/src/common/bignum/bignumimpl.c#1410

Investigating the issue, I think, I have found the issue:

match_binop2() is testing expressions having the form: (mask & value) >> shift
and the failure case is 'variable >> shift’.

What does happen is, we get incoming expression set to: '(mask & ) >> shift’

and the mask value is not correct.

In our specific error case we do have 64-bit lhs for >> operator, and we do shift 32 bits; the mask value is set to 0xFFFFFFFF, and shifting it by 32 bits, we do end up with match_binop2() reporting the warning.

Since this incoming expression is missing rhs for & expression, the fix is also obvious - add check to make sure we do have both hands for & expression.

diff --git a/usr/src/tools/smatch/src/check_shift_to_zero.c b/usr/src/tools/smatch/src/check_shift_to_zero.c
index 019b06fb75..14cfdc2258 100644
--- a/usr/src/tools/smatch/src/check_shift_to_zero.c
+++ b/usr/src/tools/smatch/src/check_shift_to_zero.c
@@ -44,7 +44,7 @@ static void match_binop2(struct expression *expr)
 {
        struct expression *left;
        struct expression *tmp;
-       sval_t mask, shift;
+       sval_t value, mask, shift;
 
        if (expr->op != SPECIAL_RIGHTSHIFT)
                return;
@@ -58,6 +58,13 @@ static void match_binop2(struct expression *expr)
 
        if (!get_implied_value(expr->right, &shift))
                return;
+       /*
+        * The LHS for expr is assumed to be (mask & value), make sure
+        * we do have both hands.
+        */
+       if (!get_implied_value(left->left, &value))
+               return;
+
        if (!get_implied_value(left->right, &mask))
                return;
 
thanks,
toomas

Re: smatch: misreported 'mask and shift to zero'

[Dan Carpenter]

On Wed, Feb 05, 2020 at 08:52:16AM +0200, Toomas Soome wrote:
> hi!
> 
> The smatch check with illumos code has warning:
> 
> /code/illumos-gate/usr/src/tools/proto/root_sparc-nd/opt/onbld/bin/sparc/smatch: /code/illumos-gate/usr/src/common/bignum/bignumimpl.c:1410 big_mul_add_vec() warn: mask and shift to zero
> 
> The code itself is perfectly legal:
> 
> http://src.illumos.org/source/xref/illumos-gate/usr/src/common/bignum/bignumimpl.c#1410
> 

The problem is that Smatch doesn't handle loops correctly.  The test is
looking for places which do:

	y = (y & 0xff) >> 8;
                 ^^^^     ^

It only cares about these two values.

The problem is that Smatch only parses loops one time.  The canonical
loops are handled so it knows that inside the loop i is in the range
"0 to len - 1".  But it doesn't know that cy1 is changed.  Smatch is
correct that "(cy1 >> (BIG_CHUNK_SIZE / 2))" is going to be zero on the
first iteration but on the second iteration that's not true so it
shouldn't print a warning.

One way to fix this is to use the diff below.  The difference is that
get_value() only considers literals and get_implied_value() looks at
variables as well.  I think in real life the mask value is always a
literal.

regards,
dan carpenter

diff --git a/check_shift_to_zero.c b/check_shift_to_zero.c
index 019b06fb..3552fe87 100644
--- a/check_shift_to_zero.c
+++ b/check_shift_to_zero.c
@@ -58,7 +58,7 @@ static void match_binop2(struct expression *expr)
 
 	if (!get_implied_value(expr->right, &shift))
 		return;
-	if (!get_implied_value(left->right, &mask))
+	if (!get_value(left->right, &mask))
 		return;
 
 	if (mask.uvalue >> shift.uvalue)