Re: SegFault in stringsubst

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

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;