Segfault on =() expansion

Segfault on =() expansion

[Mikael Magnusson]

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

Let's see if I can get a backtrace... *waits for recompile*.

Program received signal SIGSEGV, Segmentation fault.
0x0806d364 in untokenize (s=0x0) at exec.c:1639
1639	    if (*s) {
(gdb) bt full
#0  0x0806d364 in untokenize (s=0x0) at exec.c:1639
No locals.
#1  0x0806e224 in addvars (state=0xafc54c5c, pc=0xa7bd35e0,
addflags=0) at exec.c:2068
	pm = (Param) 0x80ac572
	val = 0xc <Address 0xc out of bounds>
	allexp = 0
	myflags = 0
	vl = (LinkList) 0xafc54b34
	xtr = 0
	isstr = 1
	htok = 1
	arr = (char **) 0xafc54bb0
	ptr = (char **) 0x0
	name = 0xa7bd3660 "a"
	flags = 0
	opc = (Wordcode) 0xa7bd35e4
	ac = 5
	svl = {list = {first = 0xafc54b28, last = 0xafc54b28, flags = 0}, node = {
    next = 0xafc54b28, prev = 0xafc54b28, dat = 0x0}}
	__n0 = {next = 0x0, prev = 0xafc54b34, dat = 0x0}
#2  0x0806b7c1 in execsimple (state=0xafc54c5c) at exec.c:992
	code = 5
	lv = 1
#3  0x0806ba68 in execlist (state=0xafc54c5c, dont_change_job=0,
exiting=0) at exec.c:1094
	donedebug = 0
	next = (Wordcode) 0xa7bd35f0
	code = 11329
	ret = 1103269876
	cj = -1
---Type <return> to continue, or q <return> to quit---
	csp = 0
	ltype = 354
	old_pline_level = 0
	old_list_pipe = 0
	oldlineno = 4
	oldnoerrexit = 0
	donetrap = 0
#4  0x0806b6f5 in execode (p=0xa7bd35b0, dont_change_job=0, exiting=0)
at exec.c:965
	s = {prog = 0xa7bd35b0, pc = 0xa7bd35ec, strs = 0xa7bd3620
"\213\210echo foo\211"}
#5  0x08084a7c in loop (toplevel=1, justonce=0) at init.c:181
	toksav = 1
	prog = (Eprog) 0xa7bd35b0
#6  0x080874e2 in zsh_main (argc=2, argv=0xafc54d94) at init.c:1407
	t = (char **) 0xafc54d9c
	t0 = 157
#7  0x080551f6 in main (argc=Cannot access memory at address 0x48
) at ./main.c:93
No locals.



Changing (*s) to (s && *s) fixes the crash, but maybe something else is fishy.
With that change, $a appears to be empty instead of the crash.

% a==(echo foo); : & echo $a
[1] 18053
/tmp/zsh5NP27e
%
[1]  + done       :
% a==(echo foo); : & echo $a
[1] 18054

%
[1]  + done       :


-- 
Mikael Magnusson

Re: Segfault on =() expansion

[Peter Stephenson]

On Sun, 31 Aug 2008 10:39:35 +0200
"Mikael Magnusson" <mikachu@gmail.com> 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@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: Segfault on =() expansion

[Mikael Magnusson]

2008/9/1 Peter Stephenson <p.w.stephenson@ntlworld.com>:
> On Sun, 31 Aug 2008 10:39:35 +0200
> "Mikael Magnusson" <mikachu@gmail.com> 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,

I am somehow not overly surprised by this :).

> 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.

Right, that's what i wanted to confirm just seconds before finding the bug,
I was quite surprised to find the temp file hadn't been deleted, though in
my case it was sort of what i wanted :). (what i want can also be achieved
with a shell function, i'm just very lazy).

> 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.

-- 
Mikael Magnusson