Re: Segfault on =() expansion

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

On Sun, 31 Aug 2008 10:39:35 +0200
"Mikael Magnusson" <mikachu@xxxxxxxxx> wrote:
> I could reproduce this on all my remote ssh shells, ranging from 4.2.3
> to my local current cvs.
> 
> % a==(echo foo); : & cat $a
> [1] 14645
> foo
> %
> [1]  + done       :
> % a==(echo foo); : & cat $a
> zsh: segmentation fault  zsh -f

This was quite involved (yes, I know, just for once...)  Working backwards,

1. In the second assignment, the list node's data was NULL.  This shouldn't
   happen, so caused the crash when processing it as a string.
2. The data was NULL because =(...) expansion failed.
3. That failed because "thisjob" was -1.
4. thisjob was -1 for two reasons.
  a. It didn't get set for the current job because the current command was
     parsed as "simple", so it went through execsimple(), missing out a
     lot of steps including all job handling.
  b. In this case, the execution of the background job had reset thisjob
     to -1.  That's why it didn't happen the first time.
5. The assignment was parsed as simple because all assignments are.
   "Simple" means everything happens entirely in the shell and won't
   use job handling:  this isn't true here because process substitutions
   need to have a job to attach the temporary file to.

The fixes are:
to 5: assignments with process substitutions can't be simple.
to 2: on failure of process substitutions (and others) set the data to
an empty string.

(I wonder, actually, if 4b. needs unfixing, i.e. thisjob should be made to
default to -1.)

This will cause the "cat" in the test to give an error.  This is the
correct behaviour: the file generated by the =(...) substition is a
temporary file (this is clearly documented) that only lasts for the
length of the current command, which is just the assignment.  So
actually process substitutions aren't useful for assignments, which is
probably why this hasn't shown up before.

I thought about being safe and detecting $(...)'s, too, but that caused this:

  myfd=99
  (echo >&$myfd) 2>msg
  bad_fd_msg="${$(<msg)##*:}"

to return a non-zero status.  That's because when assignments are
handled by execcmd() rather than execsimple() we don't set the status
for the command, so it retains the previous status.  I've fixed that,
but in the end I decided just to restrict the fix to 5. to process
substitution since we'd know by now if it was necessary for command
substitution.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.144
diff -u -r1.144 exec.c
--- Src/exec.c	31 Aug 2008 16:01:11 -0000	1.144
+++ Src/exec.c	1 Sep 2008 19:58:47 -0000
@@ -2220,6 +2220,7 @@
     doneps4 = 0;
     redir = (wc_code(*state->pc) == WC_REDIR ? ecgetredirs(state) : NULL);
     if (wc_code(*state->pc) == WC_ASSIGN) {
+	cmdoutval = 0;
 	varspc = state->pc;
 	while (wc_code((code = *state->pc)) == WC_ASSIGN)
 	    state->pc += (WC_ASSIGN_TYPE(code) == WC_ASSIGN_SCALAR ?
@@ -2236,6 +2237,12 @@
      * they don't modify their argument strings. */
     args = (type == WC_SIMPLE ?
 	    ecgetlist(state, WC_SIMPLE_ARGC(code), EC_DUP, &htok) : NULL);
+    /*
+     * If assignment but no command get the status from variable
+     * assignment.
+     */
+    if (!args && varspc)
+	lastval = errflag ? errflag : cmdoutval;
 
     for (i = 0; i < 10; i++) {
 	save[i] = -2;
Index: Src/parse.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/parse.c,v
retrieving revision 1.72
diff -u -r1.72 parse.c
--- Src/parse.c	31 Aug 2008 19:50:49 -0000	1.72
+++ Src/parse.c	1 Sep 2008 19:58:48 -0000
@@ -1567,6 +1567,18 @@
 		str = p + 1;
 	    } else
 		equalsplit(tokstr, &str);
+	    for (p = str; *p; p++) {
+		/*
+		 * We can't treat this as "simple" if it contains
+		 * expansions that require process subsitution, since then
+		 * we need process handling.
+		 */
+		if (p[1] == Inpar &&
+		    (*p == Equals || *p == Inang || *p == Outang)) {
+		    *complex = 1;
+		    break;
+		}
+	    }
 	    ecstr(name);
 	    ecstr(str);
 	    isnull = 0;
Index: Src/subst.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/subst.c,v
retrieving revision 1.85
diff -u -r1.85 subst.c
--- Src/subst.c	12 May 2008 13:50:42 -0000	1.85
+++ Src/subst.c	1 Sep 2008 19:58:49 -0000
@@ -66,6 +66,7 @@
 	    else
 		setdata(node, (void *) getoutputfile(str));	/* =(...) */
 	    if (!getdata(node)) {
+		setdata(node, dupstring(""));
 		unqueue_signals();
 		return;
 	    }


-- 
Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/