From: Bart Schaefer <schaefer@brasslantern.com>
To: zsh-workers@zsh.org
Subject: Re: SegFault in stringsubst
Date: Sat, 19 Apr 2014 10:54:45 -0700 [thread overview]
Message-ID: <140419105445.ZM4656@torch.brasslantern.com> (raw)
In-Reply-To: <140418100200.ZM23843@torch.brasslantern.com>
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;
next prev parent reply other threads:[~2014-04-19 17:54 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-04-16 4:46 Andrew Waldron
2014-04-16 5:15 ` Bart Schaefer
2014-04-16 6:19 ` Andrew Waldron
2014-04-16 16:13 ` Bart Schaefer
2014-04-17 8:58 ` Peter Stephenson
2014-04-17 18:42 ` Bart Schaefer
2014-04-18 16:03 ` Andrew Waldron
2014-04-18 17:02 ` Bart Schaefer
2014-04-19 17:54 ` Bart Schaefer [this message]
2014-04-20 18:00 ` Sanity of lastval ($?) in for/select loops Bart Schaefer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=140419105445.ZM4656@torch.brasslantern.com \
--to=schaefer@brasslantern.com \
--cc=zsh-workers@zsh.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).