[PATCH v2] expand: Do not quote backslashes in unquoted parameter expansion

[Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>]

On Wed, Mar 28, 2018 at 12:03:24PM +0200, Harald van Dijk wrote:
>
> When expanded backslashes are no longer treated as quoted, this would call
> expmeta() with the pattern *\, that is with a single unquoted trailing
> backslash, so:
> 
> expand.c:1333
> 
>                         if (*p == '\\')
>                                 esc++;
>                         if (p[esc] == '/') {
> 
> The first if statement will be hit and set esc to 1. p[esc] is then '\0', so
> the second if block doesn't get entered and the outer loop continues:
> 
> expand.c:1315
> 
>         for (p = name; esc = 0, *p; p += esc + 1) {
> 
> p += esc + 1 will increase p by 2, letting it point just past the
> terminating '\0'. The loop condition of *p now accesses the byte just past
> the pattern.

Thanks for the explanation.  Here is an updated patch.

---8<---
On Mon, Mar 26, 2018 at 07:25:20PM +0200, Martijn Dekker wrote:
> Op 26-03-18 om 17:38 schreef Harald van Dijk:
> > And not by dash 0.5.4. Like I wrote, dash 0.5.5 had some bugs that were
> > fixed in 0.5.6, which mostly restored the behaviour to match <0.5.5.
> 
> Ah, sorry. dash 0.5.4 and earlier don't compile on my system, so they
> are not included in my conveniently accessible arsenal of test shells.
> 
> > As for my patches, that was by accident and doesn't work reliably. When
> > the shell sees no metacharacters, pathname expansion is bypassed, and
> > backslash isn't considered a metacharacter. Which got me to my original
> > example of /de\v: there are no metacharacters in there, so the shell
> > doesn't look to see if it matches anything. Which seems highly
> > desirable: the shell shouldn't need to hit the file system for words not
> > containing metacharacters. The only way then to get consistent behaviour
> > is if the backslash is taken as quoted, so I'm not tempted to argue for
> > the behaviour you're hoping for, sorry. :)

Here is a better example:

	a="/*/\nullx" b="/*/\null"; printf "%s\n" $a $b

dash currently prints

	/*/\nullx
	/*/\null

bash prints

	/*/\nullx
	/dev/null

You may argue the bash behaviour is inconsistent but it actually
makes sense.  What happens is that quote removal only applies to
the original token as seen by the shell.  It is never applied to
the result of parameter expansion.

Now you may ask why on earth does the second line say "/dev/null"
instead of "/dev/\null".  Well that's because it is not the quote
removal step that removed the backslash, but the pathname expansion.

The fact that the /de\v does not become /dev even though it exists
is just the result of the optimisation to avoid unnecessarily
calling stat(2).  I have checked POSIX and I don't see anything
that forbids this behaviour.

So going back to dash yes I think we should adopt the bash behaviour
for pathname expansion and keep the existing case semantics.

This patch does exactly that.  Note that this patch does not work
unless you have already applied

	https://patchwork.kernel.org/patch/10306507/

because otherwise the optimisation mentioned above does not get
detected correctly and we will end up doing quote removal twice.

This patch also updates expmeta to handle naked backslashes at
the end of the pattern which is now possible.

Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>

diff --git a/src/expand.c b/src/expand.c
index dadac56..c35366e 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -848,8 +848,7 @@ memtodest(const char *p, size_t len, const char *syntax, int quotes) {
 		if (c) {
 			if ((quotes & QUOTES_ESC) &&
 			    ((syntax[c] == CCTL) ||
-			     (((quotes & EXP_FULL) || syntax != BASESYNTAX) &&
-			      syntax[c] == CBACK)))
+			     (syntax != BASESYNTAX && syntax[c] == CBACK)))
 				USTPUTC(CTLESC, q);
 		} else if (!(quotes & QUOTES_KEEPNUL))
 			continue;
@@ -1323,7 +1322,7 @@ expmeta(char *name, unsigned name_len, unsigned expdir_len)
 				}
 			}
 		} else {
-			if (*p == '\\')
+			if (*p == '\\' && p[1])
 				esc++;
 			if (p[esc] == '/') {
 				if (metaflag)
@@ -1337,7 +1336,7 @@ expmeta(char *name, unsigned name_len, unsigned expdir_len)
 			return;
 		p = name;
 		do {
-			if (*p == '\\')
+			if (*p == '\\' && p[1])
 				p++;
 			*enddir++ = *p;
 		} while (*p++);
@@ -1349,7 +1348,7 @@ expmeta(char *name, unsigned name_len, unsigned expdir_len)
 	if (name < start) {
 		p = name;
 		do {
-			if (*p == '\\')
+			if (*p == '\\' && p[1])
 				p++;
 			*enddir++ = *p++;
 		} while (p < start);
-- 
Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html