Re: interrupt causes parse error in configure script

[Ralf Wildenhues]

* Eric Blake wrote on Mon, Aug 18, 2008 at 11:59:22PM CEST:
> Ralf Wildenhues <Ralf.Wildenhues <at> gmx.de> writes:
> 
> > > $ sh -c 'if test `sleep 5; echo hi` = hi ; then echo yes; fi'
> > 
> > and found only pdksh and OpenBSD sh (which is a pdksh descendant) to
> > have this issue.
> 
> Jim's original error message does not look the same as pdksh; it reminds me 
> more of what bash would output.  I could not reproduce with bash 3.2.39, but 
> I 
> know that at least bash 3.2.20 introduced some fixes for poor subshell 
> behavior:

OK, so this issue is about several shells.

> > FWIW I'm still looking into a couple of test failures (one of "AS_IF and
> > AS_CASE" which I don't think this patch caused).
> 
> I know that when testing my m4_transform_pair patch, I was able to trip an 
> arbitrary limit in bash - it refuses to parse more than 2500 nested elif 
> keywords inside an if/else/fi.  I wonder if we are tripping some other shell 
> limits with my test enhancements last week, even though I scaled back the 
> limit 
> to 1000 instead of 2500.  Maybe it was a mistake to add torture tests to the 
> already existing AS_IF test, instead of creating a new test?

Ahh, thanks for this hint.  It is indeed weird that I can trip the
failure of test 50 with './testsuite 59-60' or './testsuite 60' but not
with './testsuite 59-61'. So I guess this messes up some internal shell
data.

Maybe we should relax that test?

> > I suppose the AS_VAR* macros deserve documentation eventually.
> 
> Yes, they are probably stable enough to document, especially if we decide to 
> keep the line in your patch that references them.

We can defer that to after the release though IMVHO.

> > address@hidden can be used to avoid this.
> 
> It might be nice to also show the solution without AS_VAR_IF

Yes.  Even more, until we document AS_VAR_IF, we should only document
the alternative solution.

> Also, I did verify that external commands are also incorrectly run, so
> it is not just the test builtin that suffer from the aborted ``:
[...
> And I guess this means we need to audit autoconf for any other uses of
> `` that are passed directly as arguments to enclosing commands.

Yep.  :-/

> > +# AS_VAR_IF(VARIABLE, [VALUE = yes], IF-TRUE, IF-FALSE)
> 
> Hmm.  All the rewrites in the patch supplied an explicit [yes], rather
> than relying on [] as the second argument.

Yes, I should have commented on this: I found it easier to read the
explicit "yes", and I found an empty value misleading in that the actual
value was expected to be the empty string.

> Theoretically, it might be nice to check that a variable is indeed
> empty, although that could still be done by passing 
> [[]], so it is still safe to let [] default to yes.

Yes, but IMVHO omitting these three characters does not help readability
of the code.  I'm sure not going to remember a year from now.

> > +m4_define([AS_VAR_IF],
> > +[AS_LITERAL_IF([$1],
> > +  [AS_IF([test "x$$1" = x""m4_default([$2], [yes])], [$3], [$4])],
> > +  [as_val=AS_VAR_GET([$1])
> > +   AS_IF([test "x$as_val" = x""m4_default([$2], [yes])], [$3], [$4])])])
> 
> Now we're getting back to Ralf's first complaint, that by adding "", we lose 
> the ability to catch logic bugs when comparing against safe strings (ie. if 
> one 
> of multiple branches failed to set the var, perhaps because of a typo).  I'm 
> wondering if we should make this smarter, by adding some m4 magic to output 
> this:
> 
> test "x$as_val" = x""$2
> 
> when $2 contains any shell metacharacters or a leading character that might 
> mess up test (note that you would thus use AC_VAR_IF([var], ["yes"], ...) if 
> you suspect that $var might be "guessing no"); while generating:
> 
> test $as_val = m4_default([$2], [yes])
> 
> when $2 is [] or purely safe characters.

But you don't know whether $as_val may be validly be "-lm" or empty or
so.  I don't think we can retain the logic bug catching flexibility.

Proposed updated doc patch.  The earlier variant also had too long
lines.

Good night,
Ralf

diff --git a/doc/autoconf.texi b/doc/autoconf.texi
index 2ce88f8..29cc1da 100644
--- a/doc/autoconf.texi
+++ b/doc/autoconf.texi
@@ -13479,6 +13479,25 @@ Shell Substitutions
 - broken differ: char 4, line 1
 @end example
 
+Upon interrupt, some shells may abort a command substitution, replace it
+with a null string, and wrongly evaluate the enclosing command before
+interrupting the script.  This can lead to spurious errors:
+
address@hidden
+$ @kbd{sh -c 'if test `sleep 5; echo hi` = hi; then echo yes; fi'}
+$ @kbd{^C}
+ksh: test: hi: unexpected operator/operand
address@hidden example
+
address@hidden
+You can avoid this by assigning the command substitution to a temporary
+variable:
+
address@hidden
+$ @kbd{sh -c 'res=`sleep 5; echo hi`
+         if test "x$res" = xhi; then echo yes; fi'}
+$ @kbd{^C}
address@hidden example
 
 @item $(@var{commands})
 @cindex $(@var{commands})