linux-kernel - Re: [PATCH v2] checkpatch: extend author Signed-off-by check for split From: header

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]

Message-ID: <alpine.DEB.2.21.2009210940060.7483@felia>
Date:   Mon, 21 Sep 2020 09:49:00 +0200 (CEST)
From:   Lukas Bulwahn <lukas.bulwahn@...il.com>
To:     Dwaipayan Ray <dwaipayanray1@...il.com>
cc:     joe@...ches.com, lukas.bulwahn@...il.com,
        linux-kernel-mentees@...ts.linuxfoundation.org, apw@...onical.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] checkpatch: extend author Signed-off-by check for
 split  From: header


Dwaipayan, just a quick nitpick:

Your subject line has two spaces in front of 'From:'

On Sun, 20 Sep 2020, Dwaipayan Ray wrote:

> Checkpatch did not handle cases where the author From: header
> was split into multiple lines. The author identity could not
> be resolved and checkpatch generated a false NO_AUTHOR_SIGN_OFF
> warning.
> 
> A typical example is Commit e33bcbab16d1 ("tee: add support for

You can write 'Commit' lowercase as 'commit'.

> session's client UUID generation"). When checkpatch was run on
> this commit, it displayed:
> 
> "WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal
> patch author ''"
> 
> This was due to split header lines not being handled properly and
> the author himself wrote in Commit cd2614967d8b ("checkpatch: warn
> if missing author Signed-off-by"):
>

Same here.
 
> "Split From: headers are not fully handled: only the first part
> is compared."
> 
> Support split From: headers by correctly parsing the header
> extension lines. RFC 2822, Section-2.2.3 stated that each extended
> line must start with a WSP character (a space or htab). The solution
> was therefore to concatenate the lines which start with a WSP to
> get the correct long header.
> 
> Suggested-by: Joe Perches <joe@...ches.com>
> Link: https://lore.kernel.org/linux-kernel-mentees/f5d8124e54a50480b0a9fa638787bc29b6e09854.camel@perches.com/
> Signed-off-by: Dwaipayan Ray <dwaipayanray1@...il.com>



On v5.4..v5.7 using data from a previous evaluation, I found 116 commits 
with
the error message "Missing Signed-off-by: line by nominal patch author 
''",
when running ./scripts/checkpatch.pl on v5.9-rc6.


After this patch application, all 116 warnings disappeared with "Missing 
Signed-off-by: line by nominal patch author ''"
and these three new warnings appeared:

322f6a3182d42df18059a89c53b09d33919f755e:35: WARNING: Missing Signed-off-by: line by nominal patch author 'Johnson CH Chen <JohnsonCH.Chen@...a.com>'
18977008f44c66bdd6d20bb432adf71372589303:37: WARNING: Missing Signed-off-by: line by nominal patch author '"Marek Szyprowski via Linux.Kernel.Org" <m.szyprowski=samsung.com@...ux.kernel.org>'
ed3520427f57327f581de0cc28c1c30df08f0103:51: WARNING: Missing Signed-off-by: line by nominal patch author 'Chengguang Xu via Linux-f2fs-devel <linux-f2fs-devel@...ts.sourceforge.net>'

With that said, I think am happy to add my tags:

Reviewed-by: Lukas Bulwahn <lukas.bulwahn@...il.com>
Tested-by: Lukas Bulwahn <lukas.bulwahn@...il.com>

Dwaipayan, you can fix the minor commit message issues above, add the tags 
from Joe and me to the end of your commit message and send the patch as v3 
out to Andrew Morton with everyone sofar as CC. Andrew Morton will pick up 
the patch and make it travel to Linus Torvalds.

Good job so far!

After you did that, let us develop and discuss a plan to refine the 
'trickier' part of false AUTHOR_SIGN_OFF warnings for developer and 
maintainers with some known special author and sign-off procedures.

Lukas

> ---
>  scripts/checkpatch.pl | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 504d2e431c60..9e65d21456f1 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2661,6 +2661,10 @@ sub process {
>  # Check the patch for a From:
>  		if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) {
>  			$author = $1;
> +			my $curline = $linenr;
> +			while(defined($rawlines[$curline]) && ($rawlines[$curline++] =~ /^[ \t]\s*(.*)/)) {
> +				$author .= $1;
> +			}
>  			$author = encode("utf8", $author) if ($line =~ /=\?utf-8\?/i);
>  			$author =~ s/"//g;
>  			$author = reformat_email($author);
> -- 
> 2.27.0
> 
>