Re: Possible code defects: macros and precedence

[Joe Perches]

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: