* add memset checks to checkpatch.pl @ 2010-08-18 20:40 Dave Jones 2010-08-18 21:32 ` Joe Perches 0 siblings, 1 reply; 8+ messages in thread From: Dave Jones @ 2010-08-18 20:40 UTC (permalink / raw) To: apw; +Cc: Linux Kernel Occasionally someone goofs the argument order to memset. This patch makes checkpatch catch those. I made memset with size of 0 an error, because it's never correct, whereas memset with a size of 1 isn't technically an incorrect thing to do so I left it as a warning. It may still be better to replace it with a single variable assignment in the false positive cases. Signed-off-by: Dave Jones <davej@redhat.com> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 2039acd..3690173 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2794,6 +2794,14 @@ sub process { } } +# check for memset with reversed 2nd/3rd arguments. + if ($line =~ /memset.*\,(\ |)(0x|)0(\ |0|)\);/) { + ERROR("memset takes size as third argument, not the second.\n" . $herecurr); + } + if ($line =~ /memset.*\,(\ |)(0x|)1\);/) { + WARN("single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr); + } + # check for lockdep_set_novalidate_class if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ || $line =~ /__lockdep_no_validate__\s*\)/ ) { ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: add memset checks to checkpatch.pl 2010-08-18 20:40 add memset checks to checkpatch.pl Dave Jones @ 2010-08-18 21:32 ` Joe Perches 2010-08-18 21:48 ` Dave Jones 0 siblings, 1 reply; 8+ messages in thread From: Joe Perches @ 2010-08-18 21:32 UTC (permalink / raw) To: Dave Jones; +Cc: apw, Linux Kernel On Wed, 2010-08-18 at 16:40 -0400, Dave Jones wrote: > Occasionally someone goofs the argument order to memset. > This patch makes checkpatch catch those. > > I made memset with size of 0 an error, because it's never correct, > whereas memset with a size of 1 isn't technically an incorrect > thing to do so I left it as a warning. It may still be better to replace > it with a single variable assignment in the false positive cases. Maybe something like allows more flexible checking? if ($line =~ /\bmemset\s*\(\s*($Lval)\s*,\s*($Lval)\s*,\s*($Lval)\s*\)/) { my $memset_addr = $1; my $memset_val = $2; my $memset_size = $3; if ($memset_size =~ /(0x|)0$/i) { ERROR("memset uses second argument as constant byte value, not third.\n" . $herecurr); elsif ($memset_size =~ /(0x|)1/i) { WARN("single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr); } } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: add memset checks to checkpatch.pl 2010-08-18 21:32 ` Joe Perches @ 2010-08-18 21:48 ` Dave Jones 2010-08-18 21:57 ` Joe Perches 0 siblings, 1 reply; 8+ messages in thread From: Dave Jones @ 2010-08-18 21:48 UTC (permalink / raw) To: Joe Perches; +Cc: apw, Linux Kernel On Wed, Aug 18, 2010 at 02:32:13PM -0700, Joe Perches wrote: > On Wed, 2010-08-18 at 16:40 -0400, Dave Jones wrote: > > Occasionally someone goofs the argument order to memset. > > This patch makes checkpatch catch those. > > > > I made memset with size of 0 an error, because it's never correct, > > whereas memset with a size of 1 isn't technically an incorrect > > thing to do so I left it as a warning. It may still be better to replace > > it with a single variable assignment in the false positive cases. > > Maybe something like allows more flexible checking? > > if ($line =~ /\bmemset\s*\(\s*($Lval)\s*,\s*($Lval)\s*,\s*($Lval)\s*\)/) { > my $memset_addr = $1; > my $memset_val = $2; > my $memset_size = $3; > if ($memset_size =~ /(0x|)0$/i) { > ERROR("memset uses second argument as constant byte value, not third.\n" . $herecurr); > elsif ($memset_size =~ /(0x|)1/i) { > WARN("single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr); > } > } I'm all for improving my shoddy perl where possible, but this doesn't seem to actually catch any of the test cases I wrote. (it's also missing a } ) Dave ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: add memset checks to checkpatch.pl 2010-08-18 21:48 ` Dave Jones @ 2010-08-18 21:57 ` Joe Perches 2010-08-18 22:17 ` Dave Jones 0 siblings, 1 reply; 8+ messages in thread From: Joe Perches @ 2010-08-18 21:57 UTC (permalink / raw) To: Dave Jones; +Cc: apw, Linux Kernel On Wed, 2010-08-18 at 17:48 -0400, Dave Jones wrote: > On Wed, Aug 18, 2010 at 02:32:13PM -0700, Joe Perches wrote: > > On Wed, 2010-08-18 at 16:40 -0400, Dave Jones wrote: > > > Occasionally someone goofs the argument order to memset. > > if ($line =~ /\bmemset\s*\(\s*($Lval)\s*,\s*($Lval)\s*,\s*($Lval)\s*\)/) { > > my $memset_addr = $1; > > my $memset_val = $2; > > my $memset_size = $3; > > if ($memset_size =~ /(0x|)0$/i) { > > ERROR("memset uses second argument as constant byte value, not third.\n" . $herecurr); > > elsif ($memset_size =~ /(0x|)1/i) { > > WARN("single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr); > > } > > } > I'm all for improving my shoddy perl where possible, but this doesn't seem to actually > catch any of the test cases I wrote. (it's also missing a } ) I intend never to be a perl monk. I notice the missing { before the elsif after I sent it. Oh well. I just typed it in the emailer, so it's not tested at all. Also it's missing a $ after 1 in the second $memset_size test. What are your test cases anyway? Likely $Lval isn't matching things like sizeof(*foo) so this isn't checked: memset(foo, bar, sizeof(*foo)); Maybe Andy has a better idea? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: add memset checks to checkpatch.pl 2010-08-18 21:57 ` Joe Perches @ 2010-08-18 22:17 ` Dave Jones 2010-08-18 23:38 ` Joe Perches 0 siblings, 1 reply; 8+ messages in thread From: Dave Jones @ 2010-08-18 22:17 UTC (permalink / raw) To: Joe Perches; +Cc: apw, Linux Kernel On Wed, Aug 18, 2010 at 02:57:59PM -0700, Joe Perches wrote: > > I'm all for improving my shoddy perl where possible, but this doesn't seem to actually > > catch any of the test cases I wrote. (it's also missing a } ) > > I intend never to be a perl monk. > > I notice the missing { before the elsif after I sent it. > Oh well. > > I just typed it in the emailer, so it's not tested at all. > Also it's missing a $ after 1 in the second $memset_size test. still didn't catch anything for me. > What are your test cases anyway? memset(foo, 0, 10); memset(foo, 10, 0); memset(foo, 1, 10); memset(foo, 10, 1); > Likely $Lval isn't matching things like > sizeof(*foo) so this isn't checked: > > memset(foo, bar, sizeof(*foo)); I chose to just ignore any non integer arguments to keep things simple. Dave ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: add memset checks to checkpatch.pl 2010-08-18 22:17 ` Dave Jones @ 2010-08-18 23:38 ` Joe Perches 2010-09-02 10:52 ` Andy Whitcroft 0 siblings, 1 reply; 8+ messages in thread From: Joe Perches @ 2010-08-18 23:38 UTC (permalink / raw) To: Dave Jones; +Cc: apw, Linux Kernel On Wed, 2010-08-18 at 18:17 -0400, Dave Jones wrote: > On Wed, Aug 18, 2010 at 02:57:59PM -0700, Joe Perches wrote: > > > I'm all for improving my shoddy perl where possible, but this doesn't seem to actually > > > catch any of the test cases I wrote. (it's also missing a } ) > > I intend never to be a perl monk. > > I notice the missing { before the elsif after I sent it. > > Oh well. > > I just typed it in the emailer, so it's not tested at all. > > Also it's missing a $ after 1 in the second $memset_size test. > still didn't catch anything for me. > > What are your test cases anyway? > > memset(foo, 0, 10); > memset(foo, 10, 0); > memset(foo, 1, 10); > memset(foo, 10, 1); > > > Likely $Lval isn't matching things like > > sizeof(*foo) so this isn't checked: > > > > memset(foo, bar, sizeof(*foo)); > > I chose to just ignore any non integer arguments to keep things simple. This seems to work. I think $FuncArg is not great though. Because $FuncArg uses $match_balanced_parens, the match list args are unexpected. $2, $4, and $6 are the args of any memset argument that uses style func(args) ie: memset(addr(foo), val(bar), sizeof(*foo)) Andy, what do you think? Not-signed-off-by: Joe Perches --- scripts/checkpatch.pl | 18 ++++++++++++++++++ 1 files changed, 18 insertions(+), 0 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 2039acd..af7a4f9 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -172,6 +172,10 @@ our $Operators = qr{ &&|\|\||,|\^|\+\+|--|&|\||\+|-|\*|\/|% }x; +our $match_balanced_parens = qr/(\((?:[^\(\)]++|(?-1))*\))/; +our $Function = qr{(?:$Ident\s*$match_balanced_parens)}; +our $FuncArg = qr{(?:$Lval|$Constant|$Function)}; + our $NonptrType; our $Type; our $Declare; @@ -2655,6 +2659,20 @@ sub process { WARN("sizeof(& should be avoided\n" . $herecurr); } +# Check for misused memsets + + if ($line =~ /^\+.*\bmemset\s*\(\s*($FuncArg)\s*,\s*($FuncArg)\s*,\s*($FuncArg)\s*\)/) { + my $memset_addr = $1; + my $memset_val = $3; + my $memset_size = $5; + + if ($memset_size =~ /^(0x|)0$/i) { + ERROR("memset uses second argument as constant byte value, not third.\n" . $herecurr); + } elsif ($memset_size =~ /^(0x|)1$/i) { + WARN("single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr); + } + } + # check for new externs in .c files. if ($realfile =~ /\.c$/ && defined $stat && $stat =~ /^.\s*(?:extern\s+)?$Type\s+($Ident)(\s*)\(/s) ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: add memset checks to checkpatch.pl 2010-08-18 23:38 ` Joe Perches @ 2010-09-02 10:52 ` Andy Whitcroft 2010-09-02 13:17 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 8+ messages in thread From: Andy Whitcroft @ 2010-09-02 10:52 UTC (permalink / raw) To: Joe Perches; +Cc: Dave Jones, Linux Kernel On Wed, Aug 18, 2010 at 04:38:41PM -0700, Joe Perches wrote: > On Wed, 2010-08-18 at 18:17 -0400, Dave Jones wrote: > > On Wed, Aug 18, 2010 at 02:57:59PM -0700, Joe Perches wrote: > > > > I'm all for improving my shoddy perl where possible, but this doesn't seem to actually > > > > catch any of the test cases I wrote. (it's also missing a } ) > > > I intend never to be a perl monk. > > > I notice the missing { before the elsif after I sent it. > > > Oh well. > > > I just typed it in the emailer, so it's not tested at all. > > > Also it's missing a $ after 1 in the second $memset_size test. > > still didn't catch anything for me. > > > What are your test cases anyway? > > > > memset(foo, 0, 10); > > memset(foo, 10, 0); > > memset(foo, 1, 10); > > memset(foo, 10, 1); > > > > > Likely $Lval isn't matching things like > > > sizeof(*foo) so this isn't checked: > > > > > > memset(foo, bar, sizeof(*foo)); > > > > I chose to just ignore any non integer arguments to keep things simple. > > This seems to work. > > I think $FuncArg is not great though. > > Because $FuncArg uses $match_balanced_parens, the > match list args are unexpected. > > $2, $4, and $6 are the args of any memset argument > that uses style func(args) > > ie: memset(addr(foo), val(bar), sizeof(*foo)) > > Andy, what do you think? Generally we deal with these by simplifying the contained bracketed sections. This allows simpler textual comparisons on the result. Something like the patch below. Perhaps you could test the version at the URL below and see if that does what you expected: http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing -apw commit 46e399ce79f2e76578a3208e901eb50727c1e95e Author: Andy Whitcroft <apw@canonical.com> Date: Thu Sep 2 11:47:41 2010 +0100 checkpatch: check for common memset parameter issues Add checks for 0 and 1 used as lengths. Generally these indicate badly ordered parameters. Based on patches by Dave Jones <davej@redhat.com> and Joe Perches <joe@perches.com>. Signed-off-by: Andy Whitcroft <apw@canonical.com> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 5c60e16..6da84cc 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2719,6 +2719,26 @@ sub process { WARN("sizeof(& should be avoided\n" . $herecurr); } +# Check for misused memsets + if (defined $stat && $stat =~ /\bmemset\s*\((.*)\)/s) { + my $args = $1; + + # Flatten any parentheses and braces + while ($args =~ s/\([^\(\)]*\)/10/s || + $args =~ s/\{[^\{\}]*\}/10/s || + $args =~ s/\[[^\[\]]*\]/10/s) + { + } + # Extract the simplified arguments. + my ($ms_addr, $ms_val, $ms_size) = + split(/\s*,\s*/, $args); + if ($ms_size =~ /^(0x|)0$/i) { + ERROR("memset uses second argument as constant byte value, not third.\n" . $herecurr); + } elsif ($ms_size =~ /^(0x|)1$/i) { + WARN("single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr); + } + } + # check for new externs in .c files. if ($realfile =~ /\.c$/ && defined $stat && $stat =~ /^.\s*(?:extern\s+)?$Type\s+($Ident)(\s*)\(/s) ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: add memset checks to checkpatch.pl 2010-09-02 10:52 ` Andy Whitcroft @ 2010-09-02 13:17 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 8+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2010-09-02 13:17 UTC (permalink / raw) To: Andy Whitcroft; +Cc: Joe Perches, Dave Jones, Linux Kernel On Thu, Sep 2, 2010 at 10:52, Andy Whitcroft <apw@canonical.com> wrote: > [...] /^(0x|)0$/i The canonical way to write this is using curlies: /^(0x)?0$/i ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-09-02 13:17 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-08-18 20:40 add memset checks to checkpatch.pl Dave Jones 2010-08-18 21:32 ` Joe Perches 2010-08-18 21:48 ` Dave Jones 2010-08-18 21:57 ` Joe Perches 2010-08-18 22:17 ` Dave Jones 2010-08-18 23:38 ` Joe Perches 2010-09-02 10:52 ` Andy Whitcroft 2010-09-02 13:17 ` Ævar Arnfjörð Bjarmason
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox