linux-kernel - Re: [PATCH] checkpatch: check for missing \n at the end of logging message

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: <60c732a1-aa4e-afab-d223-894a67713003@wanadoo.fr>
Date:   Thu, 9 Apr 2020 09:24:22 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Joe Perches <joe@...ches.com>, apw@...onical.com,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] checkpatch: check for missing \n at the end of logging
 message

Le 09/04/2020 à 05:10, Joe Perches a écrit :
> On Wed, 2020-04-08 at 22:23 +0200, Marion & Christophe JAILLET wrote:
>> Le 08/04/2020 à 04:14, Joe Perches a écrit :
>>> This works rather better:
>>>
>>> Perhaps you could test?
>>> ---
>>>
>>> v2:
>>>
>>> o Avoid pr_cont
>>> o Use only last format line if split across multiple lines
>>>
>>>    scripts/checkpatch.pl | 22 ++++++++++++++++++++++
>>>    1 file changed, 22 insertions(+)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index d64c67..f00a6c8 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -5673,6 +5673,28 @@ sub process {
>>>    			}
>>>    		}
>>>    
>>> +# check for possible missing newlines at the end of common logging functions
>>> +		if (defined($stat) &&
>>> +		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
>>> +		    $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
>>> +			my $cnt = statement_rawlines($stat);
>>> +			my $extracted_string = "";
>>> +			for (my $i = 0; $i < $cnt; $i++) {
>>> +				next if ($lines[$linenr + $i - 1] !~ /$String\s*[,\)]/);
>>> +				$extracted_string = get_quoted_string($lines[$linenr + $i - 1],
>>> +								      $rawlines[$linenr + $i - 1]);
>>> +				last if ($extracted_string ne "");
>>> +			}
>>> +			if ($extracted_string ne "" && $extracted_string !~ /\\n"$/) {
>>> +				my $herectx = $here . "\n";
>>> +				for (my $n = 0; $n < $cnt; $n++) {
>>> +					$herectx .=  raw_line($linenr, $n) . "\n";
>>> +				}
>>> +				WARN("MISSING_FORMAT_NEWLINE",
>>> +				     "Possible missing '\\n' at the end of a logging message format string\n" . $herectx);
>>> +			}
>>> +		}
>>> +
>>>    # check for logging functions with KERN_<LEVEL>
>>>    		if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ &&
>>>    		    $line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) {
>>>
>> For what I wanted to check and according to the few tests I've made, it
>> looks fine.
>>
>> Thank you very much for sharing this much more robust (and working)
>> alternative.
>>
>> For what it worth: (i.e. much more tests should be done)
>> Tested-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
> Then I think you really haven't tested it very thoroughly.
>
> For instance:
>
> $ git ls-files -- 'drivers/*.[ch]' | \
>    xargs ./scripts/checkpatch.pl -f --quiet --no-summary --types=MISSING_FORMAT_NEWLINE
>
> emits many false positives.
>
> Some types of false positives:
>
> o Many of the formats seem to end in a ':' or a ' '
>    maybe those should be excluded
>     #86: FILE: drivers/android/binder_alloc_selftest.c:86:
>     +	pr_err("free seq: ");
>
>     o Split string formats should be excluded better
>       as only the first string fragment is checked:
>     #1001: FILE: drivers/ata/pata_octeon_cf.c:1001:
>     +	dev_info(&pdev->dev, "version " DRV_VERSION" %d bit%s.\n",
>     +		 is_16bit ? 16 : 8,
>     +		 cf_port->is_true_ide ? ", True IDE" : "");
>
>     probably a few others, including a desire to check
>     if a pr_cont is below the use within a few lines.
>
>     > Maybe, at least a Suggested-By: would be appreciated.
>
> No worries, when it's cooked, it'll have that.
>
> cheers, Joe
>
>
I think that, at least printk(), WARN() and co, and panic() should also 
be handled the same way.


A few files (5 according to my grep) also have something like:
    #define pr_fmt(fmt) "bcache: %s()" fmt "\n", __func__
and then sometimes a mix of pr_xxx() with either trailing \n or not.

Maybe those should be handled manually to be consistent and avoid a "\n" 
in pr_fmt which is not widely used in other files

CJ