Re: potential bash-specific bug in `tests/defs.in'

[Ralf Wildenhues]

Hello Stefano,

thanks for the bug report.

* Stefano Lattarini wrote on Fri, Aug 21, 2009 at 01:34:58AM CEST:
> In these days, I've been playing with the automake source code
> (just to "instruct" myself by seeing how a real-word, working
> sotfware is done),

Depending on what your software aims to do, the Automake package may not
be a great example: it doesn't build any compiled code, for example, and
as such can avoid a lot of the associated complexity.

Maybe notable is the ratio of test suite code to the rest of the code
(both are roughly of the same size, give or take counting of comments
and copyright headers and so on).

> In fact, if I create a dummy testcase script with the `set -e' flag
> active and containing a syntax error, and run it using the bash shell
> (either version 3.2 or 4.0), the test case succeeds, since a `0'
> exit status is erroneusly passed to the code in the exit trap.
> 
> This bug shouldn't manifest itself frequently (after all, how often
> does someone write a testcases containing an unseen syntax
> error?), but can be potentially harmful, making a broken test
> to be considered as passed.

Ouch, that's not good.  Have you reported this with the bash maintainers
yet (bug-bash at gnu.org) or is it already fixed in a newer release or
patch version?  Otherwise, the first step is to go there.  This really
needs to be fixed upstream.  FWIW, a small reproducible example would be

  #! /bin/sh
  trap 'exit $?' 0
  set -e
  fi

and another similar one (that may however require different treatment)
would be

  #! /bin/sh
  trap '' 0
  set -e
  fi

> I'm aware that, strictly speaking, this is a bug of bash, but
> since the automake testsuite try to use "portable shell code"
> only, I think it would be good if it works around that bug,
> as it does with bugs and limitations of other shells.

First off, I typically do run changed or new tests at least once in
verbose mode before committing, inspecting all output, which should
catch the worst blunders of this kind at least for the shell tested.
If you've found any missed ones, please report them.

Possible workarounds for this issue:

1) enhance our "whether /bin/sh has working 'set -e' with exit trap"
configure test to disable -e for bash (which would then fail to catch
quite a few possible error cases),
2) do nothing (and hope that syntax errors will be found when testing
with other shells,
3) use a patch like below as a workaround, which requires adding 'Exit
0' at the end of all tests.

Currently, I still have a slight preference for (2), but may go for (3)
in the end.

Thanks,
Ralf

The change would consist of the patch at the end, plus some scripting
similar to this, and fixups to remove some unneeded code now, and fix
some tests that do not use 'set -e' but relied on the last command in
the test succeeding or failing.

cd tests
sed -si '
  ${
    s/^ *: *$/Exit 0/
    /\<Exit\>/!a\
Exit 0
  }' *.test


--- a/tests/defs.in
+++ b/tests/defs.in
@@ -238,13 +238,19 @@ esac
 # So use `Exit STATUS' instead of `exit STATUS' inside of the tests.
 # Turn off errexit here so that we don't trip the bug with OSF1/Tru64
 # sh inside this function.
+# bash 4.0 does not set $? correctly with a syntax error, when 'set -e'
+# is in effect; so also keep a global variable indicating whether we
+# exit successfully.  This requires all tests to use Exit to actually exit.
 Exit ()
 {
   set +e
+  exit_default=$1
   (exit $1)
   exit $1
 }
 
+exit_default=1
+
 curdir=`pwd`
 testSubDir=$me.dir
 test ! -d $testSubDir || {
@@ -266,6 +272,9 @@ if test "$sh_errexit_works" = yes; then
     esac
     test "$signal" != 0 &&
       echo "$as_me: caught signal $signal"
+    if test $exit_default -ne 0 && test $exit_status -eq 0; then
+      exit_status=$exit_default
+    fi
     echo "$as_me: exit $exit_status"
     exit $exit_status
   ' 0