linux-kernel - Re: [RFC PATCH] checkpatch: correctly detect lines of help text

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-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 02 Dec 2020 10:54:24 -0800
From:   Joe Perches <joe@...ches.com>
To:     Nicolai Fischer <nicolai.fischer@....de>,
        linux-kernel@...r.kernel.org, Andi Kleen <ak@...ux.intel.com>
Cc:     apw@...onical.com, johannes.czekay@....de,
        linux-kernel@...cs.fau.de
Subject: Re: [RFC PATCH] checkpatch: correctly detect lines of help text

On Wed, 2020-12-02 at 19:27 +0100, Nicolai Fischer wrote:
> Currently, checkpatch uses keywords to determine the end
> of a Kconfig help message which leads to false positives:
> 
> 1) if a line of the help text starts with any of the keywords, e.g. if:
> 
> +config FOO
> +	help
> +	  help text
> +	  if condition
> +	  previous line causes warning
> +	  last line.
> 
> 2) if the help attribute is not specified last, checkpatch counts
> other attributes like depends on towards the line count:
> 
> +config FOO
> +	help
> +	bool "no help message, but passes checkpatch"
> +	default n
> +	depends on SYSFS
> +	depends on MULTIUSER

Perhaps it'd be better to create a new warning when the help text
block is not the last block of the config section.  Maybe warn when
a blank line or endif is not the separator to the next keyword.
Maybe warn when the next line after help is not indented 2 more
spaces than the help line.

> This patch fixes this behavior by using the indentation to determine
> the end of the help message.

This probably won't work, see below:

> The code responsible for counting the lines of the help message
> seems overly complicated and we could rewrite it entirely
> in order to be more clear and compact if requested.

Yes please.

> This could potentially be addressed in the warning message,
> though we are happy for any input on this.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3234,6 +3234,7 @@ sub process {
>  			my $f;
>  			my $is_start = 0;
>  			my $is_end = 0;
> +			my $help_indent;
>  			for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) {
>  				$f = $lines[$ln - 1];
>  				$cnt-- if ($lines[$ln - 1] !~ /^-/);
> @@ -3245,7 +3246,12 @@ sub process {
>  				if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) {
>  					$is_start = 1;
>  				} elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) {

I believe all the '---help---' lines have been converted to just 'help'
so the '(?:---)?' bits here could be removed.

See:

commit 22a4ac026c15eba961883ed8466cb341e0447de1
Author: Masahiro Yamada <masahiroy@...nel.org>
Date:   Wed Jun 17 12:02:20 2020 +0900

    Revert "checkpatch: kconfig: prefer 'help' over '---help---'"
    
    This reverts commit 84af7a6194e493fae312a2b7fa5a3b51f76d9282.
    
    The conversion is done.
    
    Cc: Ulf Magnusson <ulfalizer@...il.com>
    Signed-off-by: Masahiro Yamada <masahiroy@...nel.org>

> @@ -3253,14 +3259,13 @@ sub process {
>  				$f =~ s/^\s+//;
>  				next if ($f =~ /^$/);
>  
> 
> -				# This only checks context lines in the patch
> -				# and so hopefully shouldn't trigger false
> -				# positives, even though some of these are
> -				# common words in help texts
> -				if ($f =~ /^\s*(?:config|menuconfig|choice|endchoice|
> -						  if|endif|menu|endmenu|source)\b/x) {
> -					$is_end = 1;
> -					last;
> +				# Help text ends if a line has a smaller indentation
> +				# than the first line of the message
> +				if (defined $help_indent) {
> +					if ($lines[$ln - 1] !~ /^\+$help_indent\S+/) {
> +						$is_end = 1;
> +						last;
> +					}

Indentation can vary in the help blocks.  For instance:

arch/Kconfig:   help
arch/Kconfig-     Functions will have the stack-protector canary logic added in>
arch/Kconfig-     of the following conditions:
arch/Kconfig-
arch/Kconfig-     - local variable's address used as part of the right hand sid>
arch/Kconfig-       assignment or function argument
arch/Kconfig-     - local variable is an array (or union containing an array),
arch/Kconfig-       regardless of array type or length
arch/Kconfig-     - uses register local variables
arch/Kconfig-

This doesn't allow blank lines for multi-paragraph help text either.

I think keyword parsing is necessary and some false positives are
inevitable as the parsing logic in a line-by-line analyzer will
always be incomplete.