From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11973 invoked by alias); 19 Apr 2014 17:54:50 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 32568 Received: (qmail 23412 invoked from network); 19 Apr 2014 17:54:44 -0000 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 From: Bart Schaefer Message-id: <140419105445.ZM4656@torch.brasslantern.com> Date: Sat, 19 Apr 2014 10:54:45 -0700 In-reply-to: <140418100200.ZM23843@torch.brasslantern.com> Comments: In reply to Bart Schaefer "Re: SegFault in stringsubst" (Apr 18, 10:02am) References: <20140416044612.GB24565@ewok> <140417114252.ZM22145@torch.brasslantern.com> <20140418160344.GA10455@ewok> <140418100200.ZM23843@torch.brasslantern.com> X-Mailer: OpenZMail Classic (0.9.2 24April2005) To: zsh-workers@zsh.org Subject: Re: SegFault in stringsubst MIME-version: 1.0 Content-type: text/plain; charset=us-ascii 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;