| Message ID | 20230526173921.gonna.349-kees@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] checkpatch: Warn about 0-length and 1-element arrays | expand |
On Fri, 2023-05-26 at 10:39 -0700, Kees Cook wrote: > Fake flexible arrays have been deprecated since last millennium. Proper > C99 flexible arrays must be used throughout the kernel so > CONFIG_FORTIFY_SOURCE and CONFIG_UBSAN_BOUNDS can provide proper array > bounds checking. [] > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] > @@ -7430,6 +7430,21 @@ sub process { > } > } > > +# check for array definition/declarations that should use flexible arrays instead > + if ($sline =~ /^[\+ ]\s*}\s*;\s*$/ && > + $prevline =~ /^\+\s*(?:(?:struct|union|enum)\s+$Ident|\}|$Type)\s*$Ident\s*\[\s*(0|1)\s*\]\s*;\s*$/) { I think this is overly complicated and not necessary $prevline =~ /^\+\s*$Type\s*$Ident\s*\[\s*(0|1)\s*\]\s*;\s*$/) { should work no? ($Type already includes this from @typeList): qr{struct\s+$Ident}, qr{union\s+$Ident}, qr{enum\s+$Ident}, > + if ($1 == '0') { > + if (WARN("ZERO_LENGTH_ARRAY", > + "Use C99 flexible arrays instead of zero-length arrays - see https://github.com/KSPP/linux/issues/78\n" . $hereprev) && > + $fix) { > + $fixed[$fixlinenr - 1] =~ s/\[0\]/[]/g; And this $fix doesn't work if the line is struct foo bar[ 0 ]; and the use of /g is odd. Because the message is a WARN and not an ERR, please use "Prefer/over" and not "Use/instead of" $fixed[$fixlinenr - 1] =~ s/\[\s*0\s*\]/[]/; > + } > + } else { > + WARN("ONE_ELEMENT_ARRAY", > + "Use C99 flexible arrays instead of one-element arrays - see https://github.com/KSPP/linux/issues/79\n" . $hereprev); > + } And this could have a $fix change too if (WARN("ONE_ELEMENT_ARRAY", ...) && $fix) { $fixed[$fixlinenr - 1] =~ s/\[\s*1\s*\]/[]/; > + } > + > # nested likely/unlikely calls > if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) { > WARN("LIKELY_MISUSE",
On Fri, May 26, 2023 at 06:38:27PM -0700, Joe Perches wrote: > On Fri, 2023-05-26 at 10:39 -0700, Kees Cook wrote: > > Fake flexible arrays have been deprecated since last millennium. Proper > > C99 flexible arrays must be used throughout the kernel so > > CONFIG_FORTIFY_SOURCE and CONFIG_UBSAN_BOUNDS can provide proper array > > bounds checking. > [] > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > [] > > @@ -7430,6 +7430,21 @@ sub process { > > } > > } > > > > +# check for array definition/declarations that should use flexible arrays instead > > + if ($sline =~ /^[\+ ]\s*}\s*;\s*$/ && > > + $prevline =~ /^\+\s*(?:(?:struct|union|enum)\s+$Ident|\}|$Type)\s*$Ident\s*\[\s*(0|1)\s*\]\s*;\s*$/) { > > I think this is overly complicated and not necessary > > $prevline =~ /^\+\s*$Type\s*$Ident\s*\[\s*(0|1)\s*\]\s*;\s*$/) { > > should work no? > > ($Type already includes this from @typeList): > qr{struct\s+$Ident}, > qr{union\s+$Ident}, > qr{enum\s+$Ident}, Hm, I didn't when I originally tried it. I will double-check. > > > + if ($1 == '0') { > > + if (WARN("ZERO_LENGTH_ARRAY", > > + "Use C99 flexible arrays instead of zero-length arrays - see https://github.com/KSPP/linux/issues/78\n" . $hereprev) && > > + $fix) { > > + $fixed[$fixlinenr - 1] =~ s/\[0\]/[]/g; > > And this $fix doesn't work if the line is struct foo bar[ 0 ]; > and the use of /g is odd. Thanks! > Because the message is a WARN and not an ERR, please use > "Prefer/over" and not "Use/instead of" I will change to ERR, then. We must not use [0]-sized arrays ever. > > $fixed[$fixlinenr - 1] =~ s/\[\s*0\s*\]/[]/; > > + } > > + } else { > > + WARN("ONE_ELEMENT_ARRAY", > > + "Use C99 flexible arrays instead of one-element arrays - see https://github.com/KSPP/linux/issues/79\n" . $hereprev); > > + } > > And this could have a $fix change too > > if (WARN("ONE_ELEMENT_ARRAY", > ...) && > $fix) { > $fixed[$fixlinenr - 1] =~ s/\[\s*1\s*\]/[]/; Almost never is it possible to only fix the struct when it's a 1-element array (due to the impact of the size changes that be present in other code), so I left it out. I'll send a v3. -Kees
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 30b0b4fdb3bf..cfa19bfc857c 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -7430,6 +7430,21 @@ sub process { } } +# check for array definition/declarations that should use flexible arrays instead + if ($sline =~ /^[\+ ]\s*}\s*;\s*$/ && + $prevline =~ /^\+\s*(?:(?:struct|union|enum)\s+$Ident|\}|$Type)\s*$Ident\s*\[\s*(0|1)\s*\]\s*;\s*$/) { + if ($1 == '0') { + if (WARN("ZERO_LENGTH_ARRAY", + "Use C99 flexible arrays instead of zero-length arrays - see https://github.com/KSPP/linux/issues/78\n" . $hereprev) && + $fix) { + $fixed[$fixlinenr - 1] =~ s/\[0\]/[]/g; + } + } else { + WARN("ONE_ELEMENT_ARRAY", + "Use C99 flexible arrays instead of one-element arrays - see https://github.com/KSPP/linux/issues/79\n" . $hereprev); + } + } + # nested likely/unlikely calls if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) { WARN("LIKELY_MISUSE",
Fake flexible arrays have been deprecated since last millennium. Proper C99 flexible arrays must be used throughout the kernel so CONFIG_FORTIFY_SOURCE and CONFIG_UBSAN_BOUNDS can provide proper array bounds checking. Cc: Andy Whitcroft <apw@canonical.com> Cc: Dwaipayan Ray <dwaipayanray1@gmail.com> Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com> Cc: Gustavo A. R. Silva <gustavoars@kernel.org> Fixed-by: Joe Perches <joe@perches.com> Signed-off-by: Kees Cook <keescook@chromium.org> Link: https://lore.kernel.org/r/20230517204530.never.151-kees@kernel.org --- v2: find using "end of struct" marking instead of $context_function (joe) v1: https://lore.kernel.org/all/20230517204530.never.151-kees@kernel.org/ --- scripts/checkpatch.pl | 15 +++++++++++++++ 1 file changed, 15 insertions(+)