Re: PATCH: Crash bug on garbage input (previously reported to Debian)

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Mon, 16 Feb 2015 12:57:49 +0000
Peter Stephenson <p.stephenson@xxxxxxxxxxx> wrote:
> % print $((echo foo
> mathsubst> ); echo bar)
> Attempt to inungetc() at start of input.
> zsh: Garbled input at \n (binary file as commands?)
> zsh: parse error near `)'
>
> ...we somehow need to
> detect we're doing the tentative looking for math mode and if so add the
> new line as a continuation of the old one --- that's allowed by the
> input mechanism and I don't see why that wouldn't work here (largely
> because I haven't tried it yet).  So it "only" remains working out where
> to detect this.
> 
> This will need a test, too.

The current continuation / push expansion behaviour doesn't quite do
what we want since it assumes we've finished with previous input.

Here's a simple fix for appending to the input buffer instead of
replacing it for this case.  It's logically general, however I didn't
bother to optimise it since it's special to this particular very rare
case.  I've noted this in the code.  Optimising it now would be
premature --- it would just introduce lots of new and hard to test
special cases.  This version has the virtue of being virtually
independent of anything else going on.

I've add the "parse test from hell" where the first line looks like an
arithmetic substitution but it actually turns out to be a case statement
with unbalanced parentheses that needs the new parsing behaviour.

The original error now gives something that looks a bit more rational:

% ${($((()$[(s:
braceparam mathsubst mathsubst> this ends up in the history)}})
zsh: parse error near `$[(s:'
zsh: closing brace expected

The newly added error is probably sensible for robustness, though,
although it's also likely to be a sign that something else is broken.

pws

diff --git a/Src/input.c b/Src/input.c
index 9520fdd..f919e57 100644
--- a/Src/input.c
+++ b/Src/input.c
@@ -330,8 +330,37 @@ inputline(void)
 	}
     }
     isfirstch = 1;
-    /* Put this into the input channel. */
-    inputsetline(ingetcline, INP_FREE);
+    if ((inbufflags & INP_APPEND) && inbuf) {
+	/*
+	 * We need new input but need to be able to back up
+	 * over the old input, so append this line.
+	 * Pushing the line onto the stack doesn't have the right
+	 * effect.
+	 *
+	 * This is quite a simple and inefficient fix, but currently
+	 * we only need it when backing up over a multi-line $((...
+	 * that turned out to be a command substitution rather than
+	 * a math substitution, which is a very special case.
+	 * So it's not worth rewriting.
+	 */
+	char *oinbuf = inbuf;
+	int newlen = strlen(ingetcline);
+	int oldlen = (int)(inbufptr - inbuf) + inbufleft;
+	if (inbufflags & INP_FREE) {
+	    inbuf = realloc(inbuf, oldlen + newlen + 1);
+	    inbufptr += inbuf - oinbuf;
+	    strcpy(inbuf + oldlen, ingetcline);
+	} else {
+	    /* Paranoia: don't think this is used */
+	    DPUTS(1, "Appending to unallocated input line.");
+	}
+	inbufleft += newlen;
+	inbufct += newlen;
+	inbufflags |= INP_FREE;
+    } else {
+	/* Put this into the input channel. */
+	inputsetline(ingetcline, INP_FREE);
+    }
 
     return 0;
 }
diff --git a/Src/lex.c b/Src/lex.c
index 91628d4..0068485 100644
--- a/Src/lex.c
+++ b/Src/lex.c
@@ -483,9 +483,13 @@ cmd_or_math(int cs_type)
 {
     int oldlen = lexbuf.len;
     int c;
+    int oinflags = inbufflags;
 
     cmdpush(cs_type);
+    inbufflags |= INP_APPEND;
     c = dquote_parse(')', 0);
+    if (!(oinflags & INP_APPEND))
+	inbufflags &= ~INP_APPEND;
     cmdpop();
     *lexbuf.ptr = '\0';
     if (!c) {
diff --git a/Src/zsh.h b/Src/zsh.h
index 94e9ffc..dd946d2 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -410,6 +410,7 @@ enum {
 #define INP_CONT      (1<<3)	/* continue onto previously stacked input  */
 #define INP_ALCONT    (1<<4)	/* stack is continued from alias expn.     */
 #define INP_LINENO    (1<<5)    /* update line number                      */
+#define INP_APPEND    (1<<6)    /* Append new lines to allow backup        */
 
 /* Flags for metafy */
 #define META_REALLOC	0
diff --git a/Test/C01arith.ztst b/Test/C01arith.ztst
index ea87af2..09c0822 100644
--- a/Test/C01arith.ztst
+++ b/Test/C01arith.ztst
@@ -318,3 +318,38 @@
 # 0.75 is exactly representable, don't expect rounding error.
 >0
 >0.75
+
+  # The following tests for a bug that only happens when
+  # backing up over input read a line at a time, so we'll
+  # read the input from stdin.
+  $ZTST_testdir/../Src/zsh -f <<<'
+  print $((echo first command
+  ); echo second command)
+  print third command
+  '
+0:Backing up a line of input when finding out it's not arithmetic
+>first command second command
+>third command
+
+  $ZTST_testdir/../Src/zsh -f <<<'
+  print $((3 +
+  4))
+  print next line
+  '
+0:Not needing to back up a line when reading multiline arithmetic
+>7
+>next line
+
+  $ZTST_testdir/../Src/zsh -f <<<'
+  print $((case foo in
+  bar)
+  echo not this no, no
+  ;;
+  foo)
+  echo yes, this one
+  ;;
+  esac)
+  print after case in subshell)
+  '
+0:Non-arithmetic subst with command subsitution parse from hell
+>yes, this one after case in subshell