* [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS
@ 2020-10-12 5:49 Ujjwal Kumar
2020-10-12 6:17 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Ujjwal Kumar @ 2020-10-12 5:49 UTC (permalink / raw)
To: Lukas Bulwahn, Joe Perches
Cc: linux-kernel-mentees, linux-kernel, Ujjwal Kumar
checkpatch.pl checks for invalid EXECUTE_PERMISSIONS on source
files. The script leverages filename extensions and its path in
the repository to decide whether to allow execute permissions on
the file or not.
Based on current check conditions, a perl script file having
execute permissions, without '.pl' extension in its filename
and not belonging to 'scripts/' directory is reported as ERROR
which is a false-positive.
Adding a shebang check along with current conditions will make
the check more generalised and improve checkpatch reports.
To do so, without breaking the core design decision of checkpatch,
we can fetch the first line from the patch itself and match it for
a shebang pattern.
There can be cases where the first line is not part of the patch.
In that case there may be a false-positive report but in the end we
will have less false-positives as we will be handling some of the
unhandled cases.
Signed-off-by: Ujjwal Kumar <ujjwalkumar0501@gmail.com>
---
Apologies, I forgot to include linux-kernel@vger.kernel.org so I'm
now resending.
scripts/checkpatch.pl | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fab38b493cef..e596d30794bf 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1795,6 +1795,23 @@ sub get_stat_here {
return $herectx;
}
+sub get_shebang {
+ my ($linenr, $realfile) = @_;
+ my $rawline = "";
+ my $shebang = "";
+
+ $rawline = raw_line($linenr, 3);
+ if (defined $rawline &&
+ $rawline =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
+ if (defined $1 && $1 == 1) {
+ $shebang = raw_line($linenr, 4);
+ $shebang = substr $shebang, 1;
+ }
+ }
+
+ return $shebang;
+}
+
sub cat_vet {
my ($vet) = @_;
my ($res, $coded);
@@ -2680,7 +2697,9 @@ sub process {
# Check for incorrect file permissions
if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) {
my $permhere = $here . "FILE: $realfile\n";
+ my $shebang = get_shebang($linenr, $realfile);
if ($realfile !~ m@scripts/@ &&
+ $shebang !~ /^#!\s*(\/\w)+.*/ &&
$realfile !~ /\.(py|pl|awk|sh)$/) {
ERROR("EXECUTE_PERMISSIONS",
"do not set execute permissions for source files\n" . $permhere);
base-commit: d67bc7812221606e1886620a357b13f906814af7
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS
2020-10-12 5:49 [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS Ujjwal Kumar
@ 2020-10-12 6:17 ` Joe Perches
2020-10-12 13:52 ` Ujjwal Kumar
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2020-10-12 6:17 UTC (permalink / raw)
To: Ujjwal Kumar, Lukas Bulwahn; +Cc: linux-kernel-mentees, linux-kernel
On Mon, 2020-10-12 at 11:19 +0530, Ujjwal Kumar wrote:
> checkpatch.pl checks for invalid EXECUTE_PERMISSIONS on source
> files. The script leverages filename extensions and its path in
> the repository to decide whether to allow execute permissions on
> the file or not.
>
> Based on current check conditions, a perl script file having
> execute permissions, without '.pl' extension in its filename
> and not belonging to 'scripts/' directory is reported as ERROR
> which is a false-positive.
>
> Adding a shebang check along with current conditions will make
> the check more generalised and improve checkpatch reports.
> To do so, without breaking the core design decision of checkpatch,
> we can fetch the first line from the patch itself and match it for
> a shebang pattern.
>
> There can be cases where the first line is not part of the patch.
For instance: a patch that only changes permissions
without changing any of the file content.
>
> In that case there may be a false-positive report but in the end we
> will have less false-positives as we will be handling some of the
> unhandled cases.
> Signed-off-by: Ujjwal Kumar <ujjwalkumar0501@gmail.com>
> ---
> Apologies, I forgot to include linux-kernel@vger.kernel.org so I'm
> now resending.
>
> scripts/checkpatch.pl | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -1795,6 +1795,23 @@ sub get_stat_here {
> return $herectx;
> }
First some style trivia:
> +sub get_shebang {
> + my ($linenr, $realfile) = @_;
> + my $rawline = "";
> + my $shebang = "";
> +
> + $rawline = raw_line($linenr, 3);
> + if (defined $rawline &&
> + $rawline =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
alignment to open parenthesis please
> + if (defined $1 && $1 == 1) {
> + $shebang = raw_line($linenr, 4);
> + $shebang = substr $shebang, 1;
parentheses around substr please.
> + }
> + }
> +
> + return $shebang;
> +}
And some real notes:
$realfile isn't used in this function so there doesn't
seem to be a reason to have it as an function argument.
> +
> sub cat_vet {
> my ($vet) = @_;
> my ($res, $coded);
> @@ -2680,7 +2697,9 @@ sub process {
> # Check for incorrect file permissions
> if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) {
probably better here to use a capture group for the permissions
if ($line =~ /^new (?:file )?mode (\d+)$/) {
my $mode = substr($1, -3);
> my $permhere = $here . "FILE: $realfile\n";
> + my $shebang = get_shebang($linenr, $realfile);
> if ($realfile !~ m@scripts/@ &&
Maybe remove the $realfile directory test as
there are many source files that are not scripts
in this directory and its subdirectories.
> + $shebang !~ /^#!\s*(\/\w)+.*/ &&
unnecessary capture group
and add
$mode =~ /[1357]/ &&
> $realfile !~ /\.(py|pl|awk|sh)$/) {
No need for a a capture group here either. (existing defect)
> ERROR("EXECUTE_PERMISSIONS",
> "do not set execute permissions for source files\n" . $permhere);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS
2020-10-12 6:17 ` Joe Perches
@ 2020-10-12 13:52 ` Ujjwal Kumar
2020-10-12 14:16 ` Lukas Bulwahn
2020-10-12 15:08 ` Joe Perches
0 siblings, 2 replies; 6+ messages in thread
From: Ujjwal Kumar @ 2020-10-12 13:52 UTC (permalink / raw)
To: Joe Perches, Lukas Bulwahn; +Cc: linux-kernel-mentees, linux-kernel
On 12/10/20 11:47 am, Joe Perches wrote:
> On Mon, 2020-10-12 at 11:19 +0530, Ujjwal Kumar wrote:
>> checkpatch.pl checks for invalid EXECUTE_PERMISSIONS on source
>> files. The script leverages filename extensions and its path in
>> the repository to decide whether to allow execute permissions on
>> the file or not.
>>
>> Based on current check conditions, a perl script file having
>> execute permissions, without '.pl' extension in its filename
>> and not belonging to 'scripts/' directory is reported as ERROR
>> which is a false-positive.
>>
>> Adding a shebang check along with current conditions will make
>> the check more generalised and improve checkpatch reports.
>> To do so, without breaking the core design decision of checkpatch,
>> we can fetch the first line from the patch itself and match it for
>> a shebang pattern.
>>
>> There can be cases where the first line is not part of the patch.
>
> For instance: a patch that only changes permissions
> without changing any of the file content.
>
>>
>> In that case there may be a false-positive report but in the end we
>> will have less false-positives as we will be handling some of the
>> unhandled cases.
>
>> Signed-off-by: Ujjwal Kumar <ujjwalkumar0501@gmail.com>
>> ---
>> Apologies, I forgot to include linux-kernel@vger.kernel.org so I'm
>> now resending.
>>
>> scripts/checkpatch.pl | 19 +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -1795,6 +1795,23 @@ sub get_stat_here {
>> return $herectx;
>> }
>
> First some style trivia:
>
>> +sub get_shebang {
>> + my ($linenr, $realfile) = @_;
>> + my $rawline = "";
>> + my $shebang = "";
>> +
>> + $rawline = raw_line($linenr, 3);
>> + if (defined $rawline &&
>> + $rawline =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
>
> alignment to open parenthesis please
>
>> + if (defined $1 && $1 == 1) {
>> + $shebang = raw_line($linenr, 4);
>> + $shebang = substr $shebang, 1;
>
> parentheses around substr please.
>
>> + }
>> + }
>> +
>> + return $shebang;
>> +}
>
> And some real notes:
>
> $realfile isn't used in this function so there doesn't
> seem to be a reason to have it as an function argument.
>
>> +
>> sub cat_vet {
>> my ($vet) = @_;
>> my ($res, $coded);
>> @@ -2680,7 +2697,9 @@ sub process {
>> # Check for incorrect file permissions
>> if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) {
>
> probably better here to use a capture group for the permissions
>
> if ($line =~ /^new (?:file )?mode (\d+)$/) {
> my $mode = substr($1, -3);
This
>
>> my $permhere = $here . "FILE: $realfile\n";
>> + my $shebang = get_shebang($linenr, $realfile);
>> if ($realfile !~ m@scripts/@ &&
>
> Maybe remove the $realfile directory test as
> there are many source files that are not scripts
> in this directory and its subdirectories.
this
>
>> + $shebang !~ /^#!\s*(\/\w)+.*/ &&
>
> unnecessary capture group
>
> and add
>
> $mode =~ /[1357]/ &&
this
>
>> $realfile !~ /\.(py|pl|awk|sh)$/) {
>
> No need for a a capture group here either. (existing defect)
and this.
>
>> ERROR("EXECUTE_PERMISSIONS",
>> "do not set execute permissions for source files\n" . $permhere);
>
>
>
Should these new changes go as a separate patch or can they be
included in the next iteration of this patch?
Thanks
Ujjwal Kumar
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS
2020-10-12 13:52 ` Ujjwal Kumar
@ 2020-10-12 14:16 ` Lukas Bulwahn
2020-10-12 15:23 ` Joe Perches
2020-10-12 15:08 ` Joe Perches
1 sibling, 1 reply; 6+ messages in thread
From: Lukas Bulwahn @ 2020-10-12 14:16 UTC (permalink / raw)
To: Ujjwal Kumar
Cc: Joe Perches, Lukas Bulwahn, linux-kernel-mentees, linux-kernel
On Mon, 12 Oct 2020, Ujjwal Kumar wrote:
> On 12/10/20 11:47 am, Joe Perches wrote:
> > On Mon, 2020-10-12 at 11:19 +0530, Ujjwal Kumar wrote:
> >> checkpatch.pl checks for invalid EXECUTE_PERMISSIONS on source
> >> files. The script leverages filename extensions and its path in
> >> the repository to decide whether to allow execute permissions on
> >> the file or not.
> >>
> >> Based on current check conditions, a perl script file having
> >> execute permissions, without '.pl' extension in its filename
> >> and not belonging to 'scripts/' directory is reported as ERROR
> >> which is a false-positive.
> >>
> >> Adding a shebang check along with current conditions will make
> >> the check more generalised and improve checkpatch reports.
> >> To do so, without breaking the core design decision of checkpatch,
> >> we can fetch the first line from the patch itself and match it for
> >> a shebang pattern.
> >>
> >> There can be cases where the first line is not part of the patch.
> >
> > For instance: a patch that only changes permissions
> > without changing any of the file content.
> >
> >>
> >> In that case there may be a false-positive report but in the end we
> >> will have less false-positives as we will be handling some of the
> >> unhandled cases.
> >
> >> Signed-off-by: Ujjwal Kumar <ujjwalkumar0501@gmail.com>
> >> ---
> >> Apologies, I forgot to include linux-kernel@vger.kernel.org so I'm
> >> now resending.
> >>
> >> scripts/checkpatch.pl | 19 +++++++++++++++++++
> >> 1 file changed, 19 insertions(+)
> >>
> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> >> @@ -1795,6 +1795,23 @@ sub get_stat_here {
> >> return $herectx;
> >> }
> >
> > First some style trivia:
> >
> >> +sub get_shebang {
> >> + my ($linenr, $realfile) = @_;
> >> + my $rawline = "";
> >> + my $shebang = "";
> >> +
> >> + $rawline = raw_line($linenr, 3);
> >> + if (defined $rawline &&
> >> + $rawline =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
> >
> > alignment to open parenthesis please
> >
> >> + if (defined $1 && $1 == 1) {
> >> + $shebang = raw_line($linenr, 4);
> >> + $shebang = substr $shebang, 1;
> >
> > parentheses around substr please.
> >
> >> + }
> >> + }
> >> +
> >> + return $shebang;
> >> +}
> >
> > And some real notes:
> >
> > $realfile isn't used in this function so there doesn't
> > seem to be a reason to have it as an function argument.
> >
> >> +
> >> sub cat_vet {
> >> my ($vet) = @_;
> >> my ($res, $coded);
> >> @@ -2680,7 +2697,9 @@ sub process {
> >> # Check for incorrect file permissions
> >> if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) {
> >
> > probably better here to use a capture group for the permissions
> >
> > if ($line =~ /^new (?:file )?mode (\d+)$/) {
> > my $mode = substr($1, -3);
>
> This
>
> >
> >> my $permhere = $here . "FILE: $realfile\n";
> >> + my $shebang = get_shebang($linenr, $realfile);
> >> if ($realfile !~ m@scripts/@ &&
> >
> > Maybe remove the $realfile directory test as
> > there are many source files that are not scripts
> > in this directory and its subdirectories.
>
> this
>
> >
> >> + $shebang !~ /^#!\s*(\/\w)+.*/ &&
> >
> > unnecessary capture group
> >
> > and add
> >
> > $mode =~ /[1357]/ &&
>
> this
>
> >
> >> $realfile !~ /\.(py|pl|awk|sh)$/) {
> >
> > No need for a a capture group here either. (existing defect)
>
> and this.
>
> >
> >> ERROR("EXECUTE_PERMISSIONS",
> >> "do not set execute permissions for source files\n" . $permhere);
> >
> >
> >
>
> Should these new changes go as a separate patch or can they be
> included in the next iteration of this patch?
>
>
Ujjwal, please consider the following 'strategy':
- Send one patch to clean up the existing implementation as Joe requested.
With those 'credit points' for cleaning up the implementation, you then:
- Send another clean patch for the additional functionality you propose
We can probably easily accept the first cleanup, and then dig into the
review of the additional functionality.
Lukas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS
2020-10-12 13:52 ` Ujjwal Kumar
2020-10-12 14:16 ` Lukas Bulwahn
@ 2020-10-12 15:08 ` Joe Perches
1 sibling, 0 replies; 6+ messages in thread
From: Joe Perches @ 2020-10-12 15:08 UTC (permalink / raw)
To: Ujjwal Kumar, Lukas Bulwahn; +Cc: linux-kernel-mentees, linux-kernel
On Mon, 2020-10-12 at 19:22 +0530, Ujjwal Kumar wrote:
> On 12/10/20 11:47 am, Joe Perches wrote:
> > On Mon, 2020-10-12 at 11:19 +0530, Ujjwal Kumar wrote:
> > > checkpatch.pl checks for invalid EXECUTE_PERMISSIONS on source
> > > files. The script leverages filename extensions and its path in
> > > the repository to decide whether to allow execute permissions on
> > > the file or not.
> > >
> > > Based on current check conditions, a perl script file having
> > > execute permissions, without '.pl' extension in its filename
> > > and not belonging to 'scripts/' directory is reported as ERROR
> > > which is a false-positive.
> > >
> > > Adding a shebang check along with current conditions will make
> > > the check more generalised and improve checkpatch reports.
> > > To do so, without breaking the core design decision of checkpatch,
> > > we can fetch the first line from the patch itself and match it for
> > > a shebang pattern.
> > >
> > > There can be cases where the first line is not part of the patch.
> >
> > For instance: a patch that only changes permissions
> > without changing any of the file content.
Please add verbiage like this to the commit message.
> Should these new changes go as a separate patch or can they be
> included in the next iteration of this patch?
V2 please.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS
2020-10-12 14:16 ` Lukas Bulwahn
@ 2020-10-12 15:23 ` Joe Perches
0 siblings, 0 replies; 6+ messages in thread
From: Joe Perches @ 2020-10-12 15:23 UTC (permalink / raw)
To: Lukas Bulwahn, Ujjwal Kumar; +Cc: linux-kernel-mentees, linux-kernel
On Mon, 2020-10-12 at 16:16 +0200, Lukas Bulwahn wrote:
> On Mon, 12 Oct 2020, Ujjwal Kumar wrote:
> > On 12/10/20 11:47 am, Joe Perches wrote:
> > > On Mon, 2020-10-12 at 11:19 +0530, Ujjwal Kumar wrote:
> > > > checkpatch.pl checks for invalid EXECUTE_PERMISSIONS on source
> > > > files. The script leverages filename extensions and its path in
> > > > the repository to decide whether to allow execute permissions on
> > > > the file or not.
> > > >
> > > > Based on current check conditions, a perl script file having
> > > > execute permissions, without '.pl' extension in its filename
> > > > and not belonging to 'scripts/' directory is reported as ERROR
> > > > which is a false-positive.
> > > >
> > > > Adding a shebang check along with current conditions will make
> > > > the check more generalised and improve checkpatch reports.
> > > > To do so, without breaking the core design decision of checkpatch,
> > > > we can fetch the first line from the patch itself and match it for
> > > > a shebang pattern.
> > > >
> > > > There can be cases where the first line is not part of the patch.
> > >
> > > For instance: a patch that only changes permissions
> > > without changing any of the file content.
[]
> > Should these new changes go as a separate patch or can they be
> > included in the next iteration of this patch?
[]
The commit log should be updated with the example shown.
Please send a clean V2.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-12 15:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-12 5:49 [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS Ujjwal Kumar
2020-10-12 6:17 ` Joe Perches
2020-10-12 13:52 ` Ujjwal Kumar
2020-10-12 14:16 ` Lukas Bulwahn
2020-10-12 15:23 ` Joe Perches
2020-10-12 15:08 ` Joe Perches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).