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

[Bart Schaefer <schaefer@brasslantern.com>]

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)