Re: [Linux-kernel-mentees] [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS
From: Joe Perches <joe@perches.com>
Date: 2020-10-12 06:17:27
Also in:
lkml
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.
quoted hunk ↗ jump to hunk
Signed-off-by: Ujjwal Kumar <redacted> --- 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
[]
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
+ 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