SegFault in stringsubst

SegFault in stringsubst

[Andrew Waldron]

Hi workers,

I'm getting a segfault in stringsubst when using process substitution
with anonymous functions.

I tried this with 5.0.5 and the head of git:
"() (print $1) <(:)" segfaults
"() {print $1} <(:)" works correctly.

There is also a segfault if you accidentally use process substitution
for a function name:
"function <(:) print" segfaults
"function <(:) {:}" does not segfault.

The segfault is on "restlen = strlen(rest);" when rest is uninitialised.

I made the changes below and it seemed to fix the issue.

Hope this helps,

Andrew.

diff --git a/Src/exec.c b/Src/exec.c
index f16cfd3..bf784bc 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3867,8 +3867,10 @@ getoutputfile(char *cmd, char **eptr)
     int fd;
     char *s;
 
-    if (thisjob == -1)
+    if (thisjob == -1){
+	zerr("process substitution %s cannot be used here", cmd);
 	return NULL;
+    }
     if (!(prog = parsecmd(cmd, eptr)))
 	return NULL;
     if (!(nam = gettempname(NULL, 0)))
@@ -3939,11 +3941,13 @@ namedpipe(void)
     char *tnam = gettempname(NULL, 1);
 
 # ifdef HAVE_MKFIFO
-    if (mkfifo(tnam, 0600) < 0)
+    if (mkfifo(tnam, 0600) < 0){
 # else
-    if (mknod(tnam, 0010600, 0) < 0)
+    if (mknod(tnam, 0010600, 0) < 0){
 # endif
+	zerr("failed to create named pipe: %s, %e", tnam, errno);
 	return NULL;
+    }
     return tnam;
 }
 #endif /* ! PATH_DEV_FD && HAVE_FIFOS */
@@ -3966,9 +3970,10 @@ getproc(char *cmd, char **eptr)
 
 #ifndef PATH_DEV_FD
     int fd;
-
-    if (thisjob == -1)
+    if (thisjob == -1) {
+	zerr("process substitution %s cannot be used here", cmd);
 	return NULL;
+    }
     if (!(pnam = namedpipe()))
 	return NULL;
     if (!(prog = parsecmd(cmd, eptr)))
@@ -3993,8 +3998,10 @@ getproc(char *cmd, char **eptr)
 #else /* PATH_DEV_FD */
     int pipes[2], fd;
 
-    if (thisjob == -1)
+    if (thisjob == -1) {
+	zerr("process substitution %s cannot be used here", cmd);
 	return NULL;
+    }
     pnam = hcalloc(strlen(PATH_DEV_FD) + 6);
     if (!(prog = parsecmd(cmd, eptr)))
 	return NULL;
diff --git a/Src/parse.c b/Src/parse.c
index f0d0855..530a070 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -1471,7 +1471,6 @@ par_funcdef(int *complex)
 	if (num == 0) {
 	    /* Anonymous function, possibly with arguments */
 	    incmdpos = 0;
-	    *complex = 1;
 	}
 	zshlex();
     } else if (unset(SHORTLOOPS)) {
@@ -1503,6 +1502,7 @@ par_funcdef(int *complex)
 	    num++;
 	    zshlex();
 	}
+	*complex = (num > 0);
 	ecbuf[parg] = ecused - parg; /*?*/
 	ecbuf[parg+1] = num;
     }
@@ -1736,7 +1736,6 @@ par_simple(int *complex, int nr)
 		if (argc == 0) {
 		    /* Anonymous function, possibly with arguments */
 		    incmdpos = 0;
-		    *complex = 1;
 		}
 		zshlex();
 	    } else {
@@ -1776,6 +1775,7 @@ par_simple(int *complex, int nr)
 		    argc++;
 		    zshlex();
 		}
+		*complex = (argc > 0);
 		ecbuf[parg] = ecused - parg; /*?*/
 		ecbuf[parg+1] = argc;
 	    }
diff --git a/Src/subst.c b/Src/subst.c
index cc5df3f..4713502 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -169,7 +169,7 @@ stringsubst(LinkList list, LinkNode node, int pf_flags, int asssub)
 	    if (errflag)
 		return NULL;
 	    if (!subst)
-		subst = "";
+		rest = subst = "";
 
 	    sublen = strlen(subst);
 	    restlen = strlen(rest);
diff --git a/Test/C04funcdef.ztst b/Test/C04funcdef.ztst
index 706aa28..a266031 100644
--- a/Test/C04funcdef.ztst
+++ b/Test/C04funcdef.ztst
@@ -208,6 +208,11 @@
 >Da de da
 >Do be do
 
+  () (cat $1 $2) <(print process expanded) =(print expanded to file)
+0:Process substitution with anonymous functions
+>process expanded
+>expanded to file
+
   () { print This has arguments $*; } of all sorts; print After the function
   function { print More stuff $*; } and why not; print Yet more
 0:Anonymous function with arguments

Re: SegFault in stringsubst

[Bart Schaefer]

On Apr 16,  2:46pm, Andrew Waldron wrote:
}
} The segfault is on "restlen = strlen(rest);" when rest is uninitialised.
} 
} I made the changes below and it seemed to fix the issue.

The exec.c change doesn't change anything other than verbosity about some
error conditions, correct?

-- 
Barton E. Schaefer

Re: SegFault in stringsubst

[Andrew Waldron]

On Tue, 15 Apr 2014, Bart Schaefer wrote:

> The exec.c change doesn't change anything other than verbosity about some
> error conditions, correct?

Correct, if you take the change in stringsubst then there is no segfault.
You just end up with empty substitutions.

Regards
Andrew.

Re: SegFault in stringsubst

[Bart Schaefer]

On Apr 16,  4:19pm, Andrew Waldron wrote:
}
} On Tue, 15 Apr 2014, Bart Schaefer wrote:
} 
} > The exec.c change doesn't change anything other than verbosity about some
} > error conditions, correct?
} 
} Correct, if you take the change in stringsubst then there is no segfault.
} You just end up with empty substitutions.

Any second opinions on whether we want the increased verbosity?  Peter?

-- 
Barton E. Schaefer

Re: SegFault in stringsubst

[Peter Stephenson]

[-- Attachment #1: Type: text/plain, Size: 780 bytes --]

On Wednesday, 16 April 2014, Bart Schaefer <schaefer@brasslantern.com>
wrote:
> On Apr 16,  4:19pm, Andrew Waldron wrote:
> }
> } On Tue, 15 Apr 2014, Bart Schaefer wrote:
> }
> } > The exec.c change doesn't change anything other than verbosity about
some
> } > error conditions, correct?
> }
> } Correct, if you take the change in stringsubst then there is no
segfault.
> } You just end up with empty substitutions.
>
> Any second opinions on whether we want the increased verbosity?  Peter?

I think it's certainly useful in debug mode, and there's definitely an
argument for
always reporting it as the alternative is probably rather obscure. The only
likely
negative effect I can think of might be of the caller also reported an
error.

I'm likely to be quiet for a week.

pws

Re: SegFault in stringsubst

[Bart Schaefer]

On Apr 16,  2:46pm, Andrew Waldron wrote:
}
} There is also a segfault if you accidentally use process substitution
} for a function name:
} "function <(:) print" segfaults
} "function <(:) {:}" does not segfault.

Calling zerr() for this only makes sense if the surrounding code pays
attention to errflag.  E.g. with the patch from 32552,

    function <(:) print { : }

emits an error message but defines a function named "print" anyway.

Anyone see any problems with this addition?

diff --git a/Src/exec.c b/Src/exec.c
index f16cfd3..d821d16 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2346,7 +2346,7 @@ execsubst(LinkList strs)
 {
     if (strs) {
 	prefork(strs, esprefork);
-	if (esglob) {
+	if (esglob && !errflag) {
 	    LinkList ostrs = strs;
 	    globlist(strs, 0);
 	    strs = ostrs;
@@ -4234,8 +4241,11 @@ execfuncdef(Estate state, UNUSED(int do_exec))
     plen = nprg * sizeof(wordcode);
     len = plen + (npats * sizeof(Patprog)) + nstrs;
 
-    if (htok && names)
+    if (htok && names) {
 	execsubst(names);
+	if (errflag)
+	    return 1;
+    }
 
     while (!names || (s = (char *) ugetnode(names))) {
 	if (!names) {

-- 
Barton E. Schaefer

Re: SegFault in stringsubst

[Andrew Waldron]

On Thu, 17 Apr 2014, Bart Schaefer wrote:

> Calling zerr() for this only makes sense if the surrounding code pays
> attention to errflag.  E.g. with the patch from 32552,
> 
>     function <(:) print { : }
> 
> emits an error message but defines a function named "print" anyway.
> 
> Anyone see any problems with this addition?
> 

Seems like a similar check should be made for anonymous function
arguments so that lastval will be set?


diff --git a/Src/exec.c b/Src/exec.c
index f16cfd3..36b7efa 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4291,8 +4291,11 @@ execfuncdef(Estate state, UNUSED(int do_exec))
 	    end += *state->pc++;
 	    args = ecgetlist(state, *state->pc++, EC_DUPTOK, &htok);
 
-	    if (htok && args)
+	    if (htok && args) {
 		execsubst(args);
+		if (errflag)
+		    return lastval ? lastval : 1;
+	    }
 
 	    if (!args)
 		args = newlinklist();

--
Regards, Andrew.

Re: SegFault in stringsubst

[Bart Schaefer]

On Apr 19,  2:03am, Andrew Waldron wrote:
}
} Seems like a similar check should be made for anonymous function
} arguments so that lastval will be set?

This is beginning to have knock-on effects that concern me.  E.g.:

* I don't know whether the lastval ternary test is the right thing, or
  if an unconditional return of 1 is appropriate (should lastval be
  checked in the code I already committed?);

* if we're testing errflag here, we should also do it when evaluating
  "for" and "select" loops (loop.c), which is pushing a new failure
  mode pretty far afield from the original bug;

* which means I'd be a lot happier if we were detecting this as a
  syntax error instead of a run-time error, but that may be pretty
  difficult to do.
 
} diff --git a/Src/exec.c b/Src/exec.c
} index f16cfd3..36b7efa 100644
} --- a/Src/exec.c
} +++ b/Src/exec.c
} @@ -4291,8 +4291,11 @@ execfuncdef(Estate state, UNUSED(int do_exec))
}  	    end += *state->pc++;
}  	    args = ecgetlist(state, *state->pc++, EC_DUPTOK, &htok);
}  
} -	    if (htok && args)
} +	    if (htok && args) {
}  		execsubst(args);
} +		if (errflag)
} +		    return lastval ? lastval : 1;
} +	    }
}  
}  	    if (!args)
}  		args = newlinklist();

Re: SegFault in stringsubst

[Bart Schaefer]

On Apr 18, 10:02am, Bart Schaefer wrote:
}
} * I don't know whether the lastval ternary test is the right thing, or
}   if an unconditional return of 1 is appropriate (should lastval be
}   checked in the code I already committed?);

I had to come up with a pretty convoluted example to get this to matter.
The most straighforward way to force errflag to be set by substitution
is to use the ${param:?message} syntax:

	unset x
	X=${x:-$(return 5)}		# sets $? to 5
	X=${${x:-$(return 5)}:?fail}	# sets $? to 1

So I think the right answer is that lastval shouldn't matter here, we
just return 1 for the error.

Curiously, though, *without* Andrew's 32563 patch:

	() { : } ${${:-$(return 5)}:?fail}	# sets $? to 5

Thus lastval is already being preserved, possibly incorrectly, because
execfuncdef() does not zero lastval before calling execshfunc() where
errflag is eventually tested.  This also means that:

	() { echo $? } ${:-$(return 5)}

outputs 5, because the status from the argument list is passed in to the
function body.  That's consistent with the behavior of non-anonymous
functions (and is consistent with functions in bash, for that matter).

} * if we're testing errflag here, we should also do it when evaluating
}   "for" and "select" loops (loop.c), which is pushing a new failure
}   mode pretty far afield from the original bug;

In "for"/"select", this failure would be occurring in the list of words
that appears after the "in" token.  I stepped through this, and found
that errflag is tested fairly early when the first command in the loop
body begins to execute -- early enough that the loop body becomes a
no-op and we've simply wasted a few cycles pushing/popping the pipeline
state.  

Catching the error early therefore amounts to a minor optimization.
However, lastval is zeroed by execfor() before calling execlist(), so
if for some reason we DO want to preserve lastval, that optimization
is necessary.

ASIDE:

	unset x
	for x in ${x:-$(exit 5)} y; do echo $?; done

outputs 5 in bash and 0 in zsh.  This suggests that perhaps execfor()
should NOT be clobbering lastval.  (Same for "select".)

} * which means I'd be a lot happier if we were detecting this as a
}   syntax error instead of a run-time error, but that may be pretty
}   difficult to do.

I've made myself comfortable with having this be a runtime error.

The final question is whether the bytecode program counter should be
updated before returning these error conditions.  Looking at execfor()
as the example, I think it should be, which I haven't done in previous
patch.

Therefore, ignoring the aside above for the time being, I suggest:

diff --git a/Src/exec.c b/Src/exec.c
index d821d16..8249def 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4243,8 +4243,10 @@ execfuncdef(Estate state, UNUSED(int do_exec))
 
     if (htok && names) {
 	execsubst(names);
-	if (errflag)
+	if (errflag) {
+	    state->pc = end;
 	    return 1;
+	}
     }
 
     while (!names || (s = (char *) ugetnode(names))) {
@@ -4301,8 +4303,13 @@ execfuncdef(Estate state, UNUSED(int do_exec))
 	    end += *state->pc++;
 	    args = ecgetlist(state, *state->pc++, EC_DUPTOK, &htok);
 
-	    if (htok && args)
+	    if (htok && args) {
 		execsubst(args);
+		if (errflag) {
+		    state->pc = end;
+		    return 1;
+		}
+	    }
 
 	    if (!args)
 		args = newlinklist();
diff --git a/Src/loop.c b/Src/loop.c
index 90a0761..dc8f232 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -87,8 +87,13 @@ execfor(Estate state, int do_exec)
 		state->pc = end;
 		return 0;
 	    }
-	    if (htok)
+	    if (htok) {
 		execsubst(args);
+		if (errflag) {
+		    state->pc = end;
+		    return 1;
+		}
+	    }
 	} else {
 	    char **x;
 
@@ -223,8 +228,13 @@ execselect(Estate state, UNUSED(int do_exec))
 	    state->pc = end;
 	    return 0;
 	}
-	if (htok)
+	if (htok) {
 	    execsubst(args);
+	    if (errflag) {
+		state->pc = end;
+		return 1;
+	    }
+	}
     }
     if (!args || empty(args)) {
 	state->pc = end;

Sanity of lastval ($?) in for/select loops

[Bart Schaefer]

On Apr 19, 10:54am, Bart Schaefer wrote:
}
} ASIDE:
} 
} 	unset x
} 	for x in ${x:-$(exit 5)} y; do echo $?; done
} 
} outputs 5 in bash and 0 in zsh.  This suggests that perhaps execfor()
} should NOT be clobbering lastval.  (Same for "select".)

Data point: ksh93 agrees with bash here.

Furthermore, zsh execfor() and execselect() are inconsistent in their
handling of "lastval".  E.g., execfor() has this in the branch that
handles "for ((e1;e2;e3))" constructs:

        if (!errflag)
            matheval(str);
        if (errflag) {
            state->pc = end;
            return lastval = errflag;
        }

But later, in handling the loop body itself:

            if (errflag) {
                if (breaks)
                    breaks--;
                lastval = 1;
                break;
            }

So which should it be, lastval = 1 or lastval = errflag?  In practice I
don't think errflag is ever assigned anything other than 0 or 1 so it
probably isn't significant, just a bit confusing when reading the code.

However, execselect() then has:

    if (!args || empty(args)) {
	state->pc = end;
	return 1;
    }

Not "return lastval = 1" even though elsewhere it's careful to return
lastval.  Turns out the caller [execsimple() or execcmd()] always sets
lastval to whatever value is returned from execfor() or execselect()
or any of the other similar functions, so it's redundant to assign it
immediately before returning.

All of which presented as justification for this little patch:

diff --git a/Src/loop.c b/Src/loop.c
index dc8f232..2f639fd 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -73,7 +73,7 @@ execfor(Estate state, int do_exec)
 	    matheval(str);
 	if (errflag) {
 	    state->pc = end;
-	    return lastval = errflag;
+	    return 1;
 	}
 	cond = ecgetstr(state, EC_NODUP, &ctok);
 	advance = ecgetstr(state, EC_NODUP, &atok);
@@ -102,7 +102,7 @@ execfor(Estate state, int do_exec)
 		addlinknode(args, dupstring(*x));
 	}
     }
-    lastval = 0;
+    /* lastval = 0; */
     loops++;
     pushheap();
     cmdpush(CS_FOR);
@@ -241,7 +241,7 @@ execselect(Estate state, UNUSED(int do_exec))
 	return 1;
     }
     loops++;
-    lastval = 0;
+    /* lastval = 0; */
     pushheap();
     cmdpush(CS_SELECT);
     usezle = interact && SHTTY != -1 && isset(USEZLE);