Merge Lint shell code and add shellcheck CI job

This should avoid unconsciously introducing regressions wrt. best practices for shell code.

MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/842
Acked-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Christian Boltz <apparmor@cboltz.de>

.gitlab-ci.yml

@@ -37,6 +37,7 @@ build-all:
 
 test-all:
   stage: test
+  needs: ["build-all"]
   script:
       - make -C libraries/libapparmor check
       - make -C parser check
@@ -51,6 +52,20 @@ test-all:
       - utils/test/htmlcov/
     when: always
 
+shellcheck:
+  stage: test
+  needs: []
+  script:
+  - apt-get install --no-install-recommends -y file shellcheck xmlstarlet
+  - shellcheck --version
+  - './tests/bin/shellcheck-tree --format=checkstyle
+       | xmlstarlet tr tests/checkstyle2junit.xslt
+       > shellcheck.xml'
+  artifacts:
+    when: always
+    reports:
+      junit: shellcheck.xml
+
 # Disabled due to aa-logprof dependency on /sbin/apparmor_parser existing
 #      - make -C profiles check-profiles
 

.shellcheckrc

@@ -0,0 +1,10 @@
+# Don't follow source'd scripts
+disable=SC1090
+disable=SC1091
+
+# dash supports 'local'
+disable=SC2039
+disable=SC3043
+
+# dash supports 'echo -n'
+disable=SC3037

parser/rc.apparmor.functions

@@ -91,7 +91,7 @@ is_container_with_internal_policy() {
 	local ns_name
 
 	# WSL needs to be detected explicitly
-	if [ $(systemd-detect-virt --container) = "wsl" ]; then
+	if [ "$(systemd-detect-virt --container)" = "wsl" ]; then
 		return 0
 	fi
 
@@ -131,8 +131,8 @@ __parse_profiles_dir() {
 		return 1
 	fi
 
-	"$PARSER" $PARSER_OPTS "$parser_cmd" -- "$profile_dir"
-	if [ $? -ne 0 ]; then
+	# shellcheck disable=SC2086
+	if ! "$PARSER" $PARSER_OPTS "$parser_cmd" -- "$profile_dir"; then
 		status=1
 		aa_log_failure_msg "At least one profile failed to load"
 	fi
@@ -217,7 +217,7 @@ apparmor_start() {
 	fi
 
 	# if there is anything in the profiles file don't load
-	if ! read -r line < "$SFS_MOUNTPOINT/profiles"; then
+	if ! read -r _ < "$SFS_MOUNTPOINT/profiles"; then
 		parse_profiles load
 	else
 		aa_log_skipped_msg ": already loaded with profiles."

tests/bin/shellcheck-tree

@@ -0,0 +1,34 @@
+#!/usr/bin/python3
+
+import glob
+import re
+import subprocess
+import sys
+from pathlib import Path
+
+
+def is_excluded(f):
+    return (re.match(
+        r"^([.]git/|parser/tst/|tests/|utils/test/|parser/rc[.]apparmor[.]slackware)",
+        f,
+    ) or Path(f).is_dir())
+
+
+def mimetype(f):
+    return subprocess.run(['file', '--brief', '--mime-type', f],
+                          stdout=subprocess.PIPE,
+                          universal_newlines=True,
+                          check=True).stdout.rstrip()
+
+
+def is_shell_script(f):
+    return mimetype(f) == "text/x-shellscript"
+
+
+shell_scripts = [
+    f for f in glob.glob("**/*", recursive=True)
+    if not is_excluded(f) and is_shell_script(f)
+]
+
+sys.exit(
+    subprocess.run(['shellcheck'] + sys.argv[1:] + shell_scripts).returncode)

tests/checkstyle2junit.xslt

@@ -0,0 +1,44 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
+  <xsl:output encoding="UTF-8" method="xml"></xsl:output>
+
+  <xsl:template match="/">
+    <testsuite>
+      <xsl:attribute name="tests">
+        <xsl:value-of select="count(.//file)" />
+      </xsl:attribute>
+      <xsl:attribute name="failures">
+        <xsl:value-of select="count(.//error)" />
+      </xsl:attribute>
+      <xsl:for-each select="//checkstyle">
+        <xsl:apply-templates />
+      </xsl:for-each>
+    </testsuite>
+  </xsl:template>
+
+  <xsl:template match="file">
+    <testcase>
+      <xsl:attribute name="classname">
+        <xsl:value-of select="@name" />
+      </xsl:attribute>
+      <xsl:attribute name="name">
+        <xsl:value-of select="@name" />
+      </xsl:attribute>
+      <xsl:apply-templates select="node()" />
+    </testcase>
+  </xsl:template>
+
+  <xsl:template match="error">
+    <failure>
+      <xsl:attribute name="type">
+        <xsl:value-of select="@source" />
+      </xsl:attribute>
+      <xsl:text>Line </xsl:text>
+      <xsl:value-of select="@line" />
+      <xsl:text>: </xsl:text>
+      <xsl:value-of select="@message" />
+      <xsl:text> See https://www.shellcheck.net/wiki/</xsl:text>
+      <xsl:value-of select="substring(@source, '12')" />
+    </failure>
+  </xsl:template>
+</xsl:stylesheet>

tests/regression/apparmor/dbus.inc

@@ -14,7 +14,7 @@ ${__dbus_var_decl}
 $test {
   @{gen $test}
   unix,
-  $@
+  $*
   signal receive peer=unconfined,
 }
 EOF

tests/regression/apparmor/dbus_message.sh

@@ -33,7 +33,7 @@ confined_args="--log=$confined_log $listnames"
 message_gendbusprofile()
 {
 	gendbusprofile "${confined_log} w,
-  $@"
+  $*"
 }
 
 start_bus

tests/regression/apparmor/dbus_service.sh

@@ -62,7 +62,7 @@ service_runchecktest()
 service_gendbusprofile()
 {
 	gendbusprofile "$unconfined_log w,
-  $@"
+  $*"
 }
 
 start_bus

tests/regression/apparmor/dbus_unrequested_reply.sh

@@ -63,7 +63,7 @@ ur_gendbusprofile()
 {
 	gendbusprofile "$confined_log w,
   dbus bind bus=$bus name=$dest,
-  $@"
+  $*"
 }
 
 start_bus

tests/regression/apparmor/namespaces.sh

@@ -58,8 +58,16 @@ genprofile_ns_and_verify() {
 
 	[ -d /sys/kernel/security/apparmor/policy/namespaces/${ns} ]
 	local dir_created=$?
-	[ -d /sys/kernel/security/apparmor/policy/namespaces/${ns}/profiles/${prof}* ]
-	local prof_created=$?
+	local prof_created=1
+	for d in /sys/kernel/security/apparmor/policy/namespaces/${ns}/profiles/${prof}*; do
+		if [ -d "$d" ]; then
+			prof_created=0
+		fi
+		# Either $d exists and we've set prof_created to 0. Or it does
+		# not, because its value is the unexpanded pattern above.
+		# Either way, this is all we needed to know.
+		break
+	done
 	removeprofile
 	if [ $dir_created -ne 0 ]; then
 		echo "Error: ${testname} failed. Test '${desc}' did not create the expected namespace directory in apparmorfs: policy/namespaces/${ns}"

utils/aa-decode

@@ -58,7 +58,7 @@ if [ -n "$1" ]; then
         exit 1
     fi
 
-    d=`decode $e`
+    d=$(decode "$e")
     if [ -z "$d" ]; then
         echo "Could not decode string"
         exit 1
@@ -71,6 +71,12 @@ fi
 # For now just look at 'name=...' and 'profile=...',
 # so validate input against this and output based on it.
 # TODO: better handle other cases too
+# This loop relies on subtle shell quoting/escaping behavior, and
+# non-trivial surgery would be required to make shellcheck happy.
+# shellcheck disable=SC2162
+# shellcheck disable=SC2086
+# shellcheck disable=SC2001
+# shellcheck disable=SC2006
 while read line ; do
 
     # check if line contains encoded name= or profile=

utils/aa-remove-unknown

@@ -47,7 +47,7 @@ Options:
 if [ "$#" -gt 1 ] ; then
 	usage 1
 elif [ "$#" -eq 1 ] ; then
-	if [ "$1" = "-h" -o "$1" = "--help" ] ; then
+	if [ "$1" = "-h" ] || [ "$1" = "--help" ] ; then
 		usage 0
 	elif [ "$1" = "-n" ] ; then
 		DRY_RUN=1
@@ -63,7 +63,7 @@ fi
 # We have to do this check because error checking awk's getline() below is
 # tricky and, as is, results in an infinite loop when apparmorfs returns an
 # error from open().
-if ! IFS= read line < "$PROFILES" ; then
+if ! IFS= read -r _ < "$PROFILES" ; then
 	echo "ERROR: Unable to read apparmorfs profiles file" 1>&2
 	exit 1
 elif [ ! -w "$REMOVE" ] ; then
@@ -79,6 +79,7 @@ fi
 # on removing the parent profile when the profile has had its
 # child profile names changed.
 
+# shellcheck disable=SC2086
 LOADED_PROFILES=$("$PARSER" -N $PROFILE_DIRS) || {
 	ret=$?
 	echo 'apparmor_parser exited with failure, aborting.' >&2
@@ -103,7 +104,7 @@ END {
     }
 }
 ' | LC_COLLATE=C sort -r | \
-	while IFS= read profile ; do
+	while IFS= read -r profile ; do
 		if [ "$DRY_RUN" -ne 0 ]; then
 			echo "Would remove '${profile}'"
 		else

utils/test/runtests-py2.sh

@@ -1,5 +0,0 @@
-#!/bin/sh
-
-RUNTESTS_PY__PYTHON_BINARY=python2
-. ./runtests-py3.sh
-