[bug] sh: tilde expansion after field splitting

[bug] sh: tilde expansion after field splitting

[Martijn Dekker]

POSIX says tilde expansion should be done before parameter expansion and
certainly before field spplitting (shwordsplit):
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06

zsh did this correctly up to version 5.0.8; as of 5.1, it appears to do
tilde expansion *after* field splitting, and only from the second field on.

Test script:

emulate sh
v='~/one ~/two ~/three'
printf '[%s]\n' $v

Actual output:

[~/one]
[/Users/martijn/two]
[/Users/martijn/three]

Expected output:

[~/one]
[~/two]
[~/three]

Thanks,

- M.

Re: [bug] sh: tilde expansion after field splitting

[Bart Schaefer]

On Oct 5, 12:20am, Martijn Dekker wrote:
}
} POSIX says tilde expansion should be done before parameter expansion [...]
} zsh did this correctly up to version 5.0.8; as of 5.1, it appears to do
} tilde expansion *after* field splitting, and only from the second field on.

This is a change in the effects of SH_FILE_EXPANSION when an array is
synthesized by word splitting.

Traced this to here:

diff --git a/Src/subst.c b/Src/subst.c
index 81d34d2..021d234 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -3834,8 +3834,14 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags)
                y = dupstring(nulstring);
            insertlinknode(l, n, (void *) y), incnode(n);
        }
-       if (eval)
-           n = on;
+       /* This used to omit restoring of *str and instead test
+        *   if (eval)
+        *       n = on;
+        * but that causes strange behavior of history modifiers when
+        * applied across all values of an array.  What is magic about
+        * eval here that *str seemed not to need restoring?
+        */
+       *str = getdata(n = on);
     } else {
        /*
         * Scalar value.  Handle last minute transformations

Apparently the answer to the question in that comment has something to
do with shfileexpansion.  Anyone have additional clues?

Re: [bug] sh: tilde expansion after field splitting

[Bart Schaefer]

On Oct 5, 12:20am, Martijn Dekker wrote:
} Subject: [bug] sh: tilde expansion after field splitting
}
} POSIX says tilde expansion should be done before parameter expansion [...]
} zsh did this correctly up to version 5.0.8; as of 5.1, it appears to do
} tilde expansion *after* field splitting, and only from the second field on.

The patch below fixes this, I believe.  Several comments:

- There either isn't a Test/ for the keyvalpairelement() case in the
  first hunk below, or it isn't rigorous enough, because I initially
  forgot the incnode(node) in that hunk, yet the shell did *not* go
  into an infinite loop during "make check", nor did any test fail.

- The patch covers two bugs for the price of one: unqueue_signals()
  near the end of the first hunk was missed when keyvalpairelement()
  was added (that's the only place at which errflag could become set
  at that point in the loop, and only when keyvalpairelemnt() also
  returns false).  I'm guessing a test for that failure is needed?

- Should we be testing isset(SHFILEEXPANSION) directly here, or ought
  it instead be [for example] passed in the flags?  Is it possible
  that stringsubst() [second hunk] could toggle the setopt so that
  the isset() in the third hunk inverts sense?  Of course if that IS
  possible, then the ultimate effect might be the expected one, and
  this point is moot.

- Grepping Test/* doesn't find anything for SH_FILE_EXPANSION (in
  upper/lower, with/without underscores, etc.).  Did I miss it?
  Does the test for the case in this thread belong in D04parameter
  or E01options?


diff --git a/Src/subst.c b/Src/subst.c
index 2d3eeb2..8c290cc 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -106,15 +106,20 @@ prefork(LinkList list, int flags, int *ret_flags)
 	ret_flags = &ret_flags_local; /* will be discarded */
 
     queue_signals();
-    for (node = firstnode(list); node; incnode(node)) {
+    node = firstnode(list);
+    while (node) {
+	LinkNode nextnode = 0;
 	if ((flags & (PREFORK_SINGLE|PREFORK_ASSIGN)) == PREFORK_ASSIGN &&
 	    (insnode = keyvalpairelement(list, node))) {
 	    node = insnode;
+	    incnode(node);
 	    *ret_flags |= PREFORK_KEY_VALUE;
 	    continue;
 	}
-	if (errflag)
+	if (errflag) {
+	    unqueue_signals();
 	    return;
+	}
 	if (isset(SHFILEEXPANSION)) {
 	    /*
 	     * Here and below we avoid taking the address
@@ -132,6 +137,12 @@ prefork(LinkList list, int flags, int *ret_flags)
 	     * testing if cptr changed...
 	     */
 	    setdata(node, cptr);
+	    /*
+	     * Advance now because we must not expand filenames again
+	     * after string substitution (which may insert new nodes).
+	     */
+	    nextnode = node;
+	    incnode(nextnode);
 	}
 	if (!(node = stringsubst(list, node,
 				 flags & ~(PREFORK_TYPESET|PREFORK_ASSIGN),
@@ -139,6 +150,10 @@ prefork(LinkList list, int flags, int *ret_flags)
 	    unqueue_signals();
 	    return;
 	}
+	if (isset(SHFILEEXPANSION))
+	    node = nextnode;
+	else
+	    incnode(node);
     }
     for (node = firstnode(list); node; incnode(node)) {
 	if (node == stop)

Re: [bug] sh: tilde expansion after field splitting

[Peter Stephenson]

On Sun, 8 Oct 2017 00:53:46 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Oct 5, 12:20am, Martijn Dekker wrote:
> } Subject: [bug] sh: tilde expansion after field splitting
> }
> } POSIX says tilde expansion should be done before parameter expansion [...]
> } zsh did this correctly up to version 5.0.8; as of 5.1, it appears to do
> } tilde expansion *after* field splitting, and only from the second field on.
> 
> The patch below fixes this, I believe.

Good, it sounds like the effect of the chunk you previous identified as
related was simply moving the linked list node on as a side effect, or
something like that.

> - There either isn't a Test/ for the keyvalpairelement() case in the
>   first hunk below, or it isn't rigorous enough, because I initially
>   forgot the incnode(node) in that hunk, yet the shell did *not* go
>   into an infinite loop during "make check", nor did any test
>   fail

That doesn't make sense.  This is the only place where key / value pairs
are handled and they require some sort of loop increment to work at all.
You're basically claiming they only work by magic.

> - Should we be testing isset(SHFILEEXPANSION) directly here, or ought
>   it instead be [for example] passed in the flags?  Is it possible
>   that stringsubst() [second hunk] could toggle the setopt so that
>   the isset() in the third hunk inverts sense?  Of course if that IS
>   possible, then the ultimate effect might be the expected one, and
>   this point is moot.

I can't believe this is a big deal.

> - Grepping Test/* doesn't find anything for SH_FILE_EXPANSION (in
>   upper/lower, with/without underscores, etc.).  Did I miss it?
>   Does the test for the case in this thread belong in D04parameter
>   or E01options?

Assuming SH_FILE_EXPANSION predates the test suite, I wouldn't be
surprised if it's missing.

pws

Re: [bug] sh: tilde expansion after field splitting

[Bart Schaefer]

On Oct 8,  8:20pm, Peter Stephenson wrote:
}
} Good, it sounds like the effect of the chunk you previous identified as
} related was simply moving the linked list node on as a side effect, or
} something like that.

Yes, prior to that previous chunk when stringsubst() did a shwordsplit
the newly created nodes were not properly linked into the original list
of arguments, which is why e.g. mapping history modifiers over the args
didn't work correctly.  Adding them to the list fixed that, but caused
all but the first to be examined twice for SHFILEEXPANSION.

} > - There either isn't a Test/ for the keyvalpairelement() case in the
} >   first hunk below, or it isn't rigorous enough, because I initially
} >   forgot the incnode(node) in that hunk, yet the shell did *not* go
} >   into an infinite loop during "make check", nor did any test
} >   fail
} 
} That doesn't make sense.

I don't know what to tell you; I just now commented it back out and tried
again, and it still works.  I suspect what's happening is that after the
first call to keyvalpairelement() node = insnode is done, and then we
continue around the loop and call keyvalpairelement() a second time on
the same node, at which point it fails and we fall through the rest of
the loop and hit the incnode() at the end, which fixes things.

} > - Grepping Test/* doesn't find anything for SH_FILE_EXPANSION (in
} >   upper/lower, with/without underscores, etc.).  Did I miss it?
} >   Does the test for the case in this thread belong in D04parameter
} >   or E01options?
} 
} Assuming SH_FILE_EXPANSION predates the test suite, I wouldn't be
} surprised if it's missing.

I looked at E01options and found a test there.  It's not quite enough,
obviously, so here's a new one.


diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index 8101ff5..6929f51 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -1026,6 +1026,18 @@
 >$lspath $lspath =
 >$lspath
 
+  () {
+    emulate -L sh
+    v='~/one ~/two'
+    print -l -- $v $v
+  }
+0:SH_FILE_EXPANSION option with GLOB_SUBST et al.
+F:Regression test for workers/41811
+>~/one
+>~/two
+>~/one
+>~/two
+
   testpat() {
     if [[ $1 = ${~2} ]]; then print $1 $2 yes; else print $1 $2 no; fi
   }

Re: [bug] sh: tilde expansion after field splitting

[Peter Stephenson]

On Sun, 8 Oct 2017 13:45:14 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> } > - There either isn't a Test/ for the keyvalpairelement() case in the
> } >   first hunk below, or it isn't rigorous enough, because I initially
> } >   forgot the incnode(node) in that hunk, yet the shell did *not* go
> } >   into an infinite loop during "make check", nor did any test
> } >   fail
> } 
> } That doesn't make sense.
> 
> I don't know what to tell you; I just now commented it back out and tried
> again, and it still works.  I suspect what's happening is that after the
> first call to keyvalpairelement() node = insnode is done, and then we
> continue around the loop and call keyvalpairelement() a second time on
> the same node, at which point it fails and we fall through the rest of
> the loop and hit the incnode() at the end, which fixes things.

Yes, that's it --- the key / value pair is turned into a Marker, key,
value list triad, and furthermore the key and value are untokenised as
no more substitution is wanted.  So it'll just go striaght through.
This doesn't indicate a problem.

pws

Re: [bug] sh: tilde expansion after field splitting

[Martijn Dekker]

Op 08-10-17 om 09:53 schreef Bart Schaefer:
> On Oct 5, 12:20am, Martijn Dekker wrote:
> } Subject: [bug] sh: tilde expansion after field splitting
> }
> } POSIX says tilde expansion should be done before parameter expansion [...]
> } zsh did this correctly up to version 5.0.8; as of 5.1, it appears to do
> } tilde expansion *after* field splitting, and only from the second field on.
> 
> The patch below fixes this, I believe.  Several comments:

This introduces a bug with "$@", $@ and $* expansion:

$ Src/zsh -o SHFILEEXPANSION -c 'set "a" "b"; printf "[%s]\n" "$@$@"'
[a]
[b$@]

$ Src/zsh -o SHFILEEXPANSION -c 'set "a" "b"; printf "[%s]\n" $@$@'
[a]
[b$@]

$ zsh -o SHFILEEXPANSION -c 'set "a" "b"; printf "[%s]\n" $*$*'
zsh:1: no matches found: b$*

Expected output for all three:
[a]
[ba]
[b]

Thanks,

- M.

Re: [bug] sh: tilde expansion after field splitting

[Peter Stephenson]

On Fri, 13 Oct 2017 11:43:17 +0200
Martijn Dekker <martijn@inlv.org> wrote:
> Op 08-10-17 om 09:53 schreef Bart Schaefer:
> > On Oct 5, 12:20am, Martijn Dekker wrote:
> > } Subject: [bug] sh: tilde expansion after field splitting
> > }
> > } POSIX says tilde expansion should be done before parameter expansion [...]
> > } zsh did this correctly up to version 5.0.8; as of 5.1, it appears to do
> > } tilde expansion *after* field splitting, and only from the second field on.
> > 
> > The patch below fixes this, I believe.  Several comments:
> 
> This introduces a bug with "$@", $@ and $* expansion:
> 
> $ Src/zsh -o SHFILEEXPANSION -c 'set "a" "b"; printf "[%s]\n" "$@$@"'
> [a]
> [b$@]
> 
> $ Src/zsh -o SHFILEEXPANSION -c 'set "a" "b"; printf "[%s]\n" $@$@'
> [a]
> [b$@]
> 
> $ zsh -o SHFILEEXPANSION -c 'set "a" "b"; printf "[%s]\n" $*$*'
> zsh:1: no matches found: b$*

Would it be easier to do something like this?  I don't think efficiency
is much of an issue here.

pws

diff --git a/Src/subst.c b/Src/subst.c
index 8c290cc..d027e3d 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -108,7 +108,6 @@ prefork(LinkList list, int flags, int *ret_flags)
     queue_signals();
     node = firstnode(list);
     while (node) {
-	LinkNode nextnode = 0;
 	if ((flags & (PREFORK_SINGLE|PREFORK_ASSIGN)) == PREFORK_ASSIGN &&
 	    (insnode = keyvalpairelement(list, node))) {
 	    node = insnode;
@@ -137,23 +136,31 @@ prefork(LinkList list, int flags, int *ret_flags)
 	     * testing if cptr changed...
 	     */
 	    setdata(node, cptr);
-	    /*
-	     * Advance now because we must not expand filenames again
-	     * after string substitution (which may insert new nodes).
-	     */
-	    nextnode = node;
-	    incnode(nextnode);
-	}
-	if (!(node = stringsubst(list, node,
-				 flags & ~(PREFORK_TYPESET|PREFORK_ASSIGN),
-				 ret_flags, asssub))) {
-	    unqueue_signals();
-	    return;
 	}
-	if (isset(SHFILEEXPANSION))
-	    node = nextnode;
 	else
-	    incnode(node);
+	{
+	    if (!(node = stringsubst(list, node,
+				     flags & ~(PREFORK_TYPESET|PREFORK_ASSIGN),
+				     ret_flags, asssub))) {
+		unqueue_signals();
+		return;
+	    }
+	}
+	incnode(node);
+    }
+    if (isset(SHFILEEXPANSION)) {
+	/*
+	 * stringsubst() may insert new nodes, so doesn't work
+	 * well in the same loop as file expansion.
+	 */
+	for (node = firstnode(list); node; incnode(node)) {
+	    if (!(node = stringsubst(list, node,
+				     flags & ~(PREFORK_TYPESET|PREFORK_ASSIGN),
+				     ret_flags, asssub))) {
+		unqueue_signals();
+		return;
+	    }
+	}
     }
     for (node = firstnode(list); node; incnode(node)) {
 	if (node == stop)
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index 6929f51..0f6bb34 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -1038,6 +1038,16 @@ F:Regression test for workers/41811
 >~/one
 >~/two
 
+  (
+    setopt shfileexpansion
+    set -- also appearing
+    print -l $*$*
+  )
+0:SH_FILE_EXPANSION interaction with inserting nodes from parameters
+>also
+>appearingalso
+>appearing
+
   testpat() {
     if [[ $1 = ${~2} ]]; then print $1 $2 yes; else print $1 $2 no; fi
   }

Re: [bug] sh: tilde expansion after field splitting

[Bart Schaefer]

On Fri, Oct 13, 2017 at 5:55 AM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
>
> Would it be easier to do something like this?  I don't think efficiency
> is much of an issue here.

Yes, I think this is a viable approach.