add memset checks to checkpatch.pl

add memset checks to checkpatch.pl

[Dave Jones]

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*\)/ ) {

Re: add memset checks to checkpatch.pl

[Joe Perches]

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);
		}
	}

Re: add memset checks to checkpatch.pl

[Dave Jones]

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

Re: add memset checks to checkpatch.pl

[Joe Perches]

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?

Re: add memset checks to checkpatch.pl

[Dave Jones]

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

Re: add memset checks to checkpatch.pl

[Joe Perches]

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)

Re: add memset checks to checkpatch.pl

[Andy Whitcroft]

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)

Re: add memset checks to checkpatch.pl

[Ævar Arnfjörð Bjarmason]

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