[FYI] {master, test-protocols} problems with TEST_SUITE_LOG redefinitions

[Stefano Lattarini]

Hello automakers.

While working on the 'test-protocols' branch, I issued a command like:
 $ gmake check TESTS=... TEST_SUITE_LOG=foo.log
where a test in $(TESTS) ran something like "$MAKE -e TESTS=foo.test".
And I almost freezed my machine -- ouch!  This was due to a genuine
regression in the 'test-protocol' branch, that I address here.

The first attached patch (for 'master') works around the issue in the
Automake's testsuite, by making `TEST_SUITE_LOG' unset by default in
the test scripts (and that is a good change to have regardless of the
issue in question here).  The second patch (again for 'master') adds a
test case exposing the issue; the test passes on 'master' and fails on
'test-protocols' with Solaris and GNU make (but not with BSD makes, that
apparently are smart enough to recognize and diagnose the problem by
themselves).  Finally, the third patch (for 'test-protocols') fixes the
implementation of the testsuite harness to handle the issue correctly.

Regards,
  Stefano

From 89dca9e1858f3734c651a2e49134665dcf8fa606 Mon Sep 17 00:00:00 2001
Message-Id: <address@hidden>
From: Stefano Lattarini <address@hidden>
Date: Wed, 10 Aug 2011 10:44:56 +0200
Subject: [PATCH 1/2] test defs: yet more environment cleanup

* tests/defs: Also unset the TEST_SUITE_LOG variable.
---
 ChangeLog  |    5 +++++
 tests/defs |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 797b268..e08b7c9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2011-08-09  Stefano Lattarini  <address@hidden>
 
+       test defs: yet more environment cleanup
+       * tests/defs: Also unset the TEST_SUITE_LOG variable.
+
+2011-08-09  Stefano Lattarini  <address@hidden>
+
        tests: fix various blunders in 'suffix-chain.test'
        * tests/suffix-chain.test: Fix various blunders that were causing
        this test to fail spuriously: append to `configure.in', not to
diff --git a/tests/defs b/tests/defs
index dba01fb..fe73d85 100644
--- a/tests/defs
+++ b/tests/defs
@@ -98,6 +98,7 @@ unset AM_COLOR_TESTS
 unset TESTS
 unset TEST_LOG_COMPILER
 unset TEST_LOGS
+unset TEST_SUITE_LOG
 unset RECHECK_LOGS
 unset VERBOSE
 
-- 
1.7.2.3

From 1ebdc2c1b9a75420dd7518116a1e70b1ad6d93fc Mon Sep 17 00:00:00 2001
Message-Id: <address@hidden>
In-Reply-To: <address@hidden>
References: <address@hidden>
From: Stefano Lattarini <address@hidden>
Date: Wed, 10 Aug 2011 16:03:35 +0200
Subject: [PATCH 2/2] coverage: possible infinite recursion in the test harness

Motivated by a regression in the 'test-protocols' branch.

* tests/parallel-tests-fork-bomb.test: New test, checking that
if $(TEST_SUITE_LOG) is in $(TEST_LOGS), we obtain a diagnosed
error rather than a make hang or a fork bomb.
* tests/Makefile.am (TESTS): Update.
---
 ChangeLog                           |    9 ++
 tests/Makefile.am                   |    1 +
 tests/Makefile.in                   |    1 +
 tests/parallel-tests-fork-bomb.test |  142 +++++++++++++++++++++++++++++++++++
 4 files changed, 153 insertions(+), 0 deletions(-)
 create mode 100755 tests/parallel-tests-fork-bomb.test

diff --git a/ChangeLog b/ChangeLog
index e08b7c9..2d7e5d6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,14 @@
 2011-08-09  Stefano Lattarini  <address@hidden>
 
+       coverage: possible infinite recursion in the test harness
+       Motivated by a regression in the 'test-protocols' branch.
+       * tests/parallel-tests-fork-bomb.test: New test, checking that
+       if $(TEST_SUITE_LOG) is in $(TEST_LOGS), we obtain a diagnosed
+       error rather than a make hang or a fork bomb.
+       * tests/Makefile.am (TESTS): Update.
+
+2011-08-09  Stefano Lattarini  <address@hidden>
+
        test defs: yet more environment cleanup
        * tests/defs: Also unset the TEST_SUITE_LOG variable.
 
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 82fb906..833dc7a 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -733,6 +733,7 @@ parallel-tests-log-override-2.test \
 parallel-tests-log-override-recheck.test \
 parallel-tests-cmdline-override.test \
 parallel-tests-log-compiler-example.test \
+parallel-tests-fork-bomb.test \
 parse.test \
 percent.test \
 percent2.test \
diff --git a/tests/Makefile.in b/tests/Makefile.in
index d6d418c..ede80e6 100644
--- a/tests/Makefile.in
+++ b/tests/Makefile.in
@@ -1006,6 +1006,7 @@ parallel-tests-log-override-2.test \
 parallel-tests-log-override-recheck.test \
 parallel-tests-cmdline-override.test \
 parallel-tests-log-compiler-example.test \
+parallel-tests-fork-bomb.test \
 parse.test \
 percent.test \
 percent2.test \
diff --git a/tests/parallel-tests-fork-bomb.test 
b/tests/parallel-tests-fork-bomb.test
new file mode 100755
index 0000000..6a385d6
--- /dev/null
+++ b/tests/parallel-tests-fork-bomb.test
@@ -0,0 +1,142 @@
+#! /bin/sh
+# Copyright (C) 2011 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2, or (at your option)
+# any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check parallel-tests features:
+#  - If $(TEST_SUITE_LOG) is in $(TEST_LOGS), we get a diagnosed
+#    error, not a make hang or a system freeze.
+
+parallel_tests=yes
+. ./defs || Exit 1
+
+# The tricky part of this test is to avoid that make hangs or even
+# freezes the system in case infinite recursion (which is the bug we
+# are testing against) is encountered.  The following hacky makefile
+# should minimize the probability of that happening.
+cat > Makefile.am << 'END'
+TEST_LOG_COMPILER = true
+TESTS =
+
+errmsg = ::OOPS:: Recursion too deep
+
+if IS_GNU_MAKE
+
+ is_too_deep := $(shell test $(MAKELEVEL) -lt 10 && echo no)
+
+## Indenteation here required to avoid confusing Automake.
+ ifeq ($(is_too_deep),no)
+ else
+ $(error $(errmsg), $(MAKELEVEL) levels)
+ endif
+
+else !IS_GNU_MAKE
+
+# We use mkdir to detect the level of recursion, since it is easy
+# to use and assured to be portably atomical.  Also use an higher
+# number than with GNU make above, since the level used here can
+# be incremented by tow or more per recursion.
+recursion-not-too-deep:
+       @ok=no; \
+       for i in 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 \
+                18 19 20 21 22 23 24 25 26 27 28 29; \
+       do \
+         echo " mkdir rec-$$i.d"; \
+         if mkdir rec-$$i.d; then \
+           ok=yes; break; \
+         else :; fi; \
+       done; \
+       test $$ok = yes || { echo '$(errmsg)' >&2; exit 1; }
+.PHONY: recursion-not-too-deep
+clean-local:
+       rmdir rec-[0-9].d
+
+targets = all check recheck $(TESTS) $(TEST_LOGS) $(TEST_SUITE_LOG)
+$(targets): recursion-not-too-deep
+
+# For BSD make.
+.BEGIN: recursion-not-too-deep
+
+endif !IS_GNU_MAKE
+END
+
+if using_gmake; then
+  cond=:
+else
+  cond=false
+fi
+
+cat >> configure.in << END
+AM_CONDITIONAL([IS_GNU_MAKE], [$cond])
+AC_OUTPUT
+END
+
+# Another helpful idiom to avoid hanging on capable systems.  The subshell
+# is needed since `ulimit' might be a special shell builtin.
+if (ulimit -t 8); then ulimit -t 8; fi
+
+$ACLOCAL
+$AUTOCONF
+$AUTOMAKE -a -Wno-portability
+
+./configure
+
+do_check ()
+{
+  st=0
+  log=$1; shift
+  env "$@" $MAKE -e check >output 2>&1 || st=$?
+  cat output
+  $FGREP '::OOPS::' output && Exit 1 # Possible infinite recursion.
+  # Check that at least we don't create a botched global log file.
+# FIXME: GNU make currently creates it!
+#  test ! -f "$log"
+    # Look for possible error messages about circular dependencies from
+    # either make or our own recipes.  At least one such a message must
+    # be present.
+    err_seen=no
+    for err_rx in \
+      'circular.* depend' \
+      'depend.* circular' \
+      'graph cycle' \
+      'infinite (loop|recursion)' \
+      'depend.* on itself' \
+    ; do
+      $EGREP -i "$err_rx" output | $FGREP "$log" || continue
+      err_seen=yes
+      break
+    done
+    test $err_seen = yes || Exit 1
+    # Some make implementations (e.g., NetBSD's), while smartly detecting
+    # the circular dependency early and diagnosing it, still exit with a
+    # successful exit status (yikes!).  Relax our checks not to fail in
+    # this case.
+    using_gmake && { test $st -gt 0 || Exit 1; }
+    :
+}
+
+: > test-suite.test
+do_check test-suite.log TESTS=test-suite.test
+rm -f *.log *.test
+
+: > 0.test
+: > 1.test
+: > 2.test
+: > 3.test
+: > foobar.test
+do_check foobar.log TEST_LOGS='0.log 1.log foobar.log 2.log 3.log' \
+                    TEST_SUITE_LOG=foobar.log
+rm -f *.log *.test
+
+:
-- 
1.7.2.3

From ab5069b06010a7c980513d15bd0a2cdebeb95c3d Mon Sep 17 00:00:00 2001
Message-Id: <address@hidden>
From: Stefano Lattarini <address@hidden>
Date: Wed, 10 Aug 2011 15:37:27 +0200
Subject: [PATCH] test harness: avoid possible fork bomb

This fixes a regression w.r.t. the master branch, exposed by
test 'parallel-tests-fork-bomb.test'.

* lib/am/check.am (am--redo-logs): Detect possible infinite
recursion due to a test log in $(TEST_LOGS) being the same
as $(TEST_SUITE_LOG).
* tests/parallel-tests-fork-bomb.test: Enhance and extend a
little now that we have more explicit semantics.
---
 ChangeLog                      |   11 +++++++++++
 lib/Automake/tests/Makefile.in |    7 ++++++-
 lib/am/check.am                |   11 ++++++++++-
 tests/Makefile.in              |    7 ++++++-
 4 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 76e1b3d..4e50d37 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
 2011-08-09  Stefano Lattarini  <address@hidden>
 
+       test harness: avoid possible fork bomb
+       This fixes a regression w.r.t. the master branch, exposed by
+       test 'parallel-tests-fork-bomb.test'.
+       * lib/am/check.am (am--redo-logs): Detect possible infinite
+       recursion due to a test log in $(TEST_LOGS) being the same
+       as $(TEST_SUITE_LOG).
+       * tests/parallel-tests-fork-bomb.test: Enhance and extend a
+       little now that we have more explicit semantics.
+
+2011-08-09  Stefano Lattarini  <address@hidden>
+
        coverage: possible infinite recursion in the test harness
        Motivated by a regression in the 'test-protocols' branch.
        * tests/parallel-tests-fork-bomb.test: New test, checking that
diff --git a/lib/Automake/tests/Makefile.in b/lib/Automake/tests/Makefile.in
index c898f49..aca691c 100644
--- a/lib/Automake/tests/Makefile.in
+++ b/lib/Automake/tests/Makefile.in
@@ -352,7 +352,12 @@ cscope cscopelist:
 am--redo-logs:
        @rm -f $$redo_logs
        @rm -f $$redo_results
-       @$(MAKE) $(AM_MAKEFLAGS) $$redo_logs
+       @if test -n "$$am__remaking_logs"; then \
+         echo "fatal: making $(TEST_SUITE_LOG): possible infinite" \
+              "recursion detected" >&2; \
+       else \
+         am__remaking_logs=yes $(MAKE) $(AM_MAKEFLAGS) $$redo_logs; \
+       fi;
        @st=0;  \
        errmsg="fatal: making $(TEST_SUITE_LOG): failed to create"; \
        for i in $$redo_bases; do \
diff --git a/lib/am/check.am b/lib/am/check.am
index 76c5273..50c5a18 100644
--- a/lib/am/check.am
+++ b/lib/am/check.am
@@ -132,7 +132,16 @@ am__set_TESTS_bases = \
 am--redo-logs:
        @rm -f $$redo_logs
        @rm -f $$redo_results
-       @$(MAKE) $(AM_MAKEFLAGS) $$redo_logs
+## The use of the `am__remaking_logs' environment variable below is
+## required to ensure we don't go into infinite recursion in case a
+## test log in$(TEST_LOGS) is the same as $(TEST_SUITE_LOG).  Yes,
+## this has already happened in practice.  Sigh!
+       @if test -n "$$am__remaking_logs"; then \
+         echo "fatal: making $(TEST_SUITE_LOG): possible infinite" \
+              "recursion detected" >&2; \
+       else \
+         am__remaking_logs=yes $(MAKE) $(AM_MAKEFLAGS) $$redo_logs; \
+       fi;
 ## Sanity check: each unreadable or non-existent test result file should
 ## has been properly remade at this point, as should the corresponding log
 ## file.
diff --git a/tests/Makefile.in b/tests/Makefile.in
index 1688557..9e03806 100644
--- a/tests/Makefile.in
+++ b/tests/Makefile.in
@@ -1508,7 +1508,12 @@ cscope cscopelist:
 am--redo-logs:
        @rm -f $$redo_logs
        @rm -f $$redo_results
-       @$(MAKE) $(AM_MAKEFLAGS) $$redo_logs
+       @if test -n "$$am__remaking_logs"; then \
+         echo "fatal: making $(TEST_SUITE_LOG): possible infinite" \
+              "recursion detected" >&2; \
+       else \
+         am__remaking_logs=yes $(MAKE) $(AM_MAKEFLAGS) $$redo_logs; \
+       fi;
        @st=0;  \
        errmsg="fatal: making $(TEST_SUITE_LOG): failed to create"; \
        for i in $$redo_bases; do \
-- 
1.7.2.3