[PATCH] local vs. nameref scoping (was Re: Up-scope named references, vs. ksh)

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Mon, Mar 4, 2024 at 9:43 PM Mikael Magnusson <mikachu@xxxxxxxxx> wrote:
>
> On 3/5/24, Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
> >
> >  When a _named reference_ is created with 'typeset -n', all uses of PNAME
> >  in assignments and expansions instead assign to or expand RNAME.  This
> >  also applies to 'unset PNAME' and to most subsequent uses of 'typeset'
> >  with the exception of 'typeset -n' and 'typeset +n'
>
> Is this possible to change? I feel like if "typeset myvar" (or "local
> myvar") cannot be depended on to create a local parameter, a lot of
> code will no longer be safe that previously was (in the sense that it
> doesn't break if calling code / the shell environmnet has certain
> parameters defined).

Yes, it's possible, and the attached patch does so, with doc update
and new tests.

The division of labor among bin_typeset(), typeset_single(), and
createparam() is a bit hard to follow (as in, it's not really much of
a "division" at all).

On Mon, Mar 4, 2024 at 10:31 PM Stephane Chazelas <stephane@xxxxxxxxxxxx> wrote:
>
> See also:
>
> $ nameref action=PS1
> $ zmv -n '*' '$f.back'
> zsh: segmentation fault  ./Src/zsh

A simpler reproducer:

% typeset -n foo=PS1
% () { local foo; foo=xx }
==1113679== Invalid read of size 8
==1113679==    at 0x1A16E1: assignstrvalue (params.c:2684)

This patch fixes that, and one other potential crash that valgrind
complains about:

% typeset -n foo=bar
% typeset -n foo
==1113695== Invalid read of size 4
==1113695==    at 0x136041: bin_typeset (builtin.c:3137)

I'll go ahead and push this since there doesn't seem to be any
argument about the change in function and it fixes two crash bugs.

diff --git a/Doc/Zsh/params.yo b/Doc/Zsh/params.yo
index 8c5e67e70..d179a0d1d 100644
--- a/Doc/Zsh/params.yo
+++ b/Doc/Zsh/params.yo
@@ -670,8 +670,9 @@ This manual was generated with Zsh tt(version()).
 When a em(named reference) is created with `tt(typeset -n)', all uses
 of var(pname) in assignments and expansions instead assign to or
 expand var(rname).  This also applies to `tt(unset )var(pname)' and to
-most subsequent uses of `tt(typeset)' with the exception of
-`tt(typeset -n)' and `tt(typeset +n)', so to remove a named reference,
+most subsequent uses of `tt(typeset)' with the exceptions of declaring
+a local in a called function, or updating a current-scope parameter with
+`tt(typeset -n)' or `tt(typeset +n)'. Thus to remove a named reference,
 use either `tt(unset -n )var(pname)' (preferred) or one of:
 ifzman()
 example(tt(typeset -n )var(pname=)
diff --git a/Src/builtin.c b/Src/builtin.c
index 6f98990f9..829b899f8 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2030,11 +2030,10 @@ typeset_single(char *cname, char *pname, Param pm, int func,
     int usepm, tc, keeplocal = 0, newspecial = NS_NONE, readonly, dont_set = 0;
     char *subscript;
 
-    if (pm && (pm->node.flags & PM_NAMEREF) && !((off|on) & PM_NAMEREF)) {
-	if (!(off & PM_NAMEREF)) {
-	    if ((pm = (Param)resolve_nameref(pm, NULL)))
-		pname = pm->node.nam;
-	}
+    if (pm && (pm->node.flags & PM_NAMEREF) && !((off|on) & PM_NAMEREF) &&
+	(pm->level == locallevel || !(on & PM_LOCAL))) {
+	if ((pm = (Param)resolve_nameref(pm, NULL)))
+	    pname = pm->node.nam;
 	if (pm && (pm->node.flags & PM_NAMEREF) &&
 	    (on & ~(PM_NAMEREF|PM_LOCAL|PM_READONLY))) {
 	    /* Changing type of PM_SPECIAL|PM_AUTOLOAD is a fatal error.  *
@@ -3125,8 +3124,10 @@ bin_typeset(char *name, char **argv, LinkList assigns, Options ops, int func)
 			oldpm->u.str)
 			asg->value.scalar = dupstring(oldpm->u.str);
 		    /* Defer read-only error to typeset_single() */
-		    if (!(hn->flags & PM_READONLY))
+		    if (!(hn->flags & PM_READONLY)) {
 			unsetparam_pm(oldpm, 0, 1);
+			hn = NULL;
+		    }
 		}
 		/* Passing a NULL pm to typeset_single() makes the
 		 * nameref read-only before assignment, which breaks
@@ -3134,7 +3135,7 @@ bin_typeset(char *name, char **argv, LinkList assigns, Options ops, int func)
 		 * so this is special-cased to permit that action
 		 * like assign-at-create for other parameter types.
 		 */
-		if (!(hn->flags & PM_READONLY))
+		if (hn && !(hn->flags & PM_READONLY))
 		    hn = NULL;
 	    }
 	}
diff --git a/Src/params.c b/Src/params.c
index 4bcf41c22..973df3fe5 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -1034,7 +1034,8 @@ createparam(char *name, int flags)
 	}
 
 	if (oldpm && !(flags & PM_NAMEREF) &&
-	    (!(oldpm->node.flags & PM_RO_BY_DESIGN) || !(flags & PM_LOCAL)) &&
+	    (oldpm->level == locallevel ?
+	     !(oldpm->node.flags & PM_RO_BY_DESIGN) : !(flags & PM_LOCAL)) &&
 	    (oldpm->node.flags & PM_NAMEREF) &&
 	    (oldpm = upscope(oldpm, oldpm->base))) {
 	    Param lastpm;
diff --git a/Test/K01nameref.ztst b/Test/K01nameref.ztst
index e45b922e2..bb0d11821 100644
--- a/Test/K01nameref.ztst
+++ b/Test/K01nameref.ztst
@@ -51,9 +51,19 @@
 0:remove nameref attribute
 >typeset ptr=var
 
-  typeset -n ptr
-  typeset -t ptr
-  typeset -p ptr
+ typeset -n ptr=gvar
+ () {
+   local ptr
+   typeset -p ptr
+ }
+ typeset -p ptr
+0:Local non-reference hides outside reference
+>typeset ptr
+>typeset -n ptr=gvar
+
+ typeset -n ptr
+ typeset -t ptr
+ typeset -p ptr
 0:change type of a placeholder
 F:Other type changes are fatal errors, should this also be?
 >typeset -n ptr=''
@@ -845,4 +855,39 @@ F:previously this could create an infinite recursion and crash
 1:create nameref by pattern match not allowed
 *?*typeset:1: invalid reference
 
+#
+# The following tests are run in interactive mode, using PS1 as an
+# assignable special with side-effects.  This crashed at one time.
+#
+
+ # Note bypassing TYPESET_TO_UNSET here
+ $ZTST_testdir/../Src/zsh -fis <<<$'
+ typeset -n p=PS1
+ () {
+  typeset -p p
+  local p
+  typeset -p p
+  p=xx
+  typeset -p p
+ }
+ '
+0:regression: assign to local that shadows global named reference
+>typeset -g -n p=PS1
+>typeset p=''
+>typeset p=xx
+*?*
+
+ # Note bypassing TYPESET_TO_UNSET here
+ $ZTST_testdir/../Src/zsh -fis <<<$'
+ () {
+   typeset p=PS1
+   typeset -n p
+   p=zz
+ }
+ typeset -p PS1
+ '
+0:regression - converting a string into a named reference
+>typeset PS1=zz
+*?*
+
 %clean