Subject: checkbashisms: multiple false positives and false negatives

From: Raphael Geissert <[email-masked]>
Date: Wed, 27 May 2009 19:11:24 -0500

Package: devscripts
Version: 2.10.49
User: [email-masked]
Usertags: checkbashisms

Hi,

These are some of my findings based on the recent checkbashisms run:
(NOTE: a combined diff is attached)

-----------------
FP:
> possible bashism in
> ./usr/share/pyshared/support-files/setuptools-0.6c9-py2.4.egg line 202

Fix (allow exec to be preceded by 'then'):
@@ -408,7 +409,7 @@ sub script_is_evil_and_wrong {
         last if (++$i > 55);
         if (m~
            # the exec should either be "eval"ed or a new statement
-           (^\s*|\beval\s*[\'\"]|(;|&&)\s*)
+           (^\s*|\beval\s*[\'\"]|(;|&&|\bthen)\s*)

            # eat anything between the exec and $0
            exec\s*.+\s*

-----------------
FP:
> possible bashism in ./usr/share/shorewall6-lite/lib.base line 684 (sourced
> script with arguments):
>     . $(find_file $(expand $@))

Workaround (this needs to be fixed by stripping evals, $(), ``, and any other 
form of code execution and looking for bashisms in those parts individually):
Apply the same dummy logic used for "" and '' to $()

@@ -281,8 +282,8 @@ foreach my $filename (@ARGV) {
            # detect source (.) trying to pass args to the command it runs
            # The first expression weeds out '. "foo bar"'
            if (not $found and
-               not m/^\s*\.\s+(\"[^\"]+\"|\'[^\']+\')\s*(\&|\||\d?>|<|;|\Z)/
-               and m/^\s*(\.\s+[^\s;\`:]+\s+([^\s;]+))/) {
+               not m/^\s*\.\s+(\"[^\"]+\"|\'[^\']+\'|\$\([^)]+\)+)\s*(\&|\||
\d?>|<|;|\Z)/
+               and m/\s*(\.\s+[^\s;\`:]+\s+([^\s;]+))/) {
                if ($2 =~ /^(\&|\||\d?>|<)/) {
                    # everything is ok
                    ;

-----------------
FN:

> if something; then . foo bar; else bar; fi

Fix (+ move LEADIN to the global scope):
@@ -281,8 +282,8 @@ foreach my $filename (@ARGV) {
            # detect source (.) trying to pass args to the command it runs
            # The first expression weeds out '. "foo bar"'
            if (not $found and
-               not m/^\s*\.\s+(\"[^\"]+\"|\'[^\']+\')\s*(\&|\||\d?>|<|;|\Z)/
-               and m/^\s*(\.\s+[^\s;\`:]+\s+([^\s;]+))/) {
+               not m/$LEADIN\.\s+(\"[^\"]+\"|\'[^\']+\'|\$\([^)]+\)+)\s*(\&|
\||\d?>|<|;|\Z)/
+               and m/$LEADIN(\.\s+[^\s;\`:]+\s+([^\s;]+))/) {
                if ($2 =~ /^(\&|\||\d?>|<)/) {
                    # everything is ok
                    ;

-----------------

Still to be fixed:
> . $(foo $(bar) moo)

-----------------

FP (new kind of wrapper):

usr/share/doc/systemtap-doc/examples/process/errsnoop.stp: 
> #!/bin/sh
> //usr/bin/env stap -DMAXMAPENTRIES=20480 $0 $@; exit $?
> # errsnoop.stp
> ...

-----------------

FP (ref: #530084):
> if false; then foo; else exec something; fi

@@ -408,7 +409,7 @@ sub script_is_evil_and_wrong {
         last if (++$i > 55);
         if (m~
            # the exec should either be "eval"ed or a new statement
-           (^\s*|\beval\s*[\'\"]|(;|&&)\s*)
+           (^\s*|\beval\s*[\'\"]|(;|&&|\b(then|else))\s*)

            # eat anything between the exec and $0
            exec\s*.+\s*

-----------------

FN:

> #!/bin/sh
> cat <<FOO
> hello
> $(echo -e "world\c")
> Running on $OSTYPE
> FOO

Only here docs with quoted markers should be ignored.

Cheers,
-- 
Raphael Geissert - Debian Maintainer
www.debian.org - get.debian.net
diff --git a/checkbashisms.orig b/checkbashisms
index 40cffc9..d97e6b5 100755
--- a/checkbashisms.orig
+++ b/checkbashisms
@@ -74,6 +74,7 @@ my $status = 0;
 my $makefile = 0;
 my (%bashisms, %string_bashisms, %singlequote_bashisms);
 
+our $LEADIN = qr'(?:(?:^|[`&;(|{])\s*|(?:if|then|do|while|shell)\s+)';
 init_hashes;
 
 foreach my $filename (@ARGV) {
@@ -281,8 +282,8 @@ foreach my $filename (@ARGV) {
 	    # detect source (.) trying to pass args to the command it runs
 	    # The first expression weeds out '. "foo bar"'
 	    if (not $found and
-		not m/^\s*\.\s+(\"[^\"]+\"|\'[^\']+\')\s*(\&|\||\d?>|<|;|\Z)/
-		and m/^\s*(\.\s+[^\s;\`:]+\s+([^\s;]+))/) {
+		not m/$LEADIN\.\s+(\"[^\"]+\"|\'[^\']+\'|\$\([^)]+\)+)\s*(\&|\||\d?>|<|;|\Z)/
+		and m/$LEADIN(\.\s+[^\s;\`:]+\s+([^\s;]+))/) {
 		if ($2 =~ /^(\&|\||\d?>|<)/) {
 		    # everything is ok
 		    ;
@@ -408,7 +409,7 @@ sub script_is_evil_and_wrong {
         last if (++$i > 55);
         if (m~
 	    # the exec should either be "eval"ed or a new statement
-	    (^\s*|\beval\s*[\'\"]|(;|&&)\s*)
+	    (^\s*|\beval\s*[\'\"]|(;|&&|\b(then|else))\s*)
 
 	    # eat anything between the exec and $0
 	    exec\s*.+\s*
@@ -458,7 +459,6 @@ sub script_is_evil_and_wrong {
 }
 
 sub init_hashes {
-    my $LEADIN = qr'(?:(^|[`&;(|{])\s*|(if|then|do|while|shell)\s+)';
 
     %bashisms = (
 	qr'(?:^|\s+)function \w+(\s|\(|\Z)' => q<'function' is useless>,