make checkpatch warn about memset with swapped arguments.

make checkpatch warn about memset with swapped arguments.

[Dave Jones]

Because the second and third arguments of memset have the same type,
it turns out to be really easy to mix them up.

This bug comes up time after time, so checkpatch should really
be checking for it at patch submission time.

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 58848e3..421b3e69 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2902,6 +2902,11 @@ sub process {
 		    $line =~ /DEVICE_ATTR.*S_IWUGO/ ) {
 			WARN("Exporting world writable files is usually an error. Consider more restrictive permissions.\n" . $herecurr);
 		}
+
+		# Check for memset with swapped arguments
+		if ($line =~ /memset.*\,(\ |)(0x|)0(\ |0|)\);/) {
+			ERROR("memset size is 3rd argument, not the second.\n" . $herecurr);
+		}
 	}
 
 	# If we have no input at all, then there is nothing to report on

Re: make checkpatch warn about memset with swapped arguments.

[Steven Rostedt]

On Thu, Mar 17, 2011 at 12:52:09AM -0400, Dave Jones wrote:
> Because the second and third arguments of memset have the same type,
> it turns out to be really easy to mix them up.
> 
> This bug comes up time after time, so checkpatch should really
> be checking for it at patch submission time.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 58848e3..421b3e69 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2902,6 +2902,11 @@ sub process {
>  		    $line =~ /DEVICE_ATTR.*S_IWUGO/ ) {
>  			WARN("Exporting world writable files is usually an error. Consider more restrictive permissions.\n" . $herecurr);
>  		}
> +
> +		# Check for memset with swapped arguments
> +		if ($line =~ /memset.*\,(\ |)(0x|)0(\ |0|)\);/) {

Wouldn't this be a better regex:

	if ($line =~ /memset.*\,\s*(0x)?0\s*\)\s*;/)

-- Steve

> +			ERROR("memset size is 3rd argument, not the second.\n" . $herecurr);
> +		}
>  	}
>  
>  	# If we have no input at all, then there is nothing to report on
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: make checkpatch warn about memset with swapped arguments.

[Dave Jones]

On Thu, Mar 17, 2011 at 03:36:45PM -0400, Steven Rostedt wrote:

 > > +		if ($line =~ /memset.*\,(\ |)(0x|)0(\ |0|)\);/) {
 > 
 > Wouldn't this be a better regex:
 > 
 > 	if ($line =~ /memset.*\,\s*(0x)?0\s*\)\s*;/)

I dunno, regexps are all gobble-de-gook to me.  Why is it better ?

	Dave
 

Re: make checkpatch warn about memset with swapped arguments.

[Steven Rostedt]

On Thu, 2011-03-17 at 16:02 -0400, Dave Jones wrote:
> On Thu, Mar 17, 2011 at 03:36:45PM -0400, Steven Rostedt wrote:
> 
>  > > +		if ($line =~ /memset.*\,(\ |)(0x|)0(\ |0|)\);/) {
>  > 
>  > Wouldn't this be a better regex:
>  > 
>  > 	if ($line =~ /memset.*\,\s*(0x)?0\s*\)\s*;/)
> 
> I dunno, regexps are all gobble-de-gook to me.  Why is it better ?

:) I love regex :)

But the reason for better is more robust. This will catch other bad
things users may do, like having tabs and such instead of spaces. Or
they may have more than one space. Although this should be caught by
other errors or warnings in checkpatch.

\s stands for any whitespace, 
so ",\s*x" is ,[any-amount-of-white-space]x"

The (0x)? is common for (0x|) in perl, but either is fine. I'm just use
to the '?' one.

-- Steve

Re: make checkpatch warn about memset with swapped arguments.

[Andrew Morton]

On Thu, 17 Mar 2011 16:37:04 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 2011-03-17 at 16:02 -0400, Dave Jones wrote:
> > On Thu, Mar 17, 2011 at 03:36:45PM -0400, Steven Rostedt wrote:
> > 
> >  > > +		if ($line =~ /memset.*\,(\ |)(0x|)0(\ |0|)\);/) {
> >  > 
> >  > Wouldn't this be a better regex:
> >  > 
> >  > 	if ($line =~ /memset.*\,\s*(0x)?0\s*\)\s*;/)
> > 
> > I dunno, regexps are all gobble-de-gook to me.  Why is it better ?
> 
> :) I love regex :)
> 
> But the reason for better is more robust. This will catch other bad
> things users may do, like having tabs and such instead of spaces. Or
> they may have more than one space. Although this should be caught by
> other errors or warnings in checkpatch.
> 
> \s stands for any whitespace, 
> so ",\s*x" is ,[any-amount-of-white-space]x"
> 
> The (0x)? is common for (0x|) in perl, but either is fine. I'm just use
> to the '?' one.
> 

Dave's patch is tested (I assume), so it wins ;)

Maybe it's just me, but I think it would be better to use bzero() for
this operation - it's more readable and it can't be screwed up.  Then
checkpatch checks for memset(xxx, 0, xxx) and for memset(xxx, xxx, 0)
and says "hey, use bzero".  

Re: make checkpatch warn about memset with swapped arguments.

[Dave Jones]

On Thu, Mar 17, 2011 at 02:11:08PM -0700, Andrew Morton wrote:
 
 > Dave's patch is tested (I assume), so it wins ;)

indeed, it's the one I've been using for.. ages.

 > Maybe it's just me, but I think it would be better to use bzero() for
 > this operation - it's more readable and it can't be screwed up.  Then
 > checkpatch checks for memset(xxx, 0, xxx) and for memset(xxx, xxx, 0)
 > and says "hey, use bzero".  

god yes.  I don't know why it fell out of favor in userspace[*], but we
don't have to follow suit in the kernel.  I thought we actually had
a generic bzero, but it seems not from a quick grep.  I'll hack something up.

it only needs to be a #define bzero(x,y)	memset (x, 0, y);

where should it live, linux/kernel.h ?

	Dave


[*]  man bzero shows.. "This function is deprecated", which is possibly one
     of the dumbest moves ever, given how commonplace this bug is.

Re: make checkpatch warn about memset with swapped arguments.

[Andrew Morton]

On Thu, 17 Mar 2011 17:32:52 -0400
Dave Jones <davej@redhat.com> wrote:

> On Thu, Mar 17, 2011 at 02:11:08PM -0700, Andrew Morton wrote:
>  
>  > Dave's patch is tested (I assume), so it wins ;)
> 
> indeed, it's the one I've been using for.. ages.
> 
>  > Maybe it's just me, but I think it would be better to use bzero() for
>  > this operation - it's more readable and it can't be screwed up.  Then
>  > checkpatch checks for memset(xxx, 0, xxx) and for memset(xxx, xxx, 0)
>  > and says "hey, use bzero".  
> 
> god yes.  I don't know why it fell out of favor in userspace[*], but we
> don't have to follow suit in the kernel.  I thought we actually had
> a generic bzero, but it seems not from a quick grep.  I'll hack something up.
> 
> it only needs to be a #define bzero(x,y)	memset (x, 0, y);
> 
> where should it live, linux/kernel.h ?

Thou shalt not implement in CPP that which can be implemented in C.

and..

Thou shalt not implement in C that which is already a gcc builtin :)

I'm OK with switching from memset to bzero as the preferred way of
clearing memory (shorter, clearer, not susceptible to the arg reversal
bug).  Is Linus?

Re: make checkpatch warn about memset with swapped arguments.

[Steven Rostedt]

On Thu, 2011-03-17 at 14:57 -0700, Andrew Morton wrote:

> I'm OK with switching from memset to bzero as the preferred way of
> clearing memory (shorter, clearer, not susceptible to the arg reversal
> bug).  Is Linus?

I suspect that a lot of memset(foo, 0, size); are before a kmalloc and
can also be replaced with kzalloc();

-- Steve

Re: make checkpatch warn about memset with swapped arguments.

[Steven Rostedt]

On Thu, 2011-03-17 at 14:11 -0700, Andrew Morton wrote:
> On Thu, 17 Mar 2011 16:37:04 -0400

> 
> Dave's patch is tested (I assume), so it wins ;)

I did test mine :) Checked it out before posting. Although I didn't make
a patch out of it.

> 
> Maybe it's just me, but I think it would be better to use bzero() for
> this operation - it's more readable and it can't be screwed up.  Then
> checkpatch checks for memset(xxx, 0, xxx) and for memset(xxx, xxx, 0)
> and says "hey, use bzero".  

Honestly, I've never had the issues with memset. Maybe back in college.
But heh, whatever.

-- Steve

Re: make checkpatch warn about memset with swapped arguments.

[Dave Jones]

On Thu, Mar 17, 2011 at 05:38:16PM -0400, Steven Rostedt wrote:

 > > Maybe it's just me, but I think it would be better to use bzero() for
 > > this operation - it's more readable and it can't be screwed up.  Then
 > > checkpatch checks for memset(xxx, 0, xxx) and for memset(xxx, xxx, 0)
 > > and says "hey, use bzero".  
 > 
 > Honestly, I've never had the issues with memset. Maybe back in college.
 > But heh, whatever.

You're smarter than the average bear.

Given how often this bug turns up in both userspace and kernelspace,
it's a landmine waiting for someone to step on.

	Dave
 

Re: make checkpatch warn about memset with swapped arguments.

[Andi Kleen]

Dave Jones <davej@redhat.com> writes:

> Because the second and third arguments of memset have the same type,
> it turns out to be really easy to mix them up.
>
> This bug comes up time after time, so checkpatch should really
> be checking for it at patch submission time.

Or we just readd an optimized bzero() and recommend people use that
instead of memset for zero? And then there won't be too many users left.

I always felt classic BSD was much more sensible regarding this than
ANSI-C.

It's better to avoid errors in the first place than to check for
them later.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only