checkpatch.pl: fix naked sscanf false positives

PATCH In review Kevin Wern 3 participants 8 messages Feb 5, 2016 – Feb 12, 2016

Subsystems (2)

Kernel janitors Checkpatch

lore

Share & tools

lore

Kevin Wern

author

2016-02-05 08:29 UTC #0 lore raw

PATCH 1/1 checkpatch.pl: fix naked sscanf false positives

There is a section of checkpatch.pl that intends to ensure the return value of sscanf is always checked. However, in certain cases, like in drivers/staging/dgnc/dgnc.mod.c, the symbol for sscanf is used without calling the function:

static const struct modversion_info ____versions[] __used __attribute__((section("__versions"))) = {
...
{ 0x20c55ae0, __VMLINUX_SYMBOL_STR(sscanf) },
...
};

This currently results in a warning, which is undesirable. We should adjust the script's first regex condition to match *calls* to sscanf, not just the symbol itself.

Signed-off-by Kevin Wern <[email protected]>

---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

scripts/checkpatch.pl +1 -1

index 0147c91..199247d 100755
@@ -5452,7 +5452,7 @@ sub process {
 # check for naked sscanf
 		if ($^V && $^V ge 5.10.0 &&
 		    defined $stat &&
-		    $line =~ /\bsscanf\b/ &&
+		    $line =~ /\bsscanf\b\s*$balanced_parens/ &&
 		    ($stat !~ /$Ident\s*=\s*sscanf\s*$balanced_parens/ &&
 		     $stat !~ /\bsscanf\s*$balanced_parens\s*(?:$Compare)/ &&
 		     $stat !~ /(?:$Compare)\s*\bsscanf\s*$balanced_parens/)) {

Signature · 1 line

1.9.1

Dan Carpenter

2016-02-05 09:46 UTC #1 lore raw

Quoted 3 lines · Kevin Wern

There is a section of checkpatch.pl that intends to ensure the return value of sscanf is always checked. However, in certain cases, like in drivers/staging/dgnc/dgnc.mod.c

You shouldn't be running checkpatch.pl on a mod.c file. Those are just a build artifact and not code.

regards,
dan carpenter

Joe Perches

maintainer

2016-02-06 05:26 UTC #2 lore raw

In reply to Kevin Wern

Quoted 30 lines · Kevin Wern

There is a section of checkpatch.pl that intends to ensure the return value of sscanf is always checked. However, in certain cases, like in drivers/staging/dgnc/dgnc.mod.c, the symbol for sscanf is used without calling the function:
static const struct modversion_info ____versions[] __used __attribute__((section("__versions"))) = {
...
{ 0x20c55ae0, __VMLINUX_SYMBOL_STR(sscanf) },
...
};
This currently results in a warning, which is undesirable. We should adjust the script's first regex condition to match *calls* to sscanf, not just the symbol itself.
Signed-off-by Kevin Wern <[email protected]>
---
scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
scripts/checkpatch.pl +0 -0
index 0147c91..199247d 100755
@@ -5452,7 +5452,7 @@ sub process {
  # check for naked sscanf
if ($^V && $^V ge 5.10.0 &&
defined $stat &&

scripts/checkpatch.pl sub process { · line 5455

Quoted 2 lines

scripts/checkpatch.pl +1 -1

-		    $line =~ /\bsscanf\b/ &&
+		    $line =~ /\bsscanf\b\s*$balanced_parens/ &&

No, that won't work. That's what the $stat line is for

I suppose it could be /\bsscanf\s*\(/ though
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kevin Wern

author

2016-02-06 06:58 UTC #3 lore raw

In reply to Kevin Wern

Quoted 10 lines · Dan Carpenter

Quoted 3 lines · Kevin Wern
There is a section of checkpatch.pl that intends to ensure the return value of sscanf is always checked. However, in certain cases, like in drivers/staging/dgnc/dgnc.mod.c
You shouldn't be running checkpatch.pl on a mod.c file. Those are just a build artifact and not code.
regards,
dan carpenter

Whoops, guess I'm being a bit of a newb there. Regardless, isn't it still more precise to narrow this conditional down to function calls, rather than just the symbol itself?

I can eliminate the bad use case from the commit.

Quoted 36 lines · Joe Perches

Quoted 27 lines · Kevin Wern
There is a section of checkpatch.pl that intends to ensure the return value of sscanf is always checked. However, in certain cases, like in drivers/staging/dgnc/dgnc.mod.c, the symbol for sscanf is used without calling the function:
static const struct modversion_info ____versions[] __used __attribute__((section("__versions"))) = {
...
{ 0x20c55ae0, __VMLINUX_SYMBOL_STR(sscanf) },
...
};
This currently results in a warning, which is undesirable. We should adjust the script's first regex condition to match *calls* to sscanf, not just the symbol itself.
Signed-off-by Kevin Wern <[email protected]>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
scripts/checkpatch.pl +0 -0
index 0147c91..199247d 100755
scripts/checkpatch.pl sub process { · line 5455
Quoted 5 lines
scripts/checkpatch.pl +1 -1
@@ -5452,7 +5452,7 @@ sub process {
 # check for naked sscanf
              if ($^V && $^V ge 5.10.0 &&
                  defined $stat &&
-                 $line =~ /\bsscanf\b/ &&
+                 $line =~ /\bsscanf\b\s*$balanced_parens/ &&
No, that won't work. That's what the $stat line is for

Quoted 1 line

I suppose it could be /\bsscanf\s*\(/ though

I thought the $stat line's purpose was to ensure a statement was being evaluated in the current context. I tested the change with the example statement, and it both eliminated the original example and retained the ability to recognize unchecked sscanf return values. Although the example statement is from an artifact that shouldn't be checked by the script, it is still a valid example.

I don't see why using unbalanced vs balanced parens would work better, although I believe both would work. The next statements that apply comparative contexts all use $balanced_parens for the argument list, so I figured it was consistent.

Let me know what you think. I'm willing to use a different regex, but I just would like to understand the reasoning.

-Kevin Wern

Joe Perches

maintainer

2016-02-06 17:20 UTC #4 lore raw

In reply to Kevin Wern

(Robo Bot?, I think he prefers Andy)

Quoted 55 lines · Kevin Wern

Quoted 10 lines · Dan Carpenter
Quoted 3 lines · Kevin Wern
There is a section of checkpatch.pl that intends to ensure the return value of sscanf is always checked. However, in certain cases, like in drivers/staging/dgnc/dgnc.mod.c
You shouldn't be running checkpatch.pl on a mod.c file. Those are just a build artifact and not code.
regards,
dan carpenter
Whoops, guess I'm being a bit of a newb there. Regardless, isn't it still more precise to narrow this conditional down to function calls, rather than just the symbol itself?
I can eliminate the bad use case from the commit.
Quoted 36 lines · Joe Perches
Quoted 28 lines · Kevin Wern
There is a section of checkpatch.pl that intends to ensure the return value of sscanf is always checked. However, in certain cases, like in drivers/staging/dgnc/dgnc.mod.c, the symbol for sscanf is used without calling the function:
static const struct modversion_info ____versions[] __used __attribute__((section("__versions"))) = {
...
{ 0x20c55ae0, __VMLINUX_SYMBOL_STR(sscanf) },
...
};
This currently results in a warning, which is undesirable. We should adjust the script's first regex condition to match *calls* to sscanf, not just the symbol itself.
Signed-off-by Kevin Wern <[email protected]>
---
scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
scripts/checkpatch.pl +0 -0
index 0147c91..199247d 100755
@@ -5452,7 +5452,7 @@ sub process {
  # check for naked sscanf
Quoted 4 lines
              if ($^V && $^V ge 5.10.0 &&
                  defined $stat &&
-                 $line =~ /\bsscanf\b/ &&
+                 $line =~ /\bsscanf\b\s*$balanced_parens/ &&
No, that won't work.  That's what the $stat line is for

Quoted 8 lines

Quoted 1 line
I suppose it could be /\bsscanf\s*\(/ though
I thought the $stat line's purpose was to ensure a statement was being evaluated in the current context. I tested the change with the example statement, and it both eliminated the original example and retained the ability to recognize unchecked sscanf return values. Although the example statement is from an artifact that shouldn't be checked by the script, it is still a valid example.

It's a single line sscanf vs multi-line sscanf issue

$line works on

	ret = sscanf(buf, &foo, &bar);

$stat works on that and

	ret = sscanf(buf,
		     &foo,
		     &bar);

Quoted 1 line

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kevin Wern

author

2016-02-06 21:24 UTC #5 lore raw

In reply to Kevin Wern

Sorry, I think the Andy thing was the gmail client auto-filling contacts when I reply all...

Quoted 11 lines · Joe Perches

It's a single line sscanf vs multi-line sscanf issue
$line works on
        ret = sscanf(buf, &foo, &bar);
$stat works on that and
        ret = sscanf(buf,
                     &foo,
                     &bar);

Ah, ok. I dont think having just the ending paren will work though because the paren can be on the next line, so the pre-patch statement is most correct. What we should do, then, is something like:

        [snip]
        $line =~ /\bsscanf\b/ &&
-       ($stat !~ /$Ident\s*=\s*sscanf\s*$balanced_parens/ &&
+       ($stat =~ /\bsscanf\b\s*$balanced_parens/ &&
+       $stat !~ /$Ident\s*=\s*sscanf\s*$balanced_parens/ &&
        $stat !~ /\bsscanf\s*$balanced_parens\s*(?:$Compare)/ &&
        $stat !~ /(?:$Compare)\s*\bsscanf\s*$balanced_parens/)) {
        [snip]

That way, we know we are evaluating a sscanf on the current line, but also ensure that it is a function call in the current statement.

Would this work? It seems to on my end.

I can send over the amended patch.

- Kevin

Joe Perches

maintainer

2016-02-06 23:08 UTC #6 lore raw

In reply to Kevin Wern

Quoted 13 lines · Kevin Wern

Quoted 11 lines · Joe Perches
It's a single line sscanf vs multi-line sscanf issue
$line works on
        ret = sscanf(buf, &foo, &bar);
$stat works on that and
        ret = sscanf(buf,
                     &foo,
                     &bar);

Quoted 5 lines

Ah, ok. I dont think having just the ending paren will work though because the
paren can be on the next line, so the pre-patch statement is most correct.
What we should do, then, is something like:

Not really.

That'd only find an additional

	sscanf
		(buf,
		 foo,
		 bar);

And that's quite unlikely.

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kevin Wern

author

2016-02-12 05:35 UTC #7 lore raw

In reply to Kevin Wern

Quoted 10 lines

Not really.
That'd only find an additional
        sscanf
                (buf,
                 foo,
                 bar);
And that's quite unlikely.

I fail to see the issue if it matches everything matched by /\bsscanf\b\s*(/, plus one additional case that is unlikey, but still valid.

I can do a patch the $line match you described, but I'd prefer to do what I previously described.

Thanks again for your feedback.

Kevin