Merge remote-tracking branch 'tor-github/pr/1462'

5 files changed, 241 insertions, 91 deletions

diff --git a/scripts/coccinelle/check_cocci_parse.sh b/scripts/coccinelle/check_cocci_parse.sh
new file mode 100755
index 0000000000..5c27c7bc88
--- /dev/null
+++ b/scripts/coccinelle/check_cocci_parse.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+
+# If we have coccinelle installed, run try_parse.sh on every filename passed
+# as an argument. If no filenames are supplied, scan a standard Tor 0.3.5 or
+# later directory layout.
+#
+# Uses the default coccinelle exceptions file, or $TOR_COCCI_EXCEPTIONS_FILE,
+# if it is set.
+#
+# Use TOR_COCCI_EXCEPTIONS_FILE=/dev/null check_cocci_parse.sh to disable
+# the default exception file.
+#
+# If spatch is not installed, remind the user to install it, but exit with
+# a success error status.
+
+scripts_cocci="$(dirname "$0")"
+top="$scripts_cocci/../.."
+try_parse="$scripts_cocci/try_parse.sh"
+
+exitcode=0
+
+export TOR_COCCI_EXCEPTIONS_FILE="${TOR_COCCI_EXCEPTIONS_FILE:-$scripts_cocci/exceptions.txt}"
+
+if ! command -v spatch; then
+    echo "Install coccinelle's spatch to check cocci C parsing!"
+    exit "$exitcode"
+fi
+
+if test $# -ge 1 ; then
+  "$try_parse" "$@"
+  exitcode=$?
+else
+  # This is the layout in 0.3.5
+  "$try_parse" \
+    src/lib/*/*.[ch] \
+    src/core/*/*.[ch] \
+    src/feature/*/*.[ch] \
+    src/app/*/*.[ch] \
+    src/test/*.[ch] \
+    src/test/*/*.[ch] \
+    src/tools/*.[ch]
+  exitcode=$?
+fi
+
+if test "$exitcode" != 0 ; then
+    echo "Please fix these cocci parsing errors in the above files"
+    echo "Set VERBOSE=1 for more details"
+    echo "Try running test-operator-cleanup or 'make autostyle-operators'"
+    echo "As a last resort, you can modify scripts/coccinelle/exceptions.txt"
+fi
+
+exit "$exitcode"
diff --git a/scripts/coccinelle/exceptions.txt b/scripts/coccinelle/exceptions.txt
new file mode 100644
index 0000000000..473f4b22c5
--- /dev/null
+++ b/scripts/coccinelle/exceptions.txt
@@ -0,0 +1,24 @@
+# A list of exception patterns for check_cocci_parse.sh
+# Passed to 'grep -f'
+src/lib/cc/compat_compiler.h
+src/lib/container/handles.h
+src/lib/container/map.c
+src/lib/container/map.h
+src/lib/container/order.c
+src/lib/crypt_ops/crypto_rand.c
+src/lib/fs/files.h
+src/lib/log/util_bug.c
+src/lib/pubsub/pubsub_macros.h
+src/lib/smartlist_core/smartlist_foreach.h
+src/lib/testsupport/testsupport.h
+src/lib/tls/tortls.h
+src/lib/tls/tortls_openssl.c
+src/lib/tls/x509.h
+src/lib/version/version.c
+src/core/mainloop/connection.c
+src/core/or/reasons.c
+src/feature/dirclient/dirclient.c
+src/feature/nodelist/networkstatus.c
+src/test/test_address.c
+src/test/test_hs_cache.c
+src/test/test_hs_descriptor.c
diff --git a/scripts/coccinelle/try_parse.sh b/scripts/coccinelle/try_parse.sh
index 0f91e31702..a90e51b4aa 100755
--- a/scripts/coccinelle/try_parse.sh
+++ b/scripts/coccinelle/try_parse.sh
@@ -2,6 +2,11 @@
 
 # Echo the name of every argument of this script that is not "perfect"
 # according to coccinelle's --parse-c.
+#
+# If $TOR_COCCI_EXCEPTIONS_FILE is non-empty, skip any files that match the
+# patterns in the exception file, according to "grep -f"
+#
+# If VERBOSE is non-empty, log spatch errors and skipped files.
 
 top="$(dirname "$0")/../.."
 
@@ -9,16 +14,28 @@ exitcode=0
 
 for fn in "$@"; do
 
-    if spatch -macro_file_builtins "$top"/scripts/coccinelle/tor-coccinelle.h \
-              -I "$top" -I "$top"/src -I "$top"/ext --defined COCCI \
+    if test "${TOR_COCCI_EXCEPTIONS_FILE}" ; then
+        skip_fn=$(echo "$fn" | grep -f "${TOR_COCCI_EXCEPTIONS_FILE}")
+        if test "${skip_fn}" ; then
+            if test "${VERBOSE}" != ""; then
+                echo "Skipping '${skip_fn}'"
+            fi
+            continue
+        fi
+    fi
+
+    if spatch --macro-file-builtins \
+              "$top"/scripts/coccinelle/tor-coccinelle.h \
+              --defined COCCI \
               --parse-c "$fn" \
               2>/dev/null | grep "perfect = 1" > /dev/null; then
         : # it's perfect
     else
         echo "$fn"
         if test "${VERBOSE}" != ""; then
-            spatch -macro_file_builtins "$top"/scripts/coccinelle/tor-coccinelle.h \
-                   -I "$top" -I "$top"/src -I "$top"/ext --defined COCCI \
+            spatch --macro-file-builtins \
+                   "$top"/scripts/coccinelle/tor-coccinelle.h \
+                   --defined COCCI \
                    --parse-c "$fn"
         fi
         exitcode=1
diff --git a/scripts/git/pre-commit.git-hook b/scripts/git/pre-commit.git-hook
index 1c381ec60a..2e476c310e 100755
--- a/scripts/git/pre-commit.git-hook
+++ b/scripts/git/pre-commit.git-hook
@@ -13,30 +13,61 @@ cd "$workdir" || exit 1
 
 set -e
 
+if [ $# -eq 0 ]; then
+    # When called in pre-commit, check the files modified in this commit
+    CHECK_FILTER="git diff --cached --name-only --diff-filter=ACMR"
+    # Use the appropriate owned tor source list to filter the changed files
+    if [ -d src/lib ]; then
+        # This is the layout in 0.3.5
+        CHECK_FILES="$($CHECK_FILTER \
+                           src/lib/*/*.[ch] \
+                           src/core/*/*.[ch] \
+                           src/feature/*/*.[ch] \
+                           src/app/*/*.[ch] \
+                           src/test/*.[ch] \
+                           src/test/*/*.[ch] \
+                           src/tools/*.[ch] \
+                      )"
+    elif [ -d src/common ]; then
+        # This was the layout before 0.3.5
+        CHECK_FILES="$($CHECK_FILTER \
+                           src/common/*/*.[ch] \
+                           src/or/*/*.[ch] \
+                           src/test/*.[ch] \
+                           src/test/*/*.[ch] \
+                           src/tools/*.[ch]
+                      )"
+    fi
+else
+    # When called in pre-push, concatenate the argument array
+    # Fails on special characters in file names
+    CHECK_FILES="$*"
+fi
+
+## General File Checks
+
 if [ -n "$(ls ./changes/)" ]; then
     python scripts/maint/lintChanges.py ./changes/*
 fi
 
-if [ -d src/lib ]; then
-    # This is the layout in 0.3.5
-    perl scripts/maint/checkSpace.pl -C \
-         src/lib/*/*.[ch] \
-         src/core/*/*.[ch] \
-         src/feature/*/*.[ch] \
-         src/app/*/*.[ch] \
-         src/test/*.[ch] \
-         src/test/*/*.[ch] \
-         src/tools/*.[ch]
-elif [ -d src/common ]; then
-    # This was the layout before 0.3.5
-    perl scripts/maint/checkSpace.pl -C \
-         src/common/*/*.[ch] \
-         src/or/*/*.[ch] \
-         src/test/*.[ch] \
-         src/test/*/*.[ch] \
-         src/tools/*.[ch]
+if [ -e scripts/maint/checkShellScripts.sh ]; then
+    scripts/maint/checkShellScripts.sh
+fi
+
+if [ ! "$CHECK_FILES" ]; then
+    echo "No modified tor-owned source files, skipping further checks"
+    exit 0
 fi
 
+## Owned Source File Checks
+
+printf "Modified tor-owned source files:\n%s\n" "$CHECK_FILES"
+
+# We want word splitting here, because file names are space separated
+# shellcheck disable=SC2086
+perl scripts/maint/checkSpace.pl -C \
+     $CHECK_FILES
+
 if test -e scripts/maint/practracker/includes.py; then
     python scripts/maint/practracker/includes.py
 fi
@@ -54,6 +85,13 @@ if [ -e "${PT_DIR}/practracker.py" ]; then
     fi
 fi
 
-if [ -e scripts/maint/checkShellScripts.sh ]; then
-    scripts/maint/checkShellScripts.sh
+if [ -e scripts/coccinelle/check_cocci_parse.sh ]; then
+
+    # Run a verbose cocci parse check on the changed files
+    # (spatch is slow, so we don't want to check all the files.)
+    #
+    # We want word splitting here, because file names are space separated
+    # shellcheck disable=SC2086
+    VERBOSE=1 scripts/coccinelle/check_cocci_parse.sh \
+              $CHECK_FILES
 fi
diff --git a/scripts/git/pre-push.git-hook b/scripts/git/pre-push.git-hook
index f4504c4215..2f3608029a 100755
--- a/scripts/git/pre-push.git-hook
+++ b/scripts/git/pre-push.git-hook
@@ -16,91 +16,110 @@
 # The following sample script was used as starting point:
 # https://github.com/git/git/blob/master/templates/hooks--pre-push.sample
 
+# Are you adding a new check to the git hooks?
+#  - Common checks belong in the pre-commit hook
+#  - Push-only checks belong in the pre-push hook
+
 echo "Running pre-push hook"
 
 z40=0000000000000000000000000000000000000000
 
 upstream_name=${TOR_UPSTREAM_REMOTE_NAME:-"upstream"}
 
-# Are you adding a new check to the git hooks?
-#  - Common checks belong in the pre-commit hook
-#  - Push-only checks belong in the pre-push hook
-#
-# Call the pre-commit hook for the common checks, if it is executable.
 workdir=$(git rev-parse --show-toplevel)
-if [ -x "$workdir/.git/hooks/pre-commit" ]; then
-  if ! "$workdir"/.git/hooks/pre-commit; then
-    exit 1
-  fi
-fi
 
 remote="$1"
-
 remote_name=$(git remote --verbose | grep "$2" | awk '{print $1}' | head -n 1)
 
-if [[ "$remote_name" != "$upstream_name" ]]; then
-  echo "Not pushing to upstream - refraining from further checks"
-  exit 0
-fi
 
 ref_is_upstream_branch() {
-        if [ "$1" == "refs/heads/master" ] ||
-                [[ "$1" == refs/heads/release-* ]] ||
-                [[ "$1" == refs/heads/maint-* ]]
-        then
-                return 1
-        fi
+    if [ "$1" == "refs/heads/master" ] ||
+        [[ "$1" == refs/heads/release-* ]] ||
+        [[ "$1" == refs/heads/maint-* ]]; then
+        return 1
+    fi
 }
 
 # shellcheck disable=SC2034
 while read -r local_ref local_sha remote_ref remote_sha
 do
-	if [ "$local_sha" = $z40 ]
-	then
-		# Handle delete
-		:
-	else
-		if [ "$remote_sha" = $z40 ]
-		then
-			# New branch, examine all commits
-			range="$local_sha"
-		else
-			# Update to existing branch, examine new commits
-			range="$remote_sha..$local_sha"
-		fi
-
-                if (ref_is_upstream_branch "$local_ref" == 0 ||
-                        ref_is_upstream_branch "$remote_ref"  == 0) &&
-                        [ "$local_ref" != "$remote_ref" ]
-                then
-                        if [ "$remote" == "origin" ]
-                        then
-                                echo >&2 "Not pushing: $local_ref to $remote_ref"
-			        echo >&2 "If you really want to push this, use --no-verify."
-                                exit 1
-                        else
-                                continue
-                        fi
-                fi
-
-                # Check for fixup! commit
-                commit=$(git rev-list -n 1 --grep '^fixup!' "$range")
-		if [ -n "$commit" ]
-		then
-			echo >&2 "Found fixup! commit in $local_ref, not pushing"
-			echo >&2 "If you really want to push this, use --no-verify."
-			exit 1
-		fi
-
-                # Check for squash! commit
-                commit=$(git rev-list -n 1 --grep '^squash!' "$range")
-		if [ -n "$commit" ]
-		then
-			echo >&2 "Found squash! commit in $local_ref, not pushing"
-			echo >&2 "If you really want to push this, use --no-verify."
-			exit 1
-		fi
-	fi
+    if [ "$local_sha" = $z40 ]; then
+        # Handle delete
+        :
+    else
+        if [ "$remote_sha" = $z40 ]; then
+            # New branch, examine commits not in master
+            range="master...$local_sha"
+        else
+            # Update to existing branch, examine new commits
+            range="$remote_sha..$local_sha"
+        fi
+
+        # Call the pre-commit hook for the common checks, if it is executable
+        # Only check the files newly modified in this branch
+        CHECK_FILTER="git diff --name-only --diff-filter=ACMR $range"
+        # Use the appropriate owned tor source list to filter the changed files
+        if [ -d src/lib ]; then
+            # This is the layout in 0.3.5
+            CHECK_FILES="$($CHECK_FILTER \
+                               src/lib/*/*.[ch] \
+                               src/core/*/*.[ch] \
+                               src/feature/*/*.[ch] \
+                               src/app/*/*.[ch] \
+                               src/test/*.[ch] \
+                               src/test/*/*.[ch] \
+                               src/tools/*.[ch] \
+                          )"
+        elif [ -d src/common ]; then
+            # This was the layout before 0.3.5
+            CHECK_FILES="$($CHECK_FILTER \
+                               src/common/*/*.[ch] \
+                               src/or/*/*.[ch] \
+                               src/test/*.[ch] \
+                               src/test/*/*.[ch] \
+                               src/tools/*.[ch]
+                          )"
+        fi
+
+        # We want word splitting here, because file names are space separated
+        # shellcheck disable=SC2086
+        if ! "$workdir/"scripts/git/pre-commit.git-hook $CHECK_FILES; then
+            exit 1
+        fi
+
+        if [[ "$remote_name" != "$upstream_name" ]]; then
+            echo "Not pushing to upstream - refraining from further checks"
+            continue
+        fi
+
+        if (ref_is_upstream_branch "$local_ref" == 0 ||
+                ref_is_upstream_branch "$remote_ref"  == 0) &&
+            [ "$local_ref" != "$remote_ref" ]; then
+            if [ "$remote" == "origin" ]; then
+                echo >&2 "Not pushing: $local_ref to $remote_ref"
+                echo >&2 "If you really want to push this, use --no-verify."
+                exit 1
+            else
+                continue
+            fi
+        fi
+
+        # Check for fixup! commit
+        commit=$(git rev-list -n 1 --grep '^fixup!' "$range")
+        if [ -n "$commit" ]; then
+            echo >&2 "Found fixup! commit in $local_ref, not pushing"
+            echo >&2 "If you really want to push this, use --no-verify."
+            exit 1
+        fi
+
+        # Check for squash! commit
+        commit=$(git rev-list -n 1 --grep '^squash!' "$range")
+        if [ -n "$commit" ]; then
+            echo >&2 "Found squash! commit in $local_ref, not pushing"
+            echo >&2 "If you really want to push this, use --no-verify."
+            exit 1
+        fi
+    fi
 done
 
 exit 0