linux-kernel - Re: Possible code defects: macros and precedence

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 17 Sep 2016 14:57:52 -0700
From:   Joe Perches <joe@...ches.com>
To:     Julia Lawall <julia.lawall@...6.fr>
Cc:     Dan Carpenter <error27@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: Possible code defects: macros and precedence

On Sat, 2016-09-17 at 22:24 +0200, Julia Lawall wrote:

(A 2.2MB message that (perhaps thankfully) didn't get through to lkml)

> Below is the Coccinelle output for the case where the definition of the
> macro is a single expression.  There is also the case where it is a
> sequence of statements, but that finds very few results.  Note that
> Coccinelle will only match code that it can parse, which is definitely not
> always the case for macros, so some things may be missed.
> 
> There are a huge number of results.  To the extent that you have the
> patience to look through them, do you see anything undesirable?
> 
> thanks,
> julia
> 
> diff -u -p a/lib/lz4/lz4defs.h b/lib/lz4/lz4defs.h
> --- a/lib/lz4/lz4defs.h
> +++ b/lib/lz4/lz4defs.h
> @@ -34,7 +34,7 @@ typedef struct _U64_S { u64 v; } U64_S;
>  #define PUT8(s, d) (A64(d) = A64(s))
> 
>  #define LZ4_READ_LITTLEENDIAN_16(d, s, p)      \
> -       (d = s - A16(p))
> +       (d = (s) - A16(p))
> 
>  #define LZ4_WRITE_LITTLEENDIAN_16(p, v)        \
>         do {    \
> @@ -53,7 +53,7 @@ typedef struct _U64_S { u64 v; } U64_S;
>         put_unaligned(get_unaligned((const u64 *) s), (u64 *) d)
> 
>  #define LZ4_READ_LITTLEENDIAN_16(d, s, p)      \
> -       (d = s - get_unaligned_le16(p))
> +       (d = (s) - get_unaligned_le16(p))
> 
>  #define LZ4_WRITE_LITTLEENDIAN_16(p, v)                        \
>         do {                                            \

Here's the equivalent checkpatch output for that file.
It has a few more instances.
Is what checkpatch suggests unreasonable?

$ ./scripts/checkpatch.pl -f --strict lib/lz4/lz4defs.h --types=macro_arg_precedence
CHECK: Macro argument 's' may be better as '(s)' to avoid precedence issues
#36: FILE: lib/lz4/lz4defs.h:36:
+#define LZ4_READ_LITTLEENDIAN_16(d, s, p)	\
+	(d = s - A16(p))

CHECK: Macro argument 's' may be better as '(s)' to avoid precedence issues
#55: FILE: lib/lz4/lz4defs.h:55:
+#define LZ4_READ_LITTLEENDIAN_16(d, s, p)	\
+	(d = s - get_unaligned_le16(p))

CHECK: Macro argument 'd' may be better as '(d)' to avoid precedence issues
#106: FILE: lib/lz4/lz4defs.h:106:
+#define LZ4_SECURECOPY(s, d, e)			\
+	do {					\
+		if (d < e) {			\
+			LZ4_WILDCOPY(s, d, e);	\
+		}				\
+	} while (0)

CHECK: Macro argument 'e' may be better as '(e)' to avoid precedence issues
#106: FILE: lib/lz4/lz4defs.h:106:
+#define LZ4_SECURECOPY(s, d, e)			\
+	do {					\
+		if (d < e) {			\
+			LZ4_WILDCOPY(s, d, e);	\
+		}				\
+	} while (0)

CHECK: Macro argument 'e' may be better as '(e)' to avoid precedence issues
#147: FILE: lib/lz4/lz4defs.h:147:
+#define LZ4_WILDCOPY(s, d, e)		\
+	do {				\
+		LZ4_COPYPACKET(s, d);	\
+	} while (d < e)

CHECK: Macro argument 'l' may be better as '(l)' to avoid precedence issues
#152: FILE: lib/lz4/lz4defs.h:152:
+#define LZ4_BLINDCOPY(s, d, l)	\
+	do {	\
+		u8 *e = (d) + l;	\
+		LZ4_WILDCOPY(s, d, e);	\
+		d = e;	\
+	} while (0)

total: 0 errors, 0 warnings, 6 checks, 157 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

lib/lz4/lz4defs.h has style problems, please review.

NOTE: Used message types: MACRO_ARG_PRECEDENCE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.