Merge branch 'pr-1569-squashed'

10 files changed, 208 insertions, 22 deletions

diff --git a/Makefile.am b/Makefile.am
index 38040a4e75..b01601ecc5 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -168,6 +168,13 @@ EXTRA_DIST+= \
 	ReleaseNotes							\
 	scripts/maint/checkIncludes.py					\
 	scripts/maint/checkSpace.pl					\
+	scripts/maint/checkSpaceTest.sh					\
+	scripts/maint/checkspace_tests/dubious.c			\
+	scripts/maint/checkspace_tests/dubious.h			\
+	scripts/maint/checkspace_tests/expected.txt			\
+	scripts/maint/checkspace_tests/good_guard.h			\
+	scripts/maint/checkspace_tests/same_guard.h			\
+	scripts/maint/checkspace_tests/subdir/dubious.c			\
 	scripts/maint/checkShellScripts.sh				\
 	scripts/maint/practracker/README				\
 	scripts/maint/practracker/exceptions.txt			\
diff --git a/scripts/maint/checkSpace.pl b/scripts/maint/checkSpace.pl
index 7be7f2a3c9..f4e6f733c8 100755
--- a/scripts/maint/checkSpace.pl
+++ b/scripts/maint/checkSpace.pl
@@ -4,9 +4,16 @@ use strict;
 use warnings;
 
 my $found = 0;
+my $COLON_POS = 10;
+
 sub msg {
   $found = 1;
-  print "$_[0]";
+  my $v = shift;
+  $v =~ /^\s*([^:]+):(.*)$/;
+  chomp(my $errtype = $1);
+  my $rest = $2;
+  my $padding = ' ' x ($COLON_POS - length $errtype);
+  print "$padding$errtype:$rest\n";
 }
 
 my $C = 0;
@@ -29,7 +36,7 @@ for my $fn (@ARGV) {
     my $basename = $fn;
     $basename =~ s#.*/##;
     if ($basenames{$basename}) {
-        msg "Duplicate fnames: $fn and $basenames{$basename}.\n";
+        msg "dup fname:$fn (same as $basenames{$basename}).\n";
     } else {
         $basenames{$basename} = $fn;
     }
@@ -42,12 +49,12 @@ for my $fn (@ARGV) {
         #    (We insist on lines that end with a single LF character, not
         #    CR LF.)
         if (/\r/) {
-            msg "       CR:$fn:$.\n";
+            msg "CR:$fn:$.\n";
         }
         ## Warn about tabs.
         #    (We only use spaces)
         if (/\t/) {
-            msg "      TAB:$fn:$.\n";
+            msg "TAB:$fn:$.\n";
         }
         ## Warn about labels that don't have a space in front of them
         #    (We indent every label at least one space)
@@ -63,12 +70,12 @@ for my $fn (@ARGV) {
         ## Warn about control keywords without following space.
         #    (We put a space after every 'if', 'while', 'for', 'switch', etc)
         if ($C && /\s(?:if|while|for|switch)\(/) {
-            msg "      KW(:$fn:$.\n";
+            msg "KW(:$fn:$.\n";
         }
         ## Warn about #else #if instead of #elif.
         #    (We only allow #elif)
         if (($lastline =~ /^\# *else/) and ($_ =~ /^\# *if/)) {
-            msg " #else#if:$fn:$.\n";
+            msg "#else#if:$fn:$.\n";
         }
         ## Warn about some K&R violations
         #    (We use K&R-style C, where open braces go on the same line as
@@ -83,19 +90,19 @@ for my $fn (@ARGV) {
             msg "non-K&R {:$fn:$.\n";
         }
         if (/^\s*else/ and $lastline =~ /\}$/) {
-            msg "  }\\nelse:$fn:$.\n";
+            msg "}\\nelse:$fn:$.\n";
         }
         $lastline = $_;
         ## Warn about unnecessary empty lines.
         #   (Don't put an empty line before a line that contains nothing
         #   but a closing brace.)
         if ($lastnil && /^\s*}\n/) {
-            msg "  UnnecNL:$fn:$.\n";
+            msg "UnnecNL:$fn:$.\n";
         }
         ## Warn about multiple empty lines.
         #   (At most one blank line in a row.)
         if ($lastnil && /^$/) {
-            msg " DoubleNL:$fn:$.\n";
+            msg "DoubleNL:$fn:$.\n";
         } elsif (/^$/) {
             $lastnil = 1;
         } else {
@@ -105,7 +112,7 @@ for my $fn (@ARGV) {
         ## accept double-line lines.
         #   (Don't make lines wider than 80 characters, including newline.)
         if (/^.{80}/) {
-            msg "     Wide:$fn:$.\n";
+            msg "Wide:$fn:$.\n";
         }
         ### Juju to skip over comments and strings, since the tests
         ### we're about to do are okay there.
@@ -146,26 +153,26 @@ for my $fn (@ARGV) {
             next if /^\#/;
             ## Skip C++-style comments.
             if (m!//!) {
-                #    msg "       //:$fn:$.\n";
+                #    msg "//:$fn:$.\n";
                 s!//.*!!;
             }
             ## Warn about unquoted braces preceded by non-space.
             #   (No character except a space should come before a {)
             if (/([^\s'])\{/) {
-                msg "       $1\{:$fn:$.\n";
+                msg "$1\{:$fn:$.\n";
             }
             ## Warn about double semi-colons at the end of a line.
             if (/;;$/) {
-                msg "       double semi-colons at the end of $. in $fn\n"
+                msg ";;:$fn:$.\n"
             }
             ## Warn about multiple internal spaces.
             #if (/[^\s,:]\s{2,}[^\s\\=]/) {
-            #    msg "     X  X:$fn:$.\n";
+            #    msg "X  X:$fn:$.\n";
             #}
             ## Warn about { with stuff after.
             #s/\s+$//;
             #if (/\{[^\}\\]+$/) {
-            #    msg "     {X:$fn:$.\n";
+            #    msg "{X:$fn:$.\n";
             #}
             ## Warn about function calls with space before parens.
             #   (Don't put a space between the name of a function and its
@@ -177,7 +184,7 @@ for my $fn (@ARGV) {
                     $1 ne "void" and $1 ne "__attribute__" and $1 ne "op" and
                     $1 ne "size_t" and $1 ne "double" and $1 ne "uint64_t" and
                     $1 ne "workqueue_reply_t" and $1 ne "bool") {
-                    msg "     fn ():$fn:$.\n";
+                    msg "fn ():$fn:$.\n";
                 }
             }
             ## Warn about functions not declared at start of line.
@@ -206,28 +213,28 @@ for my $fn (@ARGV) {
             ## Check for forbidden functions except when they are
             # explicitly permitted
             if (/\bassert\(/ && not /assert OK/) {
-                msg "assert :$fn:$.   (use tor_assert)\n";
+                msg "assert:$fn:$.   (use tor_assert)\n";
             }
             if (/\bmemcmp\(/ && not /memcmp OK/) {
-                msg "memcmp :$fn:$.   (use {tor,fast}_mem{eq,neq,cmp}\n";
+                msg "memcmp:$fn:$.   (use {tor,fast}_mem{eq,neq,cmp}\n";
             }
             # always forbidden.
             if (not /\ OVERRIDE\ /) {
                 if (/\bstrcat\(/ or /\bstrcpy\(/ or /\bsprintf\(/) {
-                    msg "$& :$fn:$.\n";
+                    msg "$&:$fn:$.\n";
                 }
                 if (/\bmalloc\(/ or /\bfree\(/ or /\brealloc\(/ or
                     /\bstrdup\(/ or /\bstrndup\(/ or /\bcalloc\(/) {
-                    msg "$& :$fn:$.    (use tor_malloc, tor_free, etc)\n";
+                    msg "$&:$fn:$.    (use tor_malloc, tor_free, etc)\n";
                 }
             }
         }
     }
     if ($isheader && $C) {
         if ($seenguard < 2) {
-            msg "$fn:No #ifndef/#define header guard pair found.\n";
+            msg "noguard:$fn (No #ifndef/#define header guard pair found)\n";
         } elsif ($guardnames{$guardname}) {
-            msg "$fn:Guard macro $guardname also used in $guardnames{$guardname}\n";
+            msg "dupguard:$fn (Guard macro $guardname also used in $guardnames{$guardname})\n";
         } else {
             $guardnames{$guardname} = $fn;
         }
diff --git a/scripts/maint/checkSpaceTest.sh b/scripts/maint/checkSpaceTest.sh
new file mode 100755
index 0000000000..e1d207a1a8
--- /dev/null
+++ b/scripts/maint/checkSpaceTest.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+# Copyright 2019, The Tor Project, Inc.
+# See LICENSE for licensing information
+
+# Integration test for checkSpace.pl, which we want to rewrite.
+
+umask 077
+set -e
+
+# Skip this test if we're running on Windows; we expect line-ending
+# issues in that case.
+case "$(uname -s)" in
+    CYGWIN*) WINDOWS=1;;
+    MINGW*) WINDOWS=1;;
+    MSYS*) WINDOWS=1;;
+    *) WINDOWS=0;;
+esac
+if test "$WINDOWS" = 1; then
+    # This magic value tells automake that the test has been skipped.
+    exit 77
+fi
+
+# make a safe space for temporary files
+DATA_DIR=$(mktemp -d -t tor_checkspace_tests.XXXXXX)
+trap 'rm -rf "$DATA_DIR"' 0
+
+RECEIVED_FNAME="${DATA_DIR}/got.txt"
+
+cd "$(dirname "$0")/checkspace_tests"
+
+# we expect this to give an error code.
+../checkSpace.pl -C ./*.[ch] ./*/*.[ch] > "${RECEIVED_FNAME}" && exit 1
+
+diff -u expected.txt "${RECEIVED_FNAME}" || exit 1
+
+echo "OK"
diff --git a/scripts/maint/checkspace_tests/dubious.c b/scripts/maint/checkspace_tests/dubious.c
new file mode 100644
index 0000000000..59c5f8e4fe
--- /dev/null
+++ b/scripts/maint/checkspace_tests/dubious.c
@@ -0,0 +1,83 @@
+
+// The { coming up should be on its own line.
+int
+foo(void) {
+  // There should be a space before (1)
+  if(1) x += 1;
+
+  // The following empty line is unnecessary.
+
+}
+
+
+// There should be a newline between void and bar.
+void bar(void)
+{
+  // too wide:
+  testing("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
+}
+
+long
+bad_spacing()
+{
+  // here comes a tab
+	return 2;
+  // here comes a label without space:
+foo:
+  ;
+}
+
+// Here comes a CR:

+
+// Trailing space:         
+
+int
+non_k_and_r(void)
+{
+  // non-k&r
+  if (foo)
+    {
+      // double-semi
+      return 1;;
+    }
+  else
+    {
+      return 2;
+    }
+}
+
+// #else #if causes a warning.
+#if 1
+#else
+#if 2
+#else
+#endif
+#endif
+
+// always space before a brace.
+foo{
+}
+
+void
+unexpected_space(void)
+{
+  // This space gives a warning.
+  foobar (77);
+}
+
+void
+bad_function_calls(long)
+{
+  // These are forbidden:
+  assert(1);
+  memcmp("a","b",1);
+  strcat(foo,x);
+  strcpy(foo,y);
+  sprintf(foo,"x");
+  malloc(7);
+  free(p);
+  realloc(p);
+  strdup(s);
+  strndup(s,10);
+  calloc(a,b);
+}
diff --git a/scripts/maint/checkspace_tests/dubious.h b/scripts/maint/checkspace_tests/dubious.h
new file mode 100644
index 0000000000..744ec33955
--- /dev/null
+++ b/scripts/maint/checkspace_tests/dubious.h
@@ -0,0 +1,4 @@
+
+// no guards.
+
+int foo(int);
diff --git a/scripts/maint/checkspace_tests/expected.txt b/scripts/maint/checkspace_tests/expected.txt
new file mode 100644
index 0000000000..935b750ef9
--- /dev/null
+++ b/scripts/maint/checkspace_tests/expected.txt
@@ -0,0 +1,31 @@
+    fn() {:./dubious.c:4
+       KW(:./dubious.c:6
+   UnnecNL:./dubious.c:10
+  DoubleNL:./dubious.c:12
+   tp fn():./dubious.c:15
+      Wide:./dubious.c:17
+       TAB:./dubious.c:24
+ nosplabel:./dubious.c:26
+        CR:./dubious.c:30
+ Space@EOL:./dubious.c:32
+ non-K&R {:./dubious.c:39
+        ;;:./dubious.c:41
+   }\nelse:./dubious.c:43
+  #else#if:./dubious.c:52
+        o{:./dubious.c:58
+    fn() {:./dubious.c:58
+     fn ():./dubious.c:65
+    assert:./dubious.c:72   (use tor_assert)
+    memcmp:./dubious.c:73   (use {tor,fast}_mem{eq,neq,cmp}
+   strcat(:./dubious.c:74
+   strcpy(:./dubious.c:75
+  sprintf(:./dubious.c:76
+   malloc(:./dubious.c:77    (use tor_malloc, tor_free, etc)
+     free(:./dubious.c:78    (use tor_malloc, tor_free, etc)
+  realloc(:./dubious.c:79    (use tor_malloc, tor_free, etc)
+   strdup(:./dubious.c:80    (use tor_malloc, tor_free, etc)
+  strndup(:./dubious.c:81    (use tor_malloc, tor_free, etc)
+   calloc(:./dubious.c:82    (use tor_malloc, tor_free, etc)
+   noguard:./dubious.h (No #ifndef/#define header guard pair found)
+  dupguard:./same_guard.h (Guard macro GUARD_MACRO_H also used in ./good_guard.h)
+ dup fname:./subdir/dubious.c (same as ./dubious.c).
diff --git a/scripts/maint/checkspace_tests/good_guard.h b/scripts/maint/checkspace_tests/good_guard.h
new file mode 100644
index 0000000000..b792912d90
--- /dev/null
+++ b/scripts/maint/checkspace_tests/good_guard.h
@@ -0,0 +1,6 @@
+#ifndef GUARD_MACRO_H
+#define GUARD_MACRO_H
+
+int bar(void);
+
+#endif
diff --git a/scripts/maint/checkspace_tests/same_guard.h b/scripts/maint/checkspace_tests/same_guard.h
new file mode 100644
index 0000000000..b792912d90
--- /dev/null
+++ b/scripts/maint/checkspace_tests/same_guard.h
@@ -0,0 +1,6 @@
+#ifndef GUARD_MACRO_H
+#define GUARD_MACRO_H
+
+int bar(void);
+
+#endif
diff --git a/scripts/maint/checkspace_tests/subdir/dubious.c b/scripts/maint/checkspace_tests/subdir/dubious.c
new file mode 100644
index 0000000000..7f22bf79bf
--- /dev/null
+++ b/scripts/maint/checkspace_tests/subdir/dubious.c
@@ -0,0 +1 @@
+// Nothing wrong with this file, but the name is a duplicate.
diff --git a/src/test/include.am b/src/test/include.am
index 6697dbb171..94352c8644 100644
--- a/src/test/include.am
+++ b/src/test/include.am
@@ -48,6 +48,11 @@ TESTSCRIPTS += src/test/test_rebind.sh
 endif
 endif
 
+if USE_PERL
+TESTSCRIPTS += \
+        scripts/maint/checkSpaceTest.sh
+endif
+
 TESTS += src/test/test src/test/test-slow src/test/test-memwipe \
 	src/test/test_workqueue \
 	src/test/test_keygen.sh \