[PATCH 6/3] Additional sanity for named references

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

I wasn't going to continue the N/3 subject scheme but since the
preceding five haven't yet been pushed to git origin I couldn't think
of a better way to indicate that they have to be applied in sequence.

This patch covers the discussion with Oliver Kiddle from the [PATCH 1/3] thread.

* Add "unset -n"
* Allow and enforce "typeset -n -r" for read-only references
* "can't change type via subscript reference" error
* Better checking for self-referential declarations/assignments
* Ksh-style "foo=bar; typeset -n foo" creates foo=bar reference
* Support "typeset -n ref; for ref in ..."
* Subscripted references use NO_EXEC for safety
* References assigned in called scopes reset scope at end
* Allow named references to $! $? $$ $- $0 $_

diff --git a/Src/builtin.c b/Src/builtin.c
index 8039b644e..cf7e9d9fe 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -126,7 +126,7 @@ static struct builtin builtins[] =
     BUILTIN("unalias", 0, bin_unhash, 0, -1, BIN_UNALIAS, "ams", NULL),
     BUILTIN("unfunction", 0, bin_unhash, 1, -1, BIN_UNFUNCTION, "m", "f"),
     BUILTIN("unhash", 0, bin_unhash, 1, -1, BIN_UNHASH, "adfms", NULL),
-    BUILTIN("unset", BINF_PSPECIAL, bin_unset, 1, -1, BIN_UNSET, "fmv", NULL),
+    BUILTIN("unset", BINF_PSPECIAL, bin_unset, 1, -1, BIN_UNSET, "fmvn", NULL),
     BUILTIN("unsetopt", 0, bin_setopt, 0, -1, BIN_UNSETOPT, NULL, NULL),
     BUILTIN("wait", 0, bin_fg, 0, -1, BIN_WAIT, NULL, NULL),
     BUILTIN("whence", 0, bin_whence, 0, -1, 0, "acmpvfsSwx:", NULL),
@@ -2034,11 +2034,16 @@ typeset_single(char *cname, char *pname, Param pm, int func,
 	if (!(off & PM_NAMEREF))
 	    pm = (Param)resolve_nameref(pm, NULL);
 	if (pm && (pm->node.flags & PM_NAMEREF) &&
-	    (on & ~(PM_NAMEREF|PM_LOCAL))) {
+	    (on & ~(PM_NAMEREF|PM_LOCAL|PM_READONLY))) {
 	    /* Changing type of PM_SPECIAL|PM_AUTOLOAD is a fatal error.  *
 	     * Should this be a fatal error as well, rather than warning? */
-	    zwarnnam(cname, "%s: can't change type of a named reference",
-		     pname);
+	    if (pm->width)
+		zwarnnam(cname,
+			 "%s: can't change type via subscript reference",
+			 pm->u.str);
+	    else
+		zwarnnam(cname, "%s: can't change type of a named reference",
+			 pname);
 	    return NULL;
 	}
     }
@@ -2223,6 +2228,11 @@ typeset_single(char *cname, char *pname, Param pm, int func,
 	    zerrnam(cname, "%s: restricted", pname);
 	    return pm;
 	}
+	if ((pm->node.flags & PM_READONLY) &&
+	    (pm->node.flags & PM_NAMEREF & off)) {
+	    zerrnam(cname, "%s: read-only reference", pname);
+	    return pm;
+	}
 	if ((on & PM_UNIQUE) && !(pm->node.flags & PM_READONLY & ~off)) {
 	    Param apm;
 	    char **x;
@@ -2659,7 +2669,7 @@ bin_typeset(char *name, char **argv, LinkList assigns, Options ops, int func)
 	    off |= bit;
     }
     if (OPT_MINUS(ops,'n')) {
-	if (on|off) {
+	if ((on & ~PM_READONLY)|off) {
 	    zwarnnam(name, "no other attributes allowed with -n");
 	    return 1;
 	}
@@ -3051,9 +3061,8 @@ bin_typeset(char *name, char **argv, LinkList assigns, Options ops, int func)
 
 	if (on & PM_NAMEREF) {
 	    if (asg->value.scalar &&
-		(strcmp(asg->name, asg->value.scalar) == 0 ||
-		 ((pm = (Param)resolve_nameref((Param)hn, asg)) &&
-		  (pm->node.flags & PM_NAMEREF)))) {
+		((pm = (Param)resolve_nameref((Param)hn, asg)) &&
+		 (pm->node.flags & PM_NAMEREF))) {
 		if (pm->node.flags & PM_SPECIAL)
 		    zwarnnam(name, "%s: invalid reference", pm->node.nam);
 		else
@@ -3063,8 +3072,12 @@ bin_typeset(char *name, char **argv, LinkList assigns, Options ops, int func)
 	    }
 	    if (hn) {
 		/* namerefs always start over fresh */
-		if (((Param)hn)->level >= locallevel)
+		if (((Param)hn)->level >= locallevel) {
+		    Param oldpm = (Param)hn;
+		    if (!asg->value.scalar && oldpm->u.str)
+			asg->value.scalar = dupstring(oldpm->u.str);
 		    unsetparam_pm((Param)hn, 0, 1);
+		}
 		hn = NULL;
 	    }
 	}
@@ -3762,7 +3775,11 @@ bin_unset(char *name, char **argv, Options ops, int func)
 			if ((!(pm->node.flags & PM_RESTRICTED) ||
 			     unset(RESTRICTED)) &&
 			    pattry(pprog, pm->node.nam)) {
-			    unsetparam_pm(pm, 0, 1);
+			    if (!OPT_ISSET(ops,'n') &&
+				(pm->node.flags & PM_NAMEREF) && pm->u.str)
+				unsetparam(pm->u.str);
+			    else
+				unsetparam_pm(pm, 0, 1);
 			    match++;
 			}
 		    }
@@ -3814,6 +3831,11 @@ bin_unset(char *name, char **argv, Options ops, int func)
 	    zerrnam(name, "%s: restricted", pm->node.nam);
 	    returnval = 1;
 	} else if (ss) {
+	    if ((pm->node.flags & PM_NAMEREF) &&
+		(!(pm = (Param)resolve_nameref(pm, NULL)) || pm->width)) {
+		/* warning? */
+		continue;
+	    }
 	    if (PM_TYPE(pm->node.flags) == PM_HASHED) {
 		HashTable tht = paramtab;
 		if ((paramtab = pm->gsu.h->getfn(pm)))
@@ -3852,8 +3874,11 @@ bin_unset(char *name, char **argv, Options ops, int func)
 		returnval = 1;
 	    }
 	} else {
-	    if ((pm = (Param)resolve_nameref(pm, NULL)) &&
-		unsetparam_pm(pm, 0, 1))
+	    if (!OPT_ISSET(ops,'n')) {
+		if (!(pm = (Param)resolve_nameref(pm, NULL)))
+		    continue;
+	    }
+	    if (unsetparam_pm(pm, 0, 1))
 		returnval = 1;
 	}
 	if (ss)
diff --git a/Src/loop.c b/Src/loop.c
index 7df379ecf..0f3847541 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -53,7 +53,7 @@ execfor(Estate state, int do_exec)
     wordcode code = state->pc[-1];
     int iscond = (WC_FOR_TYPE(code) == WC_FOR_COND), ctok = 0, atok = 0;
     int last = 0;
-    char *name, *str, *cond = NULL, *advance = NULL;
+    char *str, *cond = NULL, *advance = NULL;
     zlong val = 0;
     LinkList vars = NULL, args = NULL;
     int old_simple_pline = simple_pline;
@@ -151,7 +151,7 @@ execfor(Estate state, int do_exec)
 	    int count = 0;
 	    for (node = firstnode(vars); node; incnode(node))
 	    {
-		name = (char *)getdata(node);
+		char *name = (char *)getdata(node);
 		if (!args || !(str = (char *) ugetnode(args)))
 		{
 		    if (count) { 
@@ -165,7 +165,7 @@ execfor(Estate state, int do_exec)
 		    fprintf(xtrerr, "%s=%s\n", name, str);
 		    fflush(xtrerr);
 		}
-		setsparam(name, ztrdup(str));
+		setloopvar(name, ztrdup(str));
 		count++;
 	    }
 	    if (!count)
diff --git a/Src/params.c b/Src/params.c
index 98950d88f..4910d65fe 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -997,7 +997,7 @@ createparam(char *name, int flags)
 			 paramtab->getnode(paramtab, name));
 
 	if (oldpm && (oldpm->node.flags & PM_NAMEREF) &&
-	    !(flags & PM_NAMEREF)) {
+	    !(flags & PM_NAMEREF) && (oldpm = upscope(oldpm, oldpm->base))) {
 	    Param lastpm;
 	    struct asgment stop;
 	    stop.flags = PM_NAMEREF | (flags & PM_LOCAL);
@@ -1467,10 +1467,14 @@ getarg(char **str, int *inv, Value v, int a2, zlong *w,
     if (ishash && (keymatch || !rev))
 	remnulargs(s);
     if (needtok) {
+	char exe = opts[EXECOPT];
 	s = dupstring(s);
 	if (parsestr(&s))
 	    return 0;
+	if (flags & SCANPM_NOEXEC)
+	    opts[EXECOPT] = 0;
 	singsub(&s);
+	opts[EXECOPT] = exe;
     } else if (rev)
 	remnulargs(s);	/* This is probably always a no-op, but ... */
     if (!rev) {
@@ -2153,9 +2157,12 @@ fetchvalue(Value v, char **pptr, int bracks, int flags)
 		((pm->node.flags & PM_UNSET) &&
 		!(pm->node.flags & PM_DECLARED)))
 		return NULL;
-	    if (ss)
+	    if (ss) {
+		flags |= SCANPM_NOEXEC;
 		*ss = sav;
-	    s = dyncat(ss,*pptr);
+		s = dyncat(ss,*pptr);
+	    } else
+		s = *pptr;
 	}
 	if (PM_TYPE(pm->node.flags) & (PM_ARRAY|PM_HASHED)) {
 	    /* Overload v->isarr as the flag bits for hashed arrays. */
@@ -5782,7 +5789,8 @@ scanendscope(HashNode hn, UNUSED(int flags))
 		export_param(pm);
 	} else
 	    unsetparam_pm(pm, 0, 0);
-    }
+    } else if ((pm->node.flags & PM_NAMEREF) && pm->base > pm->level)
+	pm->base = locallevel;
 }
 
 
@@ -6109,10 +6117,10 @@ resolve_nameref(Param pm, const Asgment stop)
 	if (pm && (pm->node.flags & (PM_UNSET|PM_TAGGED))) {
 	    /* Semaphore with createparam() */
 	    pm->node.flags &= ~PM_UNSET;
-	    /* See V10private.ztst end is in scope but private:
-	    if (pm->node.flags & PM_SPECIAL)
+	    if (pm->node.flags & PM_NEWREF)	/* See setloopvar() */
 		return NULL;
-	    */
+	    if (pm->u.str && *(pm->u.str) && (pm->node.flags & PM_TAGGED))
+		pm->node.flags |= PM_SELFREF;	/* See setscope() */
 	    return (HashNode) pm;
 	} else if (pm->u.str) {
 	    if ((pm->node.flags & PM_TAGGED) ||
@@ -6157,13 +6165,29 @@ resolve_nameref(Param pm, const Asgment stop)
 	    if (pm)
 		pm->node.flags &= ~PM_TAGGED;
 	} else if (stop && (stop->flags & PM_NAMEREF))
-	    hn = (HashNode)pm;
+	    hn = (pm && (pm->node.flags & PM_NEWREF)) ? NULL : (HashNode)pm;
 	unqueue_signals();
     }
 
     return hn;
 }
 
+/**/
+mod_export void
+setloopvar(char *name, char *value)
+{
+  Param pm = (Param) gethashnode2(realparamtab, name);
+
+  if (pm && (pm->node.flags & PM_NAMEREF)) {
+      pm->base = pm->width = 0;
+      pm->u.str = ztrdup(value);
+      pm->node.flags |= PM_NEWREF;
+      setscope(pm);
+      pm->node.flags &= ~PM_NEWREF;
+  } else
+      setsparam(name, value);
+}
+
 /**/
 static void
 setscope(Param pm)
@@ -6188,9 +6212,50 @@ setscope(Param pm)
 	    pm->width = t - pm->u.str;
 	    *t = '[';
 	}
-	if (basepm)
-	    pm->base = ((basepm->node.flags & PM_NAMEREF) ?
-			basepm->base : basepm->level);
+	if (basepm) {
+	    if (basepm->node.flags & PM_NAMEREF) {
+		if (pm == basepm) {
+		    if (pm->node.flags & PM_SELFREF) {
+			/* Loop signalled by resolve_nameref() */
+			if (upscope(pm, pm->base) == pm) {
+			    zerr("%s: invalid self reference", pm->u.str);
+			    unsetparam_pm(pm, 0, 1);
+			    return;
+			}
+			pm->node.flags &= ~PM_SELFREF;
+		    } else if (pm->base == pm->level) {
+			if (pm->u.str && *(pm->u.str) &&
+			    strcmp(pm->node.nam, pm->u.str) == 0) {
+			    zerr("%s: invalid self reference", pm->u.str);
+			    unsetparam_pm(pm, 0, 1);
+			    return;
+			}
+		    }
+		} else if (basepm->u.str) {
+		    if (basepm->base <= basepm->level &&
+			strcmp(pm->node.nam, basepm->u.str) == 0) {
+			zerr("%s: invalid self reference", pm->u.str);
+			unsetparam_pm(pm, 0, 1);
+			return;
+		    }
+		}
+	    } else
+		pm->base = basepm->level;
+	}
+	if (pm->base > pm->level) {
+	    if (EMULATION(EMULATE_KSH)) {
+		zerr("%s: global reference cannot refer to local variable",
+		      pm->node.nam);
+		unsetparam_pm(pm, 0, 1);
+	    } else if (isset(WARNNESTEDVAR))
+		zwarn("%s: global reference to local variable: %s",
+		      pm->node.nam, pm->u.str);
+	}
+	if (pm->u.str && upscope(pm, pm->base) == pm &&
+	    strcmp(pm->node.nam, pm->u.str) == 0) {
+	    zerr("%s: invalid self reference", pm->u.str);
+	    unsetparam_pm(pm, 0, 1);
+	}
     }
 }
 
@@ -6217,7 +6282,10 @@ valid_refname(char *val)
 	if (*t == '[') {
 	    tokenize(t = dupstring(t+1));
 	    t = parse_subscript(t, 0, ']');
-	} else {
+	} else if (t[1] || !(*t == '!' || *t == '?' ||
+			     *t == '$' || *t == '-' ||
+			     *t == '0' || *t == '_')) {
+	    /* Skipping * @ # because of doshfunc() implementation */
 	    t = NULL;
 	}
     }
diff --git a/Src/zsh.h b/Src/zsh.h
index 1e35bd33e..96b4b06bd 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1935,6 +1935,9 @@ struct tieddata {
 #define PM_NAMEDDIR     (1<<29) /* has a corresponding nameddirtab entry    */
 #define PM_NAMEREF      (1<<30) /* pointer to a different parameter         */
 
+#define PM_SELFREF	PM_UNIQUE	/* Overload when namerefs resolved  */
+#define PM_NEWREF	PM_SINGLE	/* Overload in for-loop namerefs    */
+
 /* The option string corresponds to the first of the variables above */
 #define TYPESET_OPTSTR "aiEFALRZlurtxUhHT"
 
@@ -1959,6 +1962,7 @@ struct tieddata {
 				  * elements
 				  */
 #define SCANPM_CHECKING   (1<<10) /* Check if set, no need to create */
+#define SCANPM_NOEXEC     (1<<11) /* No command substitutions, etc. */
 /* "$foo[@]"-style substitution
  * Only sign bit is significant
  */