make checkpatch warn about memset with swapped arguments.
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.
scripts/checkpatch.pl +5 -0
@@ -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
Quoted 13 lines · 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 <[email protected]>scripts/checkpatch.pl +0
-0
scripts/checkpatch.pl sub process { · line 2907
Quoted 6 lines
scripts/checkpatch.pl +3
-0@@ -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
Quoted 10 lines
scripts/checkpatch.pl +2
-1+ 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Quoted 5 lines · Steven Rostedt
scripts/checkpatch.pl sub process { · line 2907
Quoted 1 line
scripts/checkpatch.pl +1
-0+ 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
Quoted 9 lines · Dave Jones
Quoted 5 lines · Steven Rostedt
scripts/checkpatch.pl sub process { · line 2907
Quoted 1 line
scripts/checkpatch.pl +1
-0+ 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
Quoted 18 lines · On Thu, 17 Mar 2011 16:37:04 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 9 lines · Dave Jones
Quoted 5 lines · Steven Rostedt
scripts/checkpatch.pl sub process { · line 2907
Quoted 1 line
scripts/checkpatch.pl +1
-0+ 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.
Quoted 6 lines
\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".
Quoted 1 line · Andrew Morton
Dave's patch is tested (I assume), so it wins ;)
indeed, it's the one I've been using for.. ages.
Quoted 4 lines
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 formemset(xxx, 0, xxx)and formemset(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.
Quoted 4 lines · Andrew Morton
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.
Quoted 5 lines
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 formemset(xxx, 0, xxx)and formemset(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
Quoted 7 lines · Steven Rostedt
Quoted 4 lines
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 formemset(xxx, 0, xxx)and formemset(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
Quoted 11 lines · On Thu, 17 Mar 2011 17:32:52 -0400 Dave Jones <[email protected]> wrote:
Quoted 1 line · Andrew Morton
Dave's patch is tested (I assume), so it wins ;)
indeed, it's the one I've been using for.. ages.
Quoted 4 lines
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 formemset(xxx, 0, xxx)and formemset(xxx, xxx, 0)and says "hey, use bzero".
Quoted 7 lines
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?
Quoted 3 lines · Andrew Morton
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
Quoted 5 lines · 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.
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
Signature · 1 line
[email protected] -- Speaking for myself only