Here-documents borked in "functions" output

Here-documents borked in "functions" output

[Bart Schaefer]

The zkbd function appears to show a number of interesting (new?) bugs in
the shell.  For example:

% autoload +X zkbd
% functions zkbd > zkbd.new
% source zkbd.new
zkbd.new:103: parse error near `\n'

This looks to be because quoting has been lost from here-documents.  Here's
a simpler example (the blank line at the start of the here-document is
important):

hello() { cat <<EOF
Anything can be here
Hi there $LOGNAME
EOF
}

Becomes this:

schaefer<544> functions hello
hello () {
        cat <<< Anything can be here
Hi there $LOGNAME
}

Zsh correctly figures out that it can't use single quotes around the
here-document content, but it doesn't replace them with anything else.
Note that the function executes correctly, it just isn't regurgitated
correctly with "functions".

-- 

Re: Here-documents borked in "functions" output

[Peter Stephenson]

On Thu, 13 Mar 2008 19:52:41 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> hello() { cat <<EOF
> Anything can be here
> Hi there $LOGNAME
> EOF
> }
> 
> Becomes this:
> 
> schaefer<544> functions hello
> hello () {
>         cat <<< Anything can be here
> Hi there $LOGNAME
> }

There has always been a bit of a hack here, so it's not surprising things
go a little wonky.  We turn HERE-documents into HERE-strings immediately;
the code in text.c that re-presents them doesn't currently know whether the
code came from a HERE-document, in which case it needs extra quoting, for
which it simply guesses.  The sensible fix is to tell it.

There are two stages involved: parsing, where we just need a bit flag
for the new state, and execution, where the redirection is expanded
out at the last minute to make it easier to handle.  For the second case it
seems sensible to add an extra flags field.  That means the code in exec.c
doesn't have to change at all.

The new tests should catch most likely glitches.

Index: Src/parse.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/parse.c,v
retrieving revision 1.68
diff -u -r1.68 parse.c
--- Src/parse.c	10 Jan 2008 18:53:50 -0000	1.68
+++ Src/parse.c	14 Mar 2008 11:32:21 -0000
@@ -1861,7 +1861,7 @@
 void
 setheredoc(int pc, int type, char *str)
 {
-    ecbuf[pc] = WCB_REDIR(type);
+    ecbuf[pc] = WCB_REDIR(type | REDIR_FROM_HEREDOC_MASK);
     ecbuf[pc + 2] = ecstrcode(str);
 }
 
@@ -2409,6 +2409,10 @@
 	r->type = WC_REDIR_TYPE(code);
 	r->fd1 = *s->pc++;
 	r->name = ecgetstr(s, EC_DUP, NULL);
+	if (WC_REDIR_FROM_HEREDOC(code))
+	    r->flags = REDIRF_FROM_HEREDOC;
+	else
+	    r->flags = 0;
 	if (WC_REDIR_VARID(code))
 	    r->varid = ecgetstr(s, EC_DUP, NULL);
 	else
Index: Src/text.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/text.c,v
retrieving revision 1.21
diff -u -r1.21 text.c
--- Src/text.c	4 Jan 2008 14:45:40 -0000	1.21
+++ Src/text.c	14 Mar 2008 11:32:22 -0000
@@ -831,17 +831,22 @@
 	    taddstr(fstr[f->type]);
 	    if (f->type != REDIR_MERGEIN && f->type != REDIR_MERGEOUT)
 		taddchr(' ');
-	    if (f->type == REDIR_HERESTR && !has_token(f->name)) {
+	    if (f->type == REDIR_HERESTR &&
+		(f->flags & REDIRF_FROM_HEREDOC)) {
 		/*
 		 * Strings that came from here-documents are converted
 		 * to here strings without quotation, so add that
-		 * now.  If tokens are already present taddstr()
-		 * will do the right thing (anyway, adding more
-		 * quotes certainly isn't right in that case).
+		 * now.  If tokens are present we need to do double quoting.
 		 */
-		taddchr('\'');
-		taddstr(quotestring(f->name, NULL, QT_SINGLE));
-		taddchr('\'');
+		if (!has_token(f->name)) {
+		    taddchr('\'');
+		    taddstr(quotestring(f->name, NULL, QT_SINGLE));
+		    taddchr('\'');
+		} else {
+		    taddchr('"');
+		    taddstr(quotestring(f->name, NULL, QT_DOUBLE));
+		    taddchr('"');
+		}
 	    } else
 		taddstr(f->name);
 	    taddchr(' ');
Index: Src/zsh.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/zsh.h,v
retrieving revision 1.118
diff -u -r1.118 zsh.h
--- Src/zsh.h	16 Dec 2007 14:05:16 -0000	1.118
+++ Src/zsh.h	14 Mar 2008 11:32:22 -0000
@@ -309,7 +309,10 @@
     REDIR_OUTPIPE		/* > >(...) */
 };
 #define REDIR_TYPE_MASK	(0x1f)
+/* Redir using {var} syntax */
 #define REDIR_VARID_MASK (0x20)
+/* Mark here-string that came from a here-document */
+#define REDIR_FROM_HEREDOC_MASK (0x40)
 
 #define IS_WRITE_FILE(X)      ((X)>=REDIR_WRITE && (X)<=REDIR_READWRITE)
 #define IS_APPEND_REDIR(X)    (IS_WRITE_FILE(X) && ((X) & 2))
@@ -550,10 +553,18 @@
 #define CONDDEF(name, flags, handler, min, max, condid) \
     { NULL, name, flags, handler, min, max, condid, NULL }
 
+/* Flags for redirections */
+
+enum {
+    /* Mark a here-string that came from a here-document */
+    REDIRF_FROM_HEREDOC = 1
+};
+
 /* tree element for redirection lists */
 
 struct redir {
     int type;
+    int flags;
     int fd1, fd2;
     char *name;
     char *varid;
@@ -744,6 +755,7 @@
 
 #define WC_REDIR_TYPE(C)    ((int)(wc_data(C) & REDIR_TYPE_MASK))
 #define WC_REDIR_VARID(C)   ((int)(wc_data(C) & REDIR_VARID_MASK))
+#define WC_REDIR_FROM_HEREDOC(C) ((int)(wc_data(C) & REDIR_FROM_HEREDOC_MASK))
 #define WCB_REDIR(T)        wc_bld(WC_REDIR, (T))
 /* Size of redir is 4 words if REDIR_VARID_MASK is set, else 3 */
 #define WC_REDIR_WORDS(C)   (WC_REDIR_VARID(C) ? 4 : 3)
Index: Test/A04redirect.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/A04redirect.ztst,v
retrieving revision 1.12
diff -u -r1.12 A04redirect.ztst
--- Test/A04redirect.ztst	12 Jul 2006 12:02:51 -0000	1.12
+++ Test/A04redirect.ztst	14 Mar 2008 11:32:22 -0000
@@ -82,6 +82,58 @@
 >b
 >c
 
+# The following tests check that output of parsed here-documents works.
+# This isn't completely trivial because we convert the here-documents
+# internally to here-strings.  So we check again that we can output
+# the reevaluated here-strings correctly.  Hence there are three slightly
+# different stages.  We don't care how the output actually looks, so
+# we don't test that.
+  heretest() {
+    print First line
+    cat <<-HERE
+	$foo$foo met celeste  'but with extra'  "stuff to test quoting"
+	HERE
+    print Last line
+  }
+  heretest
+  eval "$(functions heretest)"
+  heretest
+  eval "$(functions heretest)"
+  heretest
+0:Re-evaluation of function output with here document, unquoted
+>First line
+>barbar met celeste  'but with extra'  "stuff to test quoting"
+>Last line
+>First line
+>barbar met celeste  'but with extra'  "stuff to test quoting"
+>Last line
+>First line
+>barbar met celeste  'but with extra'  "stuff to test quoting"
+>Last line
+
+  heretest() {
+    print First line
+    cat <<'    HERE'
+    $foo$foo met celeste  'but with extra'  "stuff to test quoting"
+    HERE
+    print Last line
+  }
+  heretest
+  eval "$(functions heretest)"
+  heretest
+  eval "$(functions heretest)"
+  heretest
+0:Re-evaluation of function output with here document, quoted
+>First line
+>    $foo$foo met celeste  'but with extra'  "stuff to test quoting"
+>Last line
+>First line
+>    $foo$foo met celeste  'but with extra'  "stuff to test quoting"
+>Last line
+>First line
+>    $foo$foo met celeste  'but with extra'  "stuff to test quoting"
+>Last line
+
   #
   # exec tests: perform these in subshells so if they fail the
   # shell won't exit.


-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: Here-documents borked in "functions" output

[Peter Stephenson]

On Fri, 14 Mar 2008 11:36:41 +0000
Peter Stephenson <pws@csr.com> wrote:
> The new tests should catch most likely glitches.

However, there is a glitch in the quoting code; it's
independent of here-documents but there aren't many places
where we do this, so it hasn't shown up before:

% hello() {
  cat <<HERE
  I will `echo stuff` and then `echo more stuff`
  HERE
}
% functions hello
hello () {
        cat <<< "I will `echo stuff`` and then ``echo more stuff```"
}

Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.182
diff -u -r1.182 utils.c
--- Src/utils.c	27 Feb 2008 11:46:26 -0000	1.182
+++ Src/utils.c	14 Mar 2008 11:56:30 -0000
@@ -4268,6 +4268,8 @@
 		while (*u && *u != c)
 		    *v++ = *u++;
 		*v++ = c;
+		if (*u)
+		    u++;
 		continue;
 	    } else if ((*u == Qstring || *u == '$') && u[1] == '\'' &&
 		       instring == QT_DOUBLE) {

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: Here-documents borked in "functions" output

[Bart Schaefer]

On Mar 14, 11:36am, Peter Stephenson wrote:
}
} There has always been a bit of a hack here, so it's not surprising
} things go a little wonky. We turn HERE-documents into HERE-strings
} immediately; the code in text.c that re-presents them doesn't
} currently know whether the code came from a HERE-document, in which
} case it needs extra quoting, for which it simply guesses. The sensible
} fix is to tell it.

This is great, but in case you have some extra time just lying around
waiting to be filled, it might be nicer to store the begin/end marker
string so that it could be re-presented as a here-document again.

Of course, if you give me that cookie I'm going to want leading tabs
with my milk, so maybe it's better if I quietly let it pass.