Re: Possible code defects: macros and precedence

[Joe Perches <joe@perches.com>]

On Sat, 2016-09-17 at 22:24 +0200, Julia Lawall wrote:
> diff -u -p a/lib/sha1.c b/lib/sha1.c
[]
> @@ -49,18 +49,18 @@
>   * the input data, the next mix it from the 512-bit array.
>   */
>  #define SHA_SRC(t) get_unaligned_be32((__u32 *)data + t)
> -#define SHA_MIX(t) rol32(W(t+13) ^ W(t+8) ^ W(t+2) ^ W(t), 1)
> +#define SHA_MIX(t) rol32(W((t)+13) ^ W((t)+8) ^ W((t)+2) ^ W(t), 1)
> 
>  #define SHA_ROUND(t, input, fn, constant, A, B, C, D, E) do { \
>         __u32 TEMP = input(t); setW(t, TEMP); \
>         E += TEMP + rol32(A,5) + (fn) + (constant); \
>         B = ror32(B, 2); } while (0)
> 
> -#define T_0_15(t, A, B, C, D, E)  SHA_ROUND(t, SHA_SRC, (((C^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> -#define T_16_19(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (((C^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> -#define T_20_39(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (B^C^D) , 0x6ed9eba1, A, B, C, D, E )
> -#define T_40_59(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B&C)+(D&(B^C))) , 0x8f1bbcdc, A, B, C, D, E )
> -#define T_60_79(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (B^C^D) ,  0xca62c1d6, A, B, C, D, E )
> +#define T_0_15(t, A, B, C, D, E)  SHA_ROUND(t, SHA_SRC, ((((C)^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> +#define T_16_19(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((((C)^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> +#define T_20_39(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B)^C^D) , 0x6ed9eba1, A, B, C, D, E )
> +#define T_40_59(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (((B)&C)+((D)&((B)^C))) , 0x8f1bbcdc, A, B, C, D, E )
> +#define T_60_79(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B)^C^D) ,  0xca62c1d6, A, B, C, D, E )

I didn't look at much of the patch.

It looks as if the coccinelle code doesn't do the
transformation on each possible macro argument.

In the transform above, only the first referenced arg
is updated with parentheses and subsequent args are not.

Many or most of the uses of these various macros always
pass a single argument to these macros so there isn't a
real possibility of a precedence defect for those uses.

Is it possible to check the macro users for arguments that
might produce precedence defects and only report those
possible defects?

I also submitted a similar checkpatch addition that looks
for non-comma operators used macro arguments in function
definitions.

The checkpatch test has the same weakness as the coccinelle
test. It doesn't check uses, just the macro definition.

Commits in -next:

>From 75bd22fe82d1fd698894e4a5d21e33ffdf7d4492 Mon Sep 17 00:00:00 2001
From: Joe Perches <joe@perches.com>
Date: Thu, 15 Sep 2016 10:29:22 +1000
Subject: [PATCH] checkpatch: improve MACRO_ARG_PRECEDENCE test

It is possible for a multiple line macro definition to have a false positive
report when an argument is used on a line after a continuation \.

This line might have a leading '+' as the initial character that could be
confused by checkpatch as an operator.

Avoid the leading character on multiple line macro definitions.

Link: http://lkml.kernel.org/r/60229d13399f9b6509db5a32e30d4c16951a60cd.1473836073.git.joe@perches.com
Signed-off-by: Joe Perches <joe@perches.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

>From 0158682614df6006752ac932b2e65475384a87b3 Mon Sep 17 00:00:00 2001
From: Joe Perches <joe@perches.com>
Date: Thu, 15 Sep 2016 10:29:22 +1000
Subject: [PATCH] checkpatch: add --strict test for precedence challenged macro arguments

Add a test for macro arguents that have a non-comma leading or trailing
operator where the argument isn't parenthesized to avoid possible precedence
issues.

Link: http://lkml.kernel.org/r/47715508972f8d786f435e583ff881dbeee3a114.1473745855.git.joe@perches.com
Signed-off-by: Joe Perches <joe@perches.com>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>