From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1034 invoked by alias); 30 Sep 2016 13:50:30 -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: 39521 Received: (qmail 17379 invoked from network); 30 Sep 2016 13:50:30 -0000 X-Qmail-Scanner-Diagnostics: from mailout4.w1.samsung.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.99.2/21882. spamassassin: 3.4.1. Clear:RC:0(210.118.77.14):SA:0(-3.0/5.0):. Processed in 0.747658 secs); 30 Sep 2016 13:50:30 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-3.0 required=5.0 tests=RP_MATCHES_RCVD autolearn=unavailable autolearn_force=no version=3.4.1 X-Envelope-From: p.stephenson@samsung.com X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | Received-SPF: none (ns1.primenet.com.au: domain at samsung.com does not designate permitted sender hosts) X-AuditID: cbfec7f4-f791c6d000006eac-2d-57ee6d9ab1a2 Date: Fri, 30 Sep 2016 14:50:14 +0100 From: Peter Stephenson To: Zsh Hackers' List Subject: Re: Strange parameter visibility Message-id: <20160930145014.3df590f7@pwslap01u.europe.root.pri> In-reply-to: <20160930103625.0f46aa9d@pwslap01u.europe.root.pri> Organization: Samsung Cambridge Solution Centre X-Mailer: Claws Mail 3.7.9 (GTK+ 2.22.0; i386-redhat-linux-gnu) MIME-version: 1.0 Content-type: text/plain; charset=US-ASCII Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrNIsWRmVeSWpSXmKPExsWy7djPc7qzct+FG3y7rGhxsPkhkwOjx6qD H5gCGKO4bFJSczLLUov07RK4MlbO/MBc8CWq4tGd2+wNjM2uXYycHBICJhIz5/5hh7DFJC7c W8/WxcjFISSwlFFi89GbLBBOL5PE+qUr2GE6Jk45xQqRWMYoMevZSrCEkMA0Jon7M9UgEmcY JTbuesMI4ZxllLjZ0M8GUsUioCqxZPFSFhCbTcBQYuqm2UBFHBwiAtoS7R/FQMLCApoSG1Y+ ZAMJ8wrYSyxYpwIS5hRwkLi7YzEriM0voC9x9e8nJoiD7CVmXjnDCGLzCghK/Jh8D2w6s4CO xLZtj9khbHmJzWveMoOcIyHQzC4xZctVVpD5EgKyEpsOMEPMcZF4NmkR1JPCEq+Ob4GyZSQu T+5mgbD7GSWedPtCzJnBKHH6zA42iIS1RN/ti4wQy/gkJm2bzgwxn1eio00IosRD4uStp4wQ tqPE+19bWCYwKs5CcvYsJGfPQnL2AkbmVYwiqaXFuempxSZ6xYm5xaV56XrJ+bmbGIFJ4PS/ 4192MC4+ZnWIUYCDUYmH90TUu3Ah1sSy4srcQ4wSHMxKIrzRmUAh3pTEyqrUovz4otKc1OJD jNIcLErivHsWXAkXEkhPLEnNTk0tSC2CyTJxcEo1MIpdVQnc/zpTVmFe/NvJHxldercwh59u yIpV6Pqf53Dkj3PtuoNN9kZdvSc/PpLfetMnqfH+7+6rdlrR7j03fM5daC8U6Uo3FDaOOWH4 ZG3awx2Hp22MFJhsPNNo355rFYbnrpzR2cN3pObbodZjlW5ZbkHzAkOdDOuk/Y6cDGhSfLvi XJ2bmhJLcUaioRZzUXEiALj51jP+AgAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrMIsWRmVeSWpSXmKPExsVy+t/xy7p5ue/CDS7csLY42PyQyYHRY9XB D0wBjFFuNhmpiSmpRQqpecn5KZl56bZKoSFuuhZKCnmJuam2ShG6viFBSgpliTmlQJ6RARpw cA5wD1bSt0twy1g58wNzwZeoikd3brM3MDa7djFyckgImEhMnHKKFcIWk7hwbz1bFyMXh5DA EkaJxjMrmEESQgIzmCSuPfGAsM8xSrydqAtRdJZRYu+yvUwgCRYBVYkli5eygNhsAoYSUzfN Zuxi5OAQEdCWaP8oBhIWFtCU2LDyIRtImFfAXmLBOhWQMKeAg8TdHYtZIUa2MUtsa24H28sv oC9x9e8nJojj7CVmXjnDCGLzCghK/Jh8D2wVs4CWxOZtTawQtrzE5jVvoW5Wl7hxdzf7BEbh WUhaZiFpmYWkZQEj8ypGkdTS4tz03GIjveLE3OLSvHS95PzcTYzACNp27OeWHYxd74IPMQpw MCrx8J6IehcuxJpYVlyZe4hRgoNZSYQ3OhMoxJuSWFmVWpQfX1Sak1p8iNEUGC4TmaVEk/OB 0Z1XEm9oYmhuaWhkbGFhbmSkJM479cOVcCGB9MSS1OzU1ILUIpg+Jg5OqQbGyB+yMV7pezbn zPFd5NfY8P8st3DSqe+L1Mvdb/s5W7EuFttYMpe5oMLB2Emkd+Wam69NivWvTcz7bLrB7NVX hzvc9Wse7rrGZDk5e/6EH/7JVuaWWjs1ZmY/dT+rlbY56ZfSOsdpe1d/YWN6HG9a1RoY3bPF 4dxHqUovrjWrmfpk7cIWtjsrsRRnJBpqMRcVJwIA+gnYJ7YCAAA= X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20160930135018eucas1p182fef2a7b2d2fb0657a12fc3840d668b X-Msg-Generator: CA X-Sender-IP: 182.198.249.180 X-Local-Sender: =?UTF-8?B?UGV0ZXIgU3RlcGhlbnNvbhtTQ1NDLURhdGEgUGxhbmUb?= =?UTF-8?B?7IK87ISx7KCE7J6QG1ByaW5jaXBhbCBFbmdpbmVlciwgU29mdHdhcmU=?= X-Global-Sender: =?UTF-8?B?UGV0ZXIgU3RlcGhlbnNvbhtTQ1NDLURhdGEgUGxhbmUbU2Ft?= =?UTF-8?B?c3VuZyBFbGVjdHJvbmljcxtQcmluY2lwYWwgRW5naW5lZXIsIFNvZnR3YXJl?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDA1Q0QwNTAwNTg=?= CMS-TYPE: 201P X-HopCount: 7 X-CMS-RootMailID: 20160917181339eucas1p24d214aa618aa96b5a8ddfbf351598da6 X-RootMTR: 20160917181339eucas1p24d214aa618aa96b5a8ddfbf351598da6 References: <87bmzmtmzq.fsf@alfa.kjonca> <20160929172417.5022a014@pwslap01u.europe.root.pri> <20160929180301.5d1930d0@pwslap01u.europe.root.pri> <160929142821.ZM16694@torch.brasslantern.com> <20160930095032.17083e7b@pwslap01u.europe.root.pri> <20160930103625.0f46aa9d@pwslap01u.europe.root.pri> On Fri, 30 Sep 2016 10:36:25 +0100 Peter Stephenson wrote: > A more robust fix is a complete refactoring of execcmd(), which is far > too big anyway, with the appropriate parts being called form above, > deciding on asignments and analysing the commands early, possibly > allowing there to be a single fork in one place. However, that's a big > job --- and without extra code to delay prefork() I still don't think > that fixes the case ": ${x:=2} | echo $x", and I suspect the case > "cmd=:; $cmd ${x:=2} | echo $x" is pretty much unfixable without > delaying the assignment from the ${x:=2} until a later part of > processing, which might have side effects. A start at refactoring, which I think is generally beneficial, already fixes the original problem robustly with a small tweak: we can now check within execpline2() that there are no arguments to the command, so therefore it will be handled entirely within the shell. This probably needs more exercising, but tests already passed (including the new one which is restored). If you thought "but I really liked the huge, monolithic execcmd()" please keep your thoughts to yourself... pws diff --git a/Src/exec.c b/Src/exec.c index 2714edb..349414c 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -1808,6 +1808,7 @@ execpline2(Estate state, wordcode pcode, { pid_t pid; int pipes[2]; + struct execcmd_params eparams; if (breaks || retflag) return; @@ -1825,17 +1826,16 @@ execpline2(Estate state, wordcode pcode, else list_pipe_text[0] = '\0'; } - if (WC_PIPE_TYPE(pcode) == WC_PIPE_END) - execcmd(state, input, output, how, last1 ? 1 : 2); - else { + if (WC_PIPE_TYPE(pcode) == WC_PIPE_END) { + execcmd_analyse(state, &eparams); + execcmd_exec(state, &eparams, input, output, how, last1 ? 1 : 2); + } else { int old_list_pipe = list_pipe; int subsh_close = -1; - Wordcode next = state->pc + (*state->pc), pc; - wordcode code; + Wordcode next = state->pc + (*state->pc), start_pc; - state->pc++; - for (pc = state->pc; wc_code(code = *pc) == WC_REDIR; - pc += WC_REDIR_WORDS(code)); + start_pc = ++state->pc; + execcmd_analyse(state, &eparams); if (mpipe(pipes) < 0) { /* FIXME */ @@ -1844,7 +1844,7 @@ execpline2(Estate state, wordcode pcode, /* if we are doing "foo | bar" where foo is a current * * shell command, do foo in a subshell and do the * * rest of the pipeline in the current shell. */ - if ((wc_code(code) >= WC_CURSH) + if ((eparams.type >= WC_CURSH || !eparams.args) && (how & Z_SYNC)) { int synch[2]; struct timeval bgtime; @@ -1863,7 +1863,7 @@ execpline2(Estate state, wordcode pcode, } else if (pid) { char dummy, *text; - text = getjobtext(state->prog, state->pc); + text = getjobtext(state->prog, start_pc); addproc(pid, text, 0, &bgtime); close(synch[1]); read_loop(synch[0], &dummy, 1); @@ -1874,14 +1874,14 @@ execpline2(Estate state, wordcode pcode, entersubsh(((how & Z_ASYNC) ? ESUB_ASYNC : 0) | ESUB_PGRP | ESUB_KEEPTRAP); close(synch[1]); - execcmd(state, input, pipes[1], how, 1); + execcmd_exec(state, &eparams, input, pipes[1], how, 1); _exit(lastval); } } else { /* otherwise just do the pipeline normally. */ addfilelist(NULL, pipes[0]); subsh_close = pipes[0]; - execcmd(state, input, pipes[1], how, 0); + execcmd_exec(state, &eparams, input, pipes[1], how, 0); } zclose(pipes[1]); state->pc = next; @@ -2537,55 +2537,51 @@ resolvebuiltin(const char *cmdarg, HashNode hn) return hn; } +/* + * We are about to execute a command at the lowest level of the + * hierarchy. Analyse the parameters from the wordcode. + */ + /**/ static void -execcmd(Estate state, int input, int output, int how, int last1) +execcmd_analyse(Estate state, Execcmd_params eparams) { - HashNode hn = NULL; - LinkList args, filelist = NULL; - LinkNode node; - Redir fn; - struct multio *mfds[10]; - char *text; - int save[10]; - int fil, dfil, is_cursh, type, do_exec = 0, redir_err = 0, i, htok = 0; - int nullexec = 0, assign = 0, forked = 0, postassigns = 0; - int is_shfunc = 0, is_builtin = 0, is_exec = 0, use_defpath = 0; - /* Various flags to the command. */ - int cflags = 0, orig_cflags = 0, checked = 0, oautocont = -1; - LinkList redir; wordcode code; - Wordcode beg = state->pc, varspc, assignspc = (Wordcode)0; - FILE *oxtrerr = xtrerr, *newxtrerr = NULL; + int i; - doneps4 = 0; - redir = (wc_code(*state->pc) == WC_REDIR ? ecgetredirs(state) : NULL); + eparams->beg = state->pc; + eparams->redir = + (wc_code(*state->pc) == WC_REDIR ? ecgetredirs(state) : NULL); if (wc_code(*state->pc) == WC_ASSIGN) { cmdoutval = 0; - varspc = state->pc; + eparams->varspc = state->pc; while (wc_code((code = *state->pc)) == WC_ASSIGN) state->pc += (WC_ASSIGN_TYPE(code) == WC_ASSIGN_SCALAR ? 3 : WC_ASSIGN_NUM(code) + 2); } else - varspc = NULL; + eparams->varspc = NULL; code = *state->pc++; - type = wc_code(code); + eparams->type = wc_code(code); + eparams->postassigns = 0; /* It would be nice if we could use EC_DUPTOK instead of EC_DUP here. * But for that we would need to check/change all builtins so that * they don't modify their argument strings. */ - switch (type) { + switch (eparams->type) { case WC_SIMPLE: - args = ecgetlist(state, WC_SIMPLE_ARGC(code), EC_DUP, &htok); + eparams->args = ecgetlist(state, WC_SIMPLE_ARGC(code), EC_DUP, + &eparams->htok); + eparams->assignspc = NULL; break; case WC_TYPESET: - args = ecgetlist(state, WC_TYPESET_ARGC(code), EC_DUP, &htok); - postassigns = *state->pc++; - assignspc = state->pc; - for (i = 0; i < postassigns; i++) { + eparams->args = ecgetlist(state, WC_TYPESET_ARGC(code), EC_DUP, + &eparams->htok); + eparams->postassigns = *state->pc++; + eparams->assignspc = state->pc; + for (i = 0; i < eparams->postassigns; i++) { code = *state->pc; DPUTS(wc_code(code) != WC_ASSIGN, "BUG: miscounted typeset assignments"); @@ -2595,8 +2591,45 @@ execcmd(Estate state, int input, int output, int how, int last1) break; default: - args = NULL; + eparams->args = NULL; + eparams->assignspc = NULL; + eparams->htok = 0; + break; } +} + +/* + * Execute a command at the lowest level of the hierarchy. + */ + +/**/ +static void +execcmd_exec(Estate state, Execcmd_params eparams, + int input, int output, int how, int last1) +{ + HashNode hn = NULL; + LinkList filelist = NULL; + LinkNode node; + Redir fn; + struct multio *mfds[10]; + char *text; + int save[10]; + int fil, dfil, is_cursh, do_exec = 0, redir_err = 0, i; + int nullexec = 0, assign = 0, forked = 0; + int is_shfunc = 0, is_builtin = 0, is_exec = 0, use_defpath = 0; + /* Various flags to the command. */ + int cflags = 0, orig_cflags = 0, checked = 0, oautocont = -1; + FILE *oxtrerr = xtrerr, *newxtrerr = NULL; + /* + * Retrieve parameters for quick reference (they are unique + * to us so we can modify the structure if we want). + */ + LinkList args = eparams->args; + LinkList redir = eparams->redir; + Wordcode varspc = eparams->varspc; + int type = eparams->type; + + doneps4 = 0; /* * If assignment but no command get the status from variable @@ -2854,7 +2887,7 @@ execcmd(Estate state, int input, int output, int how, int last1) /* Do prefork substitutions */ esprefork = (assign || isset(MAGICEQUALSUBST)) ? PREFORK_TYPESET : 0; - if (args && htok) + if (args && eparams->htok) prefork(args, esprefork, NULL); if (type == WC_SIMPLE || type == WC_TYPESET) { @@ -2999,7 +3032,7 @@ execcmd(Estate state, int input, int output, int how, int last1) /* Get the text associated with this command. */ if ((how & Z_ASYNC) || (!sfcontext && !sourcelevel && (jobbing || (how & Z_TIMED)))) - text = getjobtext(state->prog, beg); + text = getjobtext(state->prog, eparams->beg); else text = NULL; @@ -3258,7 +3291,7 @@ execcmd(Estate state, int input, int output, int how, int last1) forked = 1; } - if ((esglob = !(cflags & BINF_NOGLOB)) && args && htok) { + if ((esglob = !(cflags & BINF_NOGLOB)) && args && eparams->htok) { LinkList oargs = args; globlist(args, 0); args = oargs; @@ -3572,7 +3605,7 @@ execcmd(Estate state, int input, int output, int how, int last1) } if (type == WC_FUNCDEF) { Eprog redir_prog; - if (!redir && wc_code(*beg) == WC_REDIR) { + if (!redir && wc_code(*eparams->beg) == WC_REDIR) { /* * We're not using a redirection from the currently * parsed environment, which is what we'd do for an @@ -3582,7 +3615,7 @@ execcmd(Estate state, int input, int output, int how, int last1) struct estate s; s.prog = state->prog; - s.pc = beg; + s.pc = eparams->beg; s.strs = state->prog->strs; /* @@ -3666,13 +3699,15 @@ execcmd(Estate state, int input, int output, int how, int last1) } else { /* It's a builtin */ LinkList assigns = (LinkList)0; + int postassigns = eparams->postassigns; if (forked) closem(FDT_INTERNAL); if (postassigns) { Wordcode opc = state->pc; - state->pc = assignspc; + state->pc = eparams->assignspc; assigns = newlinklist(); while (postassigns--) { + int htok; wordcode ac = *state->pc++; char *name = ecgetstr(state, EC_DUPTOK, &htok); Asgment asg; diff --git a/Src/zsh.h b/Src/zsh.h index 79747d6..a5d4455 100644 --- a/Src/zsh.h +++ b/Src/zsh.h @@ -489,6 +489,7 @@ typedef struct complist *Complist; typedef struct conddef *Conddef; typedef struct dirsav *Dirsav; typedef struct emulation_options *Emulation_options; +typedef struct execcmd_params *Execcmd_params; typedef struct features *Features; typedef struct feature_enables *Feature_enables; typedef struct funcstack *Funcstack; @@ -1391,6 +1392,21 @@ struct builtin { */ #define BINF_ASSIGN (1<<19) +/** + * Parameters passed to execcmd(). + * These are not opaque --- they are also used by the pipeline manager. + */ +struct execcmd_params { + LinkList args; /* All command prefixes, arguments & options */ + LinkList redir; /* Redirections */ + Wordcode beg; /* The code at the start of the command */ + Wordcode varspc; /* The code for assignment parsed as such */ + Wordcode assignspc; /* The code for assignment parsed as typeset */ + int type; /* The WC_* type of the command */ + int postassigns; /* The number of assignspc assiguments */ + int htok; /* tokens in parameter list */ +}; + struct module { struct hashnode node; union { diff --git a/Test/A01grammar.ztst b/Test/A01grammar.ztst index 1ad73c5..0b1085c 100644 --- a/Test/A01grammar.ztst +++ b/Test/A01grammar.ztst @@ -757,12 +757,9 @@ >} >Stuff here -## This problem is hard to fix without significant changes to how -## the shell forks for a pipeline. -# -# x=1 -# x=2 | echo $x -# echo $x -# 0:Assignment-only current shell commands in LHS of pipelin -# >1 -# >1 + x=1 + x=2 | echo $x + echo $x +0:Assignment-only current shell commands in LHS of pipelin +>1 +>1