[Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Ujjwal Kumar]

checkpatch script 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 without
'.pl' extension in its filename and not belonging to 'scripts/'
directory is reported as ERROR which is a false-positive.

The script can correctly handle patches with mode changes and
shebang line if shebang is taken into account. So, along with
the current check conditions, adding the shebang check in the
check conditions can improve the reports of the script.

Signed-off-by: Ujjwal Kumar <ujjwalkumar0501@gmail.com>
---
 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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Ujjwal Kumar]

On 11/10/20 9:51 pm, Ujjwal Kumar wrote:
> checkpatch script 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 without
> '.pl' extension in its filename and not belonging to 'scripts/'
> directory is reported as ERROR which is a false-positive.
> 
> The script can correctly handle patches with mode changes and
> shebang line if shebang is taken into account. So, along with
> the current check conditions, adding the shebang check in the
> check conditions can improve the reports of the script.
> 
> Signed-off-by: Ujjwal Kumar <ujjwalkumar0501@gmail.com>
> ---
>  scripts/checkpatch.pl | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

To test the changes, I used the following scripts:

for i in $(find -executable -type f | grep "\.[a-z]" -v | grep "scripts/" -v); \
do \
hs=$(git log --reverse --format=%H $i | head -1); \
echo $hs; \
done | sort | uniq | \
xargs -n1 -P7 scripts/checkpatch.pl --show-types --types EXECUTE_PERMISSIONS --terse -g

Before patching, the script shows 21 false-positives.
After applying the patch, the ERRORS are gone.

Thanks
Ujjwal Kumar
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Lukas Bulwahn]

On Sun, 11 Oct 2020, Ujjwal Kumar wrote:

> checkpatch script 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 without
> '.pl' extension in its filename and not belonging to 'scripts/'
> directory is reported as ERROR which is a false-positive.
> 
> The script can correctly handle patches with mode changes and
> shebang line if shebang is taken into account. So, along with
> the current check conditions, adding the shebang check in the
> check conditions can improve the reports of the script.
>

I think one of the core design decisions of checkpatch.pl is:

checkpatch.pl can run on a patch, even if the patch does not apply to the 
current repository version that is checked out.

It solely uses the information in the patch, and does not try to guess how 
it could be applied etc.

This patch violates that core design decisions.

You can propose to Joe Perches and lkml, but do not be surprised if that 
is rejected because of this reason above.

I would be interested in the discussion.

Lukas

 
> Signed-off-by: Ujjwal Kumar <ujjwalkumar0501@gmail.com>
> ---
>  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
> 
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Ujjwal Kumar]

On 11/10/20 11:20 pm, Lukas Bulwahn wrote:
> 
> 
> On Sun, 11 Oct 2020, Ujjwal Kumar wrote:
> 
>> checkpatch script 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 without
>> '.pl' extension in its filename and not belonging to 'scripts/'
>> directory is reported as ERROR which is a false-positive.
>>
>> The script can correctly handle patches with mode changes and
>> shebang line if shebang is taken into account. So, along with
>> the current check conditions, adding the shebang check in the
>> check conditions can improve the reports of the script.
>>
> 
> I think one of the core design decisions of checkpatch.pl is:
> 
> checkpatch.pl can run on a patch, even if the patch does not apply to the 
> current repository version that is checked out.

From our past conversation I remember about this particular point.

> 
> It solely uses the information in the patch, and does not try to guess how 
> it could be applied etc.

I am fetching the 'shebang' from the patch itself (therefore I do not 
understand how does the proposed change violate that design decision?).

> 
> This patch violates that core design decisions.

Can you please point out the exact change that violates the design decisions?

> 
> You can propose to Joe Perches and lkml, but do not be surprised if that 
> is rejected because of this reason above.
> 
> I would be interested in the discussion.
> 
> Lukas
> 
>  

Thanks
Ujjwal Kumar
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Lukas Bulwahn]

On Sun, 11 Oct 2020, Ujjwal Kumar wrote:

> On 11/10/20 11:20 pm, Lukas Bulwahn wrote:
> > 
> > 
> > On Sun, 11 Oct 2020, Ujjwal Kumar wrote:
> > 
> >> checkpatch script 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 without
> >> '.pl' extension in its filename and not belonging to 'scripts/'
> >> directory is reported as ERROR which is a false-positive.
> >>
> >> The script can correctly handle patches with mode changes and
> >> shebang line if shebang is taken into account. So, along with
> >> the current check conditions, adding the shebang check in the
> >> check conditions can improve the reports of the script.
> >>
> > 
> > I think one of the core design decisions of checkpatch.pl is:
> > 
> > checkpatch.pl can run on a patch, even if the patch does not apply to the 
> > current repository version that is checked out.
> 
> From our past conversation I remember about this particular point.
> 
> > 
> > It solely uses the information in the patch, and does not try to guess how 
> > it could be applied etc.
> 
> I am fetching the 'shebang' from the patch itself (therefore I do not 
> understand how does the proposed change violate that design decision?).
>

Okay, maybe I misread the patch; so, where those the first line come from?
What if that first line is not part of the patch?

> > 
> > This patch violates that core design decisions.
> 
> Can you please point out the exact change that violates the design decisions?
> 
> > 
> > You can propose to Joe Perches and lkml, but do not be surprised if that 
> > is rejected because of this reason above.
> > 
> > I would be interested in the discussion.
> > 
> > Lukas
> > 
> >  
> 
> Thanks
> Ujjwal Kumar
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Ujjwal Kumar]

On 11/10/20 11:49 pm, Lukas Bulwahn wrote:
> 
> 
> On Sun, 11 Oct 2020, Ujjwal Kumar wrote:
> 
>> On 11/10/20 11:20 pm, Lukas Bulwahn wrote:
>>>
>>>
>>> On Sun, 11 Oct 2020, Ujjwal Kumar wrote:
>>>
>>>> checkpatch script 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 without
>>>> '.pl' extension in its filename and not belonging to 'scripts/'
>>>> directory is reported as ERROR which is a false-positive.
>>>>
>>>> The script can correctly handle patches with mode changes and
>>>> shebang line if shebang is taken into account. So, along with
>>>> the current check conditions, adding the shebang check in the
>>>> check conditions can improve the reports of the script.
>>>>
>>>
>>> I think one of the core design decisions of checkpatch.pl is:
>>>
>>> checkpatch.pl can run on a patch, even if the patch does not apply to the 
>>> current repository version that is checked out.
>>
>> From our past conversation I remember about this particular point.
>>
>>>
>>> It solely uses the information in the patch, and does not try to guess how 
>>> it could be applied etc.
>>
>> I am fetching the 'shebang' from the patch itself (therefore I do not 
>> understand how does the proposed change violate that design decision?).
>>
> 
> Okay, maybe I misread the patch; so, where those the first line come from?
> What if that first line is not part of the patch?

In that case there might be a false-positive. But I tried to handle such cases 
where the first line is in the patch itself.

Okay, so there are total of 4 cases with file mode change:
1. mode change + create + modify
2. mode change + create
3. mode change + modify
4. mode change

the patch will handle all cases of type 1 and some cases of type 3.
Other cases won't be handled because they cannot be handled without 
breaking the core design decision.

I do have references to say that the cases 2 and 4 are

Which I thought is an improvement over current logic used for the check.

Correct me if I am wrong.


Thanks
Ujjwal Kumar
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Lukas Bulwahn]

On Sun, Oct 11, 2020 at 8:32 PM Ujjwal Kumar <ujjwalkumar0501@gmail.com> wrote:
>
> On 11/10/20 11:49 pm, Lukas Bulwahn wrote:
> >
> >
> > On Sun, 11 Oct 2020, Ujjwal Kumar wrote:
> >
> >> On 11/10/20 11:20 pm, Lukas Bulwahn wrote:
> >>>
> >>>
> >>> On Sun, 11 Oct 2020, Ujjwal Kumar wrote:
> >>>
> >>>> checkpatch script 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 without
> >>>> '.pl' extension in its filename and not belonging to 'scripts/'
> >>>> directory is reported as ERROR which is a false-positive.
> >>>>
> >>>> The script can correctly handle patches with mode changes and
> >>>> shebang line if shebang is taken into account. So, along with
> >>>> the current check conditions, adding the shebang check in the
> >>>> check conditions can improve the reports of the script.
> >>>>
> >>>
> >>> I think one of the core design decisions of checkpatch.pl is:
> >>>
> >>> checkpatch.pl can run on a patch, even if the patch does not apply to the
> >>> current repository version that is checked out.
> >>
> >> From our past conversation I remember about this particular point.
> >>
> >>>
> >>> It solely uses the information in the patch, and does not try to guess how
> >>> it could be applied etc.
> >>
> >> I am fetching the 'shebang' from the patch itself (therefore I do not
> >> understand how does the proposed change violate that design decision?).
> >>
> >
> > Okay, maybe I misread the patch; so, where those the first line come from?
> > What if that first line is not part of the patch?
>
> In that case there might be a false-positive. But I tried to handle such cases
> where the first line is in the patch itself.
>
> Okay, so there are total of 4 cases with file mode change:
> 1. mode change + create + modify
> 2. mode change + create
> 3. mode change + modify
> 4. mode change
>
> the patch will handle all cases of type 1 and some cases of type 3.
> Other cases won't be handled because they cannot be handled without
> breaking the core design decision.
>
> I do have references to say that the cases 2 and 4 are
>
> Which I thought is an improvement over current logic used for the check.
>

Sounds good, maybe you can explain that in the commit message and then
send that to the mailing list and Joe Perches.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[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>
---
 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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Lukas Bulwahn]

Please add linux-kernel@vger.kernel.org (lkml) to the recipient list.

On Mon, Oct 12, 2020 at 6:37 AM Ujjwal Kumar <ujjwalkumar0501@gmail.com> 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.
>

s/false-positive/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>
> ---
>  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
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Joe Perches]

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



_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Ujjwal Kumar]

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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Lukas Bulwahn]

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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Joe Perches]

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.


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Joe Perches]

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.


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees