diff mbox series

[v2] expand: Fix multiple issues with EXP_DISCARD in evalvar

Message ID 20180912062716.boa32pwabi6fpaal@gondor.apana.org.au (mailing list archive)
State Accepted
Delegated to: Herbert Xu
Headers show
Series [v2] expand: Fix multiple issues with EXP_DISCARD in evalvar | expand

Commit Message

Herbert Xu Sept. 12, 2018, 6:27 a.m. UTC 
On Fri, Sep 07, 2018 at 06:55:29AM +0200, Martijn Dekker wrote:
>
> But now I'm encountering another, similar bug, that was a bit harder to
> track down:
> 
> $ src/dash -c 'foo=bar; echo ${foo=BUG}; echo $foo'
> barBUG
> bar
> $ src/dash -c 'foo=bar; echo ${foo:=BUG}; echo $foo'
> barBUG
> bar
> 
> Expected output: 'bar' twice in both cases. The ${foo=BUG} and ${foo:=BUG}
> expansions fail to discard the word 'BUG' if foo is set.

Thanks for the update.  In this case we didn't call the parsing
function subevalvar at all when we should have called it with
EXP_DISCARD.

As patchwork was having issues I've rolled all three patches into
one.

Cheers,

---8<---
The commit 3cd538634f71538370f5af239f342aec48b7470b broke parameter
expansion in multiple ways because the EXP_DISCARD flag wasn't set
or tested for various cases:

	$ src/dash -c 'var=; echo ${var:+nonempty}'
	nonempty
        $ src/dash -u -c 'unset foo bar; echo ${foo+${bar}}'
        dash: 1: bar: parameter not set
        $ src/dash -c 'foo=bar; echo ${foo=BUG}; echo $foo'
        barBUG
        bar
        $

This patch fixes them by introducing a new discard variable that
tracks whether the extra word should be discarded or not when it
is parsed.

Reported-by: Martijn Dekker <martijn@inlv.org>
Fixes: 3cd538634f71 ("expand: Do not reprocess data when...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Comments

Martijn Dekker Nov. 14, 2018, 10:15 p.m. UTC | #1 
I encountered another bug, introduced by 3cd5386 and not fixed by v2 of 
this patch: the presence of a length-counting expansion like ${#foo} in 
a string causes the rest of the string to be discarded.

$ src/dash -c 'foo=bar; echo "baz ${#foo} quux"'
baz 3
$ src/dash -c 'foo=bar; echo baz\ ${#foo}\ quux'
baz 3

(expected outout: baz 3 quux)

- Martijn
Herbert Xu Nov. 19, 2018, 5:54 a.m. UTC | #2 
On Wed, Nov 14, 2018 at 11:15:36PM +0100, Martijn Dekker wrote:
> I encountered another bug, introduced by 3cd5386 and not fixed by v2 of this
> patch: the presence of a length-counting expansion like ${#foo} in a string
> causes the rest of the string to be discarded.
> 
> $ src/dash -c 'foo=bar; echo "baz ${#foo} quux"'
> baz 3
> $ src/dash -c 'foo=bar; echo baz\ ${#foo}\ quux'
> baz 3
> 
> (expected outout: baz 3 quux)

Thanks.  This is an unrelated issue where we fail to eat the
closing brace for $# expansion.  This causes the outer argstr
to termiante prematurely.

---8<---
expand: Eat closing brace for length parameter expansion

When we are doing VSLENGTH expansion, the closing brace is currently
not removed in evalvar.  This causes the caller argstr to terminate
prematurely as it would interpret the closing brace as one that
belongs to a parameter expansion at the outer level.

This patch fixes it.

Reported-by: Martijn Dekker <martijn@inlv.org>
Fixes: 3cd538634f71 ("expand: Do not reprocess data when...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/src/expand.c b/src/expand.c
index 856b7a9..af9cac9 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -745,6 +745,7 @@ again:
 		varunset(p, var, 0, 0);
 
 	if (subtype == VSLENGTH) {
+		p++;
 		if (flag & EXP_DISCARD)
 			return p;
 		cvtnum(varlen > 0 ? varlen : 0, flag);
diff mbox series

Patch

diff --git a/src/expand.c b/src/expand.c
index 14daa63..856b7a9 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -698,6 +698,7 @@  evalvar(char *p, int flag)
 	int patloc;
 	int startloc;
 	ssize_t varlen;
+	int discard;
 	int quoted;
 
 	varflags = *p++;
@@ -713,41 +714,41 @@  again:
 	if (varflags & VSNUL)
 		varlen--;
 
+	discard = varlen < 0 ? EXP_DISCARD : 0;
+
 	switch (subtype) {
 	case VSPLUS:
-		varlen = -1 - varlen;
+		discard ^= EXP_DISCARD;
 		/* fall through */
 
 	case 0:
 	case VSMINUS:
-		p = argstr(p, flag | EXP_TILDE | EXP_WORD);
-		if (varlen < 0)
-			return p;
+		p = argstr(p, flag | EXP_TILDE | EXP_WORD |
+			      (discard ^ EXP_DISCARD));
 		goto record;
 
 	case VSASSIGN:
 	case VSQUESTION:
-		if (varlen >= 0)
-			goto record;
-
 		p = subevalvar(p, var, 0, startloc, varflags,
-			       flag & ~QUOTES_ESC);
+			       (flag & ~QUOTES_ESC) |
+			       (discard ^ EXP_DISCARD));
 
-		if (flag & EXP_DISCARD)
-			return p;
+		if ((flag | ~discard) & EXP_DISCARD)
+			goto record;
 
 		varflags &= ~VSNUL;
+		subtype = VSNORMAL;
 		goto again;
 	}
 
-	if (varlen < 0 && uflag)
+	if ((discard & ~flag) && uflag)
 		varunset(p, var, 0, 0);
 
 	if (subtype == VSLENGTH) {
 		if (flag & EXP_DISCARD)
 			return p;
 		cvtnum(varlen > 0 ? varlen : 0, flag);
-		goto record;
+		goto really_record;
 	}
 
 	if (subtype == VSNORMAL)
@@ -765,7 +766,7 @@  again:
 	}
 #endif
 
-	flag |= varlen < 0 ? EXP_DISCARD : 0;
+	flag |= discard;
 	if (!(flag & EXP_DISCARD)) {
 		/*
 		 * Terminate the string and start recording the pattern
@@ -778,9 +779,10 @@  again:
 	p = subevalvar(p, NULL, patloc, startloc, varflags, flag);
 
 record:
-	if (flag & EXP_DISCARD)
+	if ((flag | discard) & EXP_DISCARD)
 		return p;
 
+really_record:
 	if (quoted) {
 		quoted = *var == '@' && shellparam.nparam;
 		if (!quoted)