[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20260630195333.GU27423@brightrain.aerifal.cx>
Date: Tue, 30 Jun 2026 15:53:33 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 0/4] wordexp fixes
On Tue, Jun 30, 2026 at 04:31:33PM +0200, Szabolcs Nagy wrote:
> 2 bugfix patches, WRDE_UNDEF implementation and
> a much tighter WRDE_CMDSUB.
> >From b241b6210294a9b2cb8bd44ba4105d862f582ee6 Mon Sep 17 00:00:00 2001
> From: Szabolcs Nagy <nsz@...t70.net>
> Date: Sun, 21 Jun 2026 11:33:28 +0000
> Subject: [PATCH 1/4] wordexp: fix stderr redirection
>
> stderr redirection to /dev/null didn't work, can be done in the
> command, but would not redirect errors printed during sh startup.
>
> wordexp(")", p, 0)
>
> clobbered stderr, now silent.
> ---
> src/misc/wordexp.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/src/misc/wordexp.c b/src/misc/wordexp.c
> index db83a69f..73349aac 100644
> --- a/src/misc/wordexp.c
> +++ b/src/misc/wordexp.c
> @@ -29,7 +29,6 @@ static int do_wordexp(const char *s, wordexp_t *we, int flags)
> int sq=0, dq=0;
> size_t np=0;
> char *w, **tmp;
> - char *redir = (flags & WRDE_SHOWERR) ? "" : "2>/dev/null";
> int err = 0;
> FILE *f;
> size_t wc = 0;
> @@ -108,9 +107,13 @@ static int do_wordexp(const char *s, wordexp_t *we, int flags)
> if (!pid) {
> if (p[1] == 1) fcntl(1, F_SETFD, 0);
> else dup2(p[1], 1);
> + if (!(flags & WRDE_SHOWERR)) {
> + int fd = open("/dev/null", O_WRONLY|O_CLOEXEC);
> + if (fd < 0 || dup2(fd, 2) < 0) close(2);
> + }
> execl("/bin/sh", "sh", "-c",
> - "eval \"printf %s\\\\\\\\0 x $1 $2\"",
> - "sh", s, redir, (char *)0);
> + "eval \"printf %s\\\\\\\\0 x $1\"",
> + "sh", s, (char *)0);
> _exit(1);
> }
> close(p[1]);
> --
> 2.52.0
I don't think silently proceeding with close(2) is reasonable here. It
will potentially cause any descendant to corrupt other output by
mixing in things intended for stderr.
If we need to open /dev/null ourselves, it should probably be done in
the parent with only the dup2 in the child. This way the error can be
handled before there's even a child.
> From 700db8148126afef1331b4109377c6c8be07b43b Mon Sep 17 00:00:00 2001
> From: Szabolcs Nagy <nsz@...t70.net>
> Date: Sun, 21 Jun 2026 12:18:51 +0000
> Subject: [PATCH 2/4] wordexp: clear positional parameters
>
> added 'set --' so $@, $*, $#, $1, $2 are cleared
> (does not seem to be in the spec, but cleaner)
>
> wordexp("$*", p, 0)
>
> was "$*","2>/dev/stderr", now empty list.
> ---
> src/misc/wordexp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/misc/wordexp.c b/src/misc/wordexp.c
> index 73349aac..c4154fad 100644
> --- a/src/misc/wordexp.c
> +++ b/src/misc/wordexp.c
> @@ -112,7 +112,7 @@ static int do_wordexp(const char *s, wordexp_t *we, int flags)
> if (fd < 0 || dup2(fd, 2) < 0) close(2);
> }
> execl("/bin/sh", "sh", "-c",
> - "eval \"printf %s\\\\\\\\0 x $1\"",
> + "eval \"set --;printf %s\\\\\\\\0 x $1\"",
> "sh", s, (char *)0);
> _exit(1);
> }
> --
> 2.52.0
I think this is right. It's in the eval so as not to affect the $1
used in the eval expression, right?
> From 6083d3ed8ca93a69cb4be9d3ea73e4a2d968e344 Mon Sep 17 00:00:00 2001
> From: Szabolcs Nagy <nsz@...t70.net>
> Date: Sun, 21 Jun 2026 13:33:38 +0000
> Subject: [PATCH 3/4] wordexp: implement WRDE_UNDEF
>
> use sh -u to detect undef vars
>
> wordexp("x $U y", p, WRDE_UNDEF)
>
> was "x","y", now fails with WRDE_BADVAL, note that
>
> wordexp("$(echo x $U) y", p, WRDE_UNDEF|WRDE_SHOWERR)
>
> was "x","y", now "y" and prints an error about U. the echo is not
> executed, but wordexp succeeds. undef failure is not propagated from
> a command substitution (but it inherits -u).
I think this is contrary to the behavior as specified. The $U here is
not an expansion by the wordexp, but an expansion in the command
inside $(), which is not specified to be subject to set -u. However it
is somewhat ambiguous.
But as you've noted, if the WRDE_UNDEF should affect things inside
$(), it's still wrong because wordexp isn't failing.
I think it's clear that the behavior should be either success or an
error from wordexp, not just a silent error from the $().
> on failure rerun with sh -n to distinguish unset from syntax errors
> without side-effects. for the syntax check the string is passed
> directly as a command which can be parsed differently:
Nice, I think this approach solves the problem of double side effects.
> wordexp("{ $U", p, 0)
> wordexp("{ $U", p, WRDE_UNDEF)
>
> former is "{" but latter fails with WRDE_SYNTAX, not WRDE_UNDEF.
> (note: using eval "set -n --;..." does not work on mksh, oksh.)
> we == NULL marks the rerun (ub for user code).
>
> wordexp("$(echo>>x) $U $(echo>>x)", p, WRDE_UNDEF)
> wordexp("$(echo>>x) $U $(echo>>x", p, WRDE_UNDEF)
>
> former returns WRDE_BADVAL and writes one \n to x, latter returns
> WRDE_SYNTAX and has no side-effect.
> ---
> src/misc/wordexp.c | 36 ++++++++++++++++++++++++++----------
> 1 file changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/src/misc/wordexp.c b/src/misc/wordexp.c
> index c4154fad..e65f2381 100644
> --- a/src/misc/wordexp.c
> +++ b/src/misc/wordexp.c
> @@ -11,10 +11,11 @@
> #include <fcntl.h>
> #include "pthread_impl.h"
>
> -static void reap(pid_t pid)
> +static int reap(pid_t pid)
> {
> int status;
> while (waitpid(pid, &status, 0) < 0 && errno == EINTR);
> + return WIFEXITED(status) && WEXITSTATUS(status) == 0;
> }
>
> static char *getword(FILE *f)
> @@ -36,6 +37,7 @@ static int do_wordexp(const char *s, wordexp_t *we, int flags)
> int p[2];
> pid_t pid;
> sigset_t set;
> + int syntax_check = !we;
>
> if (flags & WRDE_REUSE) wordfree(we);
>
> @@ -91,7 +93,7 @@ static int do_wordexp(const char *s, wordexp_t *we, int flags)
> if (we->we_offs > SIZE_MAX/sizeof(void *)/4)
> goto nospace;
> i += we->we_offs;
> - } else {
> + } else if (!syntax_check) {
> we->we_offs = 0;
> }
>
> @@ -111,9 +113,12 @@ static int do_wordexp(const char *s, wordexp_t *we, int flags)
> int fd = open("/dev/null", O_WRONLY|O_CLOEXEC);
> if (fd < 0 || dup2(fd, 2) < 0) close(2);
> }
> - execl("/bin/sh", "sh", "-c",
> - "eval \"set --;printf %s\\\\\\\\0 x $1\"",
> - "sh", s, (char *)0);
> + if (syntax_check)
> + execl("/bin/sh", "sh", "-cn", "--", s, (char *)0);
> + else
> + execl("/bin/sh", "sh", flags & WRDE_UNDEF ? "-cu" : "-c",
> + "eval \"set --;printf %s\\\\\\\\0 x $1\"",
> + "sh", s, (char *)0);
> _exit(1);
> }
> close(p[1]);
> @@ -121,9 +126,7 @@ static int do_wordexp(const char *s, wordexp_t *we, int flags)
> f = fdopen(p[0], "r");
> if (!f) {
> close(p[0]);
> - kill(pid, SIGKILL);
> - reap(pid);
> - goto nospace;
> + goto killchild;
> }
>
> l = wv ? i+1 : 0;
> @@ -131,9 +134,19 @@ static int do_wordexp(const char *s, wordexp_t *we, int flags)
> free(getword(f));
> if (feof(f)) {
> fclose(f);
> - reap(pid);
> + int success = reap(pid);
> + if (syntax_check && success)
> + return WRDE_BADVAL;
> + if (flags & WRDE_UNDEF)
> + /* rerun with set -n */
> + return do_wordexp(s, 0, 0);
> return WRDE_SYNTAX;
> }
> + if (syntax_check) {
> + /* there should not be output. */
> + fclose(f);
> + goto killchild;
> + }
>
> while ((w = getword(f))) {
> if (i+1 >= l) {
> @@ -162,8 +175,11 @@ static int do_wordexp(const char *s, wordexp_t *we, int flags)
> }
> return err;
>
> +killchild:
> + kill(pid, SIGKILL);
> + reap(pid);
> nospace:
> - if (!(flags & WRDE_APPEND)) {
> + if (!(flags & WRDE_APPEND) && !syntax_check) {
> we->we_wordc = 0;
> we->we_wordv = 0;
> }
> --
> 2.52.0
I haven't read this in depth yet, but at a high level it looks ok.
> From 05d0c797efcb101c855bb47095261ed49dcb5a26 Mon Sep 17 00:00:00 2001
> From: Szabolcs Nagy <nsz@...t70.net>
> Date: Thu, 25 Jun 2026 20:12:48 +0000
> Subject: [PATCH 4/4] wordexp: reimplement WRDE_NOCMD
>
> nested structures and various details were not handled so it was easy
> to pass the checks with input doing cmd sub, but the checks rejected
> a simple ${var} breaking valid use-cases.
>
> implement enough of the shell tokenization rules to detect cmd sub
> with low false positive rate and no false negative (missed cmd sub).
> nocmd check design breakdown:
>
> 1. \ \n and # comment handling
> 2. ' state tracking
> 3. ( ) tracking to match ))
> 4. filter impl specific, unsafe cases
>
> without 1. cmd sub can be missed:
>
> $\
> (cmd)
>
> #\
> $(cmd)
>
> 2. without ' state tracking it is possible to construct a suffix with
> missed cmd sub. suffix examples when miscalculated ' state is
>
> esc: '$(cmd)'
> nop: '"}))..} '"}))..} $(cmd)\'
>
> to track ', the relevant parse states:
>
> i: initial state, where ' is esc
> q: in "...", where ' is nop
> b%: in ${v%...}, where ' is esc
> b-: in ${v-...}, where ' is esc,nop,bad of the enclosing state
> p: in $((...)), where ' is bad, " is bad, # is bad
>
> b%, b-, p can nest into any other state, q can nest into b%, b- and i.
> esc means escape until the next ', nop means literal ' without special
> processing, bad means rejected. e.g. the nesting state is
>
> i b- b% q p b- q
>
> when parsing is at . in
>
> ${v-${v%"$((${v-"."}))"}}
>
> linear time parsing requires linear storage in input length to decide
> if ' is nop or esc. for bounded storage, the accepted language needs
> to be restricted e.g. depth limit (in posix q b- .. q is unspecified
> but that's not enough). the bad ' state covers impl specific behaviour
>
> $((${x-'$(cmd)'}))
>
> ksh treats ' as esc, while bash (and posix) as nop. such cases are
> rejected at ' without further analysing the '... suffix.
>
> 3. is about disambiguating $((arith)) and $(cmd), since cmd can be
> (subcmd) i.e. $((subcmd)). posix requires conforming scripts to use
> $( (subcmd) ) and that arith parsing has precedence and cmd is only
> considered if that fails (but not clear what is a fail). in practice
>
> $((cmd) )
>
> does cmd sub, so at least we need to detect )) mismatch. there is
>
> $((echo hi #(
> )
> ))
>
> where )) matches $((, but bash evals it to hi), so any # is rejected.
> another case is << here doc with special ' and parsing rules, but it
> cannot hide cmd sub if )) mismatch is rejected as well as ', ", ;, #.
> this seems enough to avoid missed cmd sub.
>
> 4. impl specific cases that affect ' or $( handling are rejected:
>
> "${X-"$"(cmd)}"
> $((${X-"$"(cmd)}))
>
> is cmd sub in bash, but not in other posix sh and
>
> $'\'$(cmd)'\'
>
> depends on $' support. otherwise various extensions are accepted under
> the assumption that the ' state and cmd sub are not affected by them.
>
> this patch uses a depth limited stack and 32bit counters to track the
> ' and )) states. detection of $( or ` returns WRDE_CMDSUB without
> verifying the syntax of the suffix. WRDE_BADCHAR is returned for
> rejected cases related to chars specified to be bad in wordexp other
> rejection is WRDE_SYNTAX and it is 0 if parsing completes in i state.
>
> the nocmd_check code is around 2k, the change adds about 1.2k .text,
> a bit of that can be dropped by changing T[*s] to *s<128U ? T[*s]:0.
> ---
> src/misc/wordexp.c | 224 ++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 182 insertions(+), 42 deletions(-)
>
> diff --git a/src/misc/wordexp.c b/src/misc/wordexp.c
> index e65f2381..d433858e 100644
> --- a/src/misc/wordexp.c
> +++ b/src/misc/wordexp.c
> @@ -3,6 +3,7 @@
> #include <stdio.h>
> #include <string.h>
> #include <limits.h>
> +#include <ctype.h>
> #include <stdint.h>
> #include <stdlib.h>
> #include <sys/wait.h>
> @@ -24,11 +25,187 @@ static char *getword(FILE *f)
> return getdelim(&s, (size_t [1]){0}, 0, f) < 0 ? 0 : s;
> }
>
> +static const char *next(const char *s)
> +{
> + s++;
> + while (s[0]=='\\' && s[1]=='\n')
> + s += 2;
> + return s;
> +}
> +
> +/* $c.. is a variable */
> +static int varchar(int c)
> +{
> + if (isalnum(c)) return 1;
> + switch (c) {
> + case '_':
> + case '@':
> + case '*':
> + case '#':
> + case '?':
> + case '-':
> + case '$':
> + case '!':
> + return 1;
> + }
> + return 0;
> +}
> +
> +static const char *skipvarname(const char *s)
> +{
> + while (isalnum(*s) || *s=='_')
> + s = next(s);
> + return s;
> +}
> +
> +static int nocmd_check(const char *s)
> +{
> + enum {
> + INIT=0, /* initial state */
> + QUOTE=4, /* in ".." */
> + BRACE=8, /* in ${..} */
> + PAREN=12, /* in $((..)) */
> +
> + XSQ_ESC=0, /* ' escapes in ${..} */
> + XSQ_BAD=16, /* ' is rejected in ${..} */
> + XD_OK=32, /* ' has no effect, "..$" is ok */
> + XD_BAD=48, /* ' has no effect, "..$" is rejected */
> + XMASK=48
> + };
> + enum {ROK=1, RCMD, RBAD, RSYN, ESC, SQ, SQE, DQ, D, LP, RP, POP, WS, H};
> + static const unsigned short T[256]={
> +#define C(c,opi,opq,opb,opp) [c] = opi<<INIT|opq<<QUOTE|opb<<BRACE|opp<<PAREN,
> + /* C INIT QUOTE BRACE PAREN */
> + C( 0 , ROK, RSYN, RSYN, RSYN )
> + C( '`', RCMD, RCMD, RCMD, RCMD )
> + C( '$', D, D, D, D )
> + C('\\', ESC, ESC, ESC, ESC )
> + C( '"', DQ, POP, DQ, RSYN )
> + C('\'', SQE, 0, SQ, RSYN )
> + C( '#', H, 0, 0, RSYN )
> + C( ' ', WS, 0, 0, 0 )
> + C('\t', WS, 0, 0, 0 )
> + C('\n', RBAD, 0, 0, 0 )
> + C( '|', RBAD, 0, 0, 0 )
> + C( '&', RBAD, 0, 0, 0 )
> + C( ';', RBAD, 0, 0, RBAD )
> + C( '<', RBAD, 0, 0, 0 )
> + C( '>', RBAD, 0, 0, 0 )
> + C( '(', RBAD, 0, 0, LP )
> + C( ')', RBAD, 0, 0, RP )
> + C( '{', RBAD, 0, 0, RBAD )
> + C( '}', RBAD, 0, POP, RBAD )
> +#undef C
> + };
> + const char *wstart = s;
> + enum {MAXSP=1024}; /* limit on #QUOTE+#BRACE+5*#PAREN nesting depth */
> + unsigned char stack[MAXSP];
> + /* note: ( depth in a $((..)) is limited by 32bit np and ARG_MAX */
> + unsigned np=0, sp=0, st=INIT, stx=XSQ_ESC, newst, newstx;
> +
> + for (;;s++) switch (T[(unsigned char)*s]>>st & 15) {
> + case 0: break;
> + case ROK: return 0;
> + case RCMD: return WRDE_CMDSUB;
> + case RBAD: return WRDE_BADCHAR;
> + case RSYN: return WRDE_SYNTAX;
> + case ESC:
> + if (*++s) break;
> + return WRDE_SYNTAX;
> + case SQ:
> + if (stx == XSQ_BAD) return WRDE_SYNTAX;
> + if (stx == XSQ_ESC) {
> + case SQE:
> + s = strchr(s+1, '\'');
> + if (!s) return WRDE_SYNTAX;
> + }
> + break;
> + case DQ:
> + newst = QUOTE;
> + newstx = stx == XSQ_ESC ? XD_OK : XD_BAD;
> +push:
> + if (sp > MAXSP-1) return WRDE_SYNTAX;
> + stack[sp++] = st | stx;
> + st = newst;
> + stx = newstx;
> + if (st != PAREN) break;
> + if (sp > MAXSP-4) return WRDE_SYNTAX;
> + stack[sp++] = np;
> + stack[sp++] = np>>8;
> + stack[sp++] = np>>16;
> + stack[sp++] = np>>24;
> + np = 1; /* (( counts as 1, deal with the extra at the end */
> + break;
> + case D:
> + s = next(s);
> + if (*s=='{') {
> + s = next(s);
> + if (!varchar(*s)) return WRDE_SYNTAX;
> + s = skipvarname(s+1);
> + switch (*s) {
> + case ':':
> + case '-':
> + case '+':
> + case '=':
> + case '?':
> + newstx = stx;
> + break;
> + case '#':
> + case '%':
> + newstx = XSQ_ESC;
> + break;
> + default: /* extension, simple ${var}, eof */
> + s--;
> + newstx = XSQ_BAD;
> + }
> + newst = BRACE;
> + goto push;
> + }
> + if (*s=='(') {
> + s = next(s);
> + if (*s!='(') return WRDE_CMDSUB;
> + newstx = XSQ_BAD; /* $((${v-'})) */
> + newst = PAREN;
> + goto push;
> + }
> + if (*s=='\'' || (*s=='"' && st==QUOTE && stx==XD_BAD))
> + /* reject $' and deal with "${x-"$"y}" */
> + return WRDE_SYNTAX;
> + if (!varchar(*s))
> + s--; /* assume $c is not special. */
> + break;
> + case LP:
> + np++;
> + if (!np) return WRDE_SYNTAX;
> + break;
> + case RP:
> + np--;
> + if (np) break;
> + s = next(s);
> + if (*s != ')') return WRDE_CMDSUB; /* )) mismatch */
> + np = (unsigned)stack[--sp]<<24;
> + np |= stack[--sp]<<16;
> + np |= stack[--sp]<<8;
> + np |= stack[--sp];
> + /* fallthrough */
> + case POP:
> + stx = stack[--sp];
> + st = stx&15;
> + stx &= XMASK;
> + break;
> + case WS:
> + wstart = s = next(s);
> + s--;
> + break;
> + case H:
> + if (s != wstart) break;
> + return strchr(s, '\n') ? WRDE_BADCHAR : 0;
> + }
> +}
> +
> static int do_wordexp(const char *s, wordexp_t *we, int flags)
> {
> size_t i, l;
> - int sq=0, dq=0;
> - size_t np=0;
> char *w, **tmp;
> int err = 0;
> FILE *f;
> @@ -41,46 +218,9 @@ static int do_wordexp(const char *s, wordexp_t *we, int flags)
>
> if (flags & WRDE_REUSE) wordfree(we);
>
> - if (flags & WRDE_NOCMD) for (i=0; s[i]; i++) switch (s[i]) {
> - case '\\':
> - if (!sq && !s[++i]) return WRDE_SYNTAX;
> - break;
> - case '\'':
> - if (!dq) sq^=1;
> - break;
> - case '"':
> - if (!sq) dq^=1;
> - break;
> - case '(':
> - if (np) {
> - np++;
> - break;
> - }
> - case ')':
> - if (np) {
> - np--;
> - break;
> - }
> - case '\n':
> - case '|':
> - case '&':
> - case ';':
> - case '<':
> - case '>':
> - case '{':
> - case '}':
> - if (!(sq|dq|np)) return WRDE_BADCHAR;
> - break;
> - case '$':
> - if (sq) break;
> - if (s[i+1]=='(' && s[i+2]=='(') {
> - i += 2;
> - np += 2;
> - break;
> - } else if (s[i+1] != '(') break;
> - case '`':
> - if (sq) break;
> - return WRDE_CMDSUB;
> + if (flags & WRDE_NOCMD) {
> + err = nocmd_check(s);
> + if (err) return err;
> }
>
> if (flags & WRDE_APPEND) {
> --
> 2.52.0
>
This needs to be done, but it's a lot to review. I think I'll start
with some examples the current code doesn't handle right and work
through how the proposed change fixes them, but it might take a while.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
