Honour attributes for local assignments preceding certain commands

Reproducer:

    $ typeset -i NUM=123
    $ (NUM=3+4 env; :)|grep ^NUM=
    NUM=3+4
    $ (NUM=3+4 env)|grep ^NUM=
    NUM=7

The correct output is NUM=7. This is also the output if ksh is
compiled with SHOPT_SPAWN disabled, suggesting that the problem is
here in sh_ntfork(), where the invocation-local environment list is
set using a sh_scope() call:

src/cmd/ksh93/sh/xec.c:
3496:	if(t->com.comset)
3497:	{
3498:		scope++;
3499:		sh_scope(t->com.comset,0);
3500:	}

Analysis:

When ksh is compiled with SHOPT_SPAWN disabled, external commands
are always run using the regular forking mechanism. First the shell
forks, then in the fork, the preceding assignments list (if any)
are executed and exported in the main scope. Replacing global
variables is not a problem as the variables are exported and the
forked shell is then replaced by the external command using
execve(2).

But when using SHOPT_SPAWN/sh_ntfork(), this cannot be done as the
fork(2) use is replaced by posix_spawn(2) which does not copy the
parent process into the child, therefore it's not possible to
execute anything in the child before calling execve(2). Which means
the preceding assignments list must be processed in the parent, not
the child. Which makes overwriting global variables a no-no.

To avoid overwriting global variables, sh_ntfork() treats preceding
assignments like local variables in functions, which means they do
not inherit any attributes from the parent scope. That is why the
integer attribute is not honoured in the buggy reproducers.

And this is not just an issue for external commands. sh_scope() is
also used for assignments preceding a built-in command. Which is
logical, as those don't create a process at all.

src/cmd/ksh93/sh/xec.c:
1325:	if(argp)
1326:	{
1327:		scope++;
1328:		sh_scope(argp,0);
1329:	}

Which means this bug exists for them as well, regardless of whether
SHOPT_SPAWN is compiled in.

$ /bin/ksh -c 'typeset -i NUM; NUM=3+4 command eval '\''echo $NUM'\'
3+4
(expected: 7, as on mksh and zsh)

So, the attributes from the parent scope will need to be copied
into the child scope. This should be done in nv_setlist() which is
called from sh_scope() with both the NV_EXPORT and NV_NOSCOPE flags
passed. Those flag bits are how we can recognise the need to copy
attributes.

Commit f6bc5c0 fixed a similar inconsistency with the check for the
read-only attribute. In fact, the bug fixed there was simply a
specific instance of this bug. The extra check for readonly was
because the readonly attribute was not copied to the temporary
local scope. So that fix is now replaced by the more general fix
for this bug.

src/cmd/ksh93/sh/name.c: nv_setlist():
- Introduce a 'vartree' local variable to avoid repetitive
  'sh.prefix_root ? sh.prefix_root : sh.var_tree' expressions.
- Use the NV_EXPORT|NV_NOSCOPE flag combination to check if parent
  scope attributes need to be copied to the temporary local scope
  of an assignment preceding a command.
- If so, copy everything but the value itself: attributes (nvflag),
  size parameter (nvsize), discipline function pointer (nvfun) and
  the name pointer (nvname). The latter is needed because some
  code, at least put_lang() in init.c, compares names by comparing
  the pointers instead of the strings; copying the nvname pointer
  avoids a regression in tests/locale.sh.

src/cmd/ksh93/sh/xec.c: local_exports():
- Fix a separate bug exporting attributes to a new ksh function
  scope, which was previously masked by the other bug. The
  attributes (nvflag) were copied *after* nv_putval()ing the value,
  which is incorrect as the behaviour of nv_putval() is influenced
  by the attributes. But here, we're copying the value too, so we
  can simplify the whole function by using nv_clone() instead. This
  may also fix other corner cases. (re: c1994b87)

Resolves: https://github.com/ksh93/ksh/issues/465

NEWS

@@ -8,6 +8,10 @@ Any uppercase BUG_* names are modernish shell bug IDs.
 - Fixed a couple of bugs that caused floating point variables imported from
   a parent ksh through the environment to be corrupted.
 
+- Fixed a bug where invocation-local assignments preceding a built-in or
+  external command or ksh function call did not honour their pre-existing
+  attributes set by typeset (such as -i or -F) in certain cases.
+
 2022-06-01:
 
 - New 'typeset' feature inspired by bash (and zsh and mksh): the -g flag

src/cmd/ksh93/sh/name.c

@@ -253,7 +253,7 @@ void nv_setlist(register struct argnod *arg,register int flags, Namval_t *typ)
 	struct Namref	nr;
 	int		maketype = flags&NV_TYPE;  /* make a 'typeset -T' type definition command */
 	struct sh_type	shtp;
-	Dt_t		*save_vartree;
+	Dt_t		*vartree, *save_vartree;
 #if SHOPT_NAMESPACE
 	Namval_t	*save_namespace;
 #endif
@@ -287,6 +287,7 @@ void nv_setlist(register struct argnod *arg,register int flags, Namval_t *typ)
 	}
 	else
 		sh.prefix_root = sh.first_root = 0;
+	vartree = sh.prefix_root ? sh.prefix_root : sh.var_tree;
 	for(;arg; arg=arg->argnxt.ap)
 	{
 		sh.used_pos = 0;
@@ -474,8 +475,9 @@ void nv_setlist(register struct argnod *arg,register int flags, Namval_t *typ)
 						cp = stakcopy(nv_name(np));
 						if(!(arg->argflag&ARG_APPEND))
 							flag &= ~NV_ARRAY;
-						sh.prefix_root = sh.first_root;
-						np = nv_open(cp,sh.prefix_root?sh.prefix_root:sh.var_tree,flag);
+						if(sh.prefix_root = sh.first_root)
+							vartree = sh.prefix_root;
+						np = nv_open(cp,vartree,flag);
 					}
 					if(arg->argflag&ARG_APPEND)
 					{
@@ -563,20 +565,28 @@ void nv_setlist(register struct argnod *arg,register int flags, Namval_t *typ)
 			cp = arg->argval;
 			mp = 0;
 		}
-		if(eqp = strchr(cp,'='))
+		/*
+		 * Check if parent scope attributes need to be copied to the
+		 * temporary local scope of an assignment preceding a command.
+		 * https://github.com/ksh93/ksh/issues/465
+		 */
+		if((flags&(NV_EXPORT|NV_NOSCOPE))==(NV_EXPORT|NV_NOSCOPE) && dtvnext(vartree) && (eqp = strchr(cp,'=')))
 		{
-			/* Check for read-only */
 			*eqp = '\0';
-			np = nv_search(cp,sh.var_tree,0);
-			*eqp = '=';
-			if(np && nv_isattr(np,NV_RDONLY))
+			if((np = nv_search(cp,dtvnext(vartree),0)) && np->nvflag)
 			{
-				errormsg(SH_DICT,ERROR_exit(1),e_readonly,nv_name(np));
-				UNREACHABLE();
+				mp = nv_search(cp,vartree,NV_ADD|HASH_NOSCOPE);
+				mp->nvname = np->nvname;  /* put_lang() (init.c) compares nvname pointers */
+				mp->nvflag = np->nvflag;
+				mp->nvsize = np->nvsize;
+				mp->nvfun = np->nvfun;
 			}
-
+			*eqp = '=';
 		}
-		np = nv_open(cp,sh.prefix_root?sh.prefix_root:sh.var_tree,flags);
+		/*
+		 * Perform the assignment
+		 */
+		np = nv_open(cp,vartree,flags);
 		if(!np->nvfun && (flags&NV_NOREF))
 		{
 			if(sh.used_pos)

src/cmd/ksh93/sh/xec.c

@@ -3052,15 +3052,9 @@ pid_t sh_fork(int flags, int *jobid)
 static void  local_exports(register Namval_t *np, void *data)
 {
 	register Namval_t	*mp;
-	register char		*cp;
 	NOT_USED(data);
-	if(nv_isarray(np))
-		nv_putsub(np,NIL(char*),0);
-	if((cp = nv_getval(np)) && (mp = nv_search(nv_name(np), sh.var_tree, NV_ADD|HASH_NOSCOPE)) && nv_isnull(mp))
-	{
-		nv_putval(mp, cp, 0);
-		mp->nvflag = np->nvflag;
-	}
+	if(!nv_isnull(np) && (mp = nv_search(nv_name(np), sh.var_tree, NV_ADD|HASH_NOSCOPE)) && nv_isnull(mp))
+		nv_clone(np,mp,0);
 }
 
 /*

src/cmd/ksh93/tests/attributes.sh

@@ -799,5 +799,16 @@ got=$(export LC_NUMERIC=debug; typeset -xF5 num=7,75; "$SHELL" -c 'typeset -p nu
 [[ $got == "$exp" ]] || err_exit "floating ',' attribute/value not imported correctly" \
 	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
 
+# ======
+# Check that assignments preceding commands correctly honour existing attributes
+# https://github.com/ksh93/ksh/issues/465
+exp='typeset -x -F 5 num=7.75000'
+got=$(typeset -F5 num; num=3.25+4.5 "$SHELL" -c 'typeset -p num')
+[[ $got == "$exp" ]] || err_exit 'assignment preceding external command call does not honour pre-set attributes' \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+got=$(typeset -F5 num; num=3.25+4.5 command eval 'typeset -p num')
+[[ $got == "$exp" ]] || err_exit 'assignment preceding built-in command call does not honour pre-set attributes' \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
 # ======
 exit $((Errors<125?Errors:125))

src/cmd/ksh93/tests/functions.sh

@@ -1274,10 +1274,18 @@ got=$( { "$SHELL" -c 'PATH=/dev/null; function fn { unset -f fn; true; }; fn; fn
 # ======
 # Check if environment variables passed while invoking a function are exported
 # https://github.com/att/ast/issues/32
-unset foo
-function f2 { env | grep -q "^foo" || err_exit "Environment variable is not propagated from caller function"; }
-function f1 { f2; env | grep -q "^foo" || err_exit "Environment variable is not passed to a function"; }
-foo=bar f1
+# https://github.com/ksh93/ksh/issues/465
+exp='typeset -x -F 5 num=7.75000'
+exp=$exp$'\n'$exp$'\n'$exp
+got=$(
+	function f1 { typeset -p num; f2; }
+	function f2 { typeset -p num; f3; }
+	function f3 { typeset -p num; }
+	typeset -F5 num
+	num=3.25+4.5 f1
+)
+[[ $got == "$exp" ]] || err_exit 'assignment preceding ksh function call is not correctly exported or propagated' \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
 
 # ======
 # Over-shifting in a POSIX function should terminate the script