Fix crashes on redefining/unsetting predefined alias (re: 7e7f137)

Reproducer 1:

  $ alias r
  r='hist -s'
  $ alias r=foo
  $ unalias r
  ksh(10127,0x10d6c35c0) malloc: *** error for object 0x7ffdcd404468: pointer being freed was not allocated
  ksh(10127,0x10d6c35c0) malloc: *** set a breakpoint in malloc_error_break to debug
  Abort

The crash happens as unall() (typeset.c) calls nv_delete() (name.c)
which tries to free a node pointer that was not directly allocated.

Reproducer 2:

  $ ENV=/./dev/null ksh
  $ echo : >script
  $ chmod +x script
  $ alias r=foo
  $ ./script
  ksh(10193,0x10c8505c0) malloc: *** error for object 0x7fa136c04468: pointer being freed was not allocated
  ksh(10193,0x10c8505c0) malloc: *** set a breakpoint in malloc_error_break to debug
  Abort

This crash happens for the same reason, but in another location,
namely in sh_reinit() (init.c) as it is freeing up the alias table
before executing a script that does not start with a #! path.

This is a serious bug because it is not uncommon for .kshrc or
.profile scripts to (re)define an alias called 'r'.

Analysis:

These crashes happen because the incorrectly freed node pointer is
part of a larger block of nodes initialised by sh_inittree() in
init.c. That function allocates all the nodes for a table (see
data/{aliases,builtins,variables}.c) in a contiguous block
addressable by numeric index (see builtins.h and variables.h for
how that is used).

So, while the value of the alias is correctly marked NV_NOFREE and
is not freed, that attribute does not apply to the node pointer
itself, which also is not freeable. Thus, if the value is replaced
by a freeable one, the node pointer is incorrectly freed upon
unaliasing it, and the shell crashes.

The simplest fix is to allocate each predefined alias node
individually, like any other alias -- because, in fact, we do not
need the predefined alias nodes to be in a contiguous addressable
block; there is nothing that specifically addresses these aliases.

src/cmd/ksh93/sh/main.c: sh_main():
- Instead of calling sh_inittree(), use a dedicated loop to
  allocate each predefined alias node individually, making each
  node invidually freeable. The value does remain non-freeable,
  but the NV_NOFREE attribute correctly takes care of that.

src/cmd/ksh93/bltins/typeset.c:
- Get rid of the incomplete and now unnecessary workarounds in
  unall() and sh_reinit().

Thanks to @jghub and @JohnoKing for finding and reporting the bug.
Discussion: #503 (reply in thread)

NEWS

@@ -2,6 +2,14 @@ This documents significant changes in the 1.0 branch of ksh 93u+m.
 For full details, see the git log at: https://github.com/ksh93/ksh/tree/1.0
 Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library.
 
+2022-08-06:
+
+- Fixed an interactive shell crashing on redefining, then unsetting one of
+  the predefined aliases.
+
+- Fixed an interactive shell crashing on redefining one of the predefined
+  aliases, then executing a shell script that does not start with a #! path.
+
 2022-08-05:
 
 - Reverted a buggy exec optimisation introduced on 2022-06-18. It is known

src/cmd/ksh93/bltins/typeset.c

@@ -1286,7 +1286,7 @@ static int unall(int argc, char **argv, register Dt_t *troot)
 	register const char *name;
 	volatile int r;
 	Dt_t	*dp;
-	int nflag=0,all=0,isfun,jmpval,nofree_attr;
+	int nflag=0,all=0,isfun,jmpval;
 	struct checkpt buff;
 	NOT_USED(argc);
 	if(troot==sh.alias_tree)
@@ -1388,13 +1388,6 @@ static int unall(int argc, char **argv, register Dt_t *troot)
 					sh_assignok(np, !nv_isattr(np,NV_NODISC|NV_ARRAY) && !nv_isvtree(np));
 				}
 			}
-			/*
-			 * Preset aliases have the NV_NOFREE attribute and cannot be safely freed from memory.
-			 * _nv_unset discards this flag so it's obtained now to prevent an invalid free crash.
-			 */
-			if(troot==sh.alias_tree)
-				nofree_attr = nv_isattr(np,NV_NOFREE);	/* note: returns bitmask, not boolean */
-
 			if(!nv_isnull(np) || nv_size(np) || nv_isattr(np,~(NV_MINIMAL|NV_NOFREE)))
 				_nv_unset(np,0);
 			if(troot==sh.var_tree && sh.st.real_fun && (dp=sh.var_tree->walk) && dp==sh.st.real_fun->sdict)
@@ -1411,7 +1404,8 @@ static int unall(int argc, char **argv, register Dt_t *troot)
 			{
 				if(sh.subshell && !sh.subshare)
 					sh_subfork();	/* avoid affecting the parent shell's alias table */
-				nv_delete(np,troot,nofree_attr);
+				_nv_unset(np,nv_isattr(np,NV_NOFREE));
+				nv_delete(np,troot,0);
 			}
 		}
 		else if(troot==sh.alias_tree)

src/cmd/ksh93/include/version.h

@@ -17,8 +17,8 @@
 #include <releaseflags.h>
 
 #define SH_RELEASE_FORK	"93u+m"		/* only change if you develop a new ksh93 fork */
-#define SH_RELEASE_SVER	"1.0.1"		/* semantic version number: https://semver.org */
-#define SH_RELEASE_DATE	"2022-08-05"	/* must be in this format for $((.sh.version)) */
+#define SH_RELEASE_SVER	"1.0.2-alpha"	/* semantic version number: https://semver.org */
+#define SH_RELEASE_DATE	"2022-08-06"	/* must be in this format for $((.sh.version)) */
 #define SH_RELEASE_CPYR	"(c) 2020-2022 Contributors to ksh " SH_RELEASE_FORK
 
 /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */

src/cmd/ksh93/sh/init.c

@@ -1627,9 +1627,8 @@ int sh_reinit(char *argv[])
 		if((dp = sh.alias_tree)->walk)
 			dp = dp->walk;
 		npnext = (Namval_t*)dtnext(sh.alias_tree,np);
-		nofree = nv_isattr(np,NV_NOFREE);			/* note: returns bitmask, not boolean */
-		_nv_unset(np,NV_RDONLY);				/* also clears NV_NOFREE attr, if any */
-		nv_delete(np,dp,nofree);
+		_nv_unset(np,nv_isattr(np,NV_NOFREE));
+		nv_delete(np,dp,0);
 	}
 	/* Delete hash table entries */
 	for(np = dtfirst(sh.track_tree); np; np = npnext)

src/cmd/ksh93/sh/main.c

@@ -158,11 +158,18 @@ int sh_main(int ac, char *av[], Shinit_f userinit)
 			sh_onoption(SH_INTERACTIVE);
 		if(sh_isoption(SH_INTERACTIVE))
 		{
+			const struct shtable2 *tp;
 			sh_onoption(SH_BGNICE);
 			sh_onoption(SH_RC);
 			/* preset aliases for interactive ksh/sh */
-			dtclose(sh.alias_tree);
-			sh.alias_tree = sh_inittree(shtab_aliases);
+			for(tp = shtab_aliases; *tp->sh_name; tp++)
+			{
+				Namval_t *np = sh_calloc(1,sizeof(Namval_t));
+				np->nvname = (char*)tp->sh_name;	/* alias name */
+				np->nvflag = tp->sh_number;		/* attributes (must include NV_NOFREE) */
+				np->nvalue.cp = (char*)tp->sh_value;	/* non-freeable value */
+				dtinstall(sh.alias_tree,np);
+			}
 		}
 #if SHOPT_REMOTE
 		/*

src/cmd/ksh93/tests/alias.sh

@@ -290,5 +290,15 @@ got=$(unalias foo; echo "$? $$")
 [[ $got == "$exp" ]] || err_exit "unalias in subshell: expected $(printf %q "$exp"), got $(printf %q "$got")"
 unalias foo
 
+# ======
+# Redefining a predefined alias, then unsetting it, caused a crash on trying to free a non-allocated pointer
+# https://github.com/ksh93/ksh/discussions/503#discussioncomment-3337172
+got=$({ "$SHELL" -i -c 'alias r=foo; unalias r'; } 2>&1) \
+|| err_exit "crash on redefining and unsetting predefined alias (got $(printf %q "$got"))"
+echo : >|script
+chmod +x script
+got=$({ "$SHELL" -i -c 'alias r=foo; ./script'; } 2>&1) \
+|| err_exit "crash on running script after redefining predefined alias (got $(printf %q "$got"))"
+
 # ======
 exit $((Errors<125?Errors:125))

src/lib/libast/man/cdt.3

@@ -272,7 +272,8 @@ The calls \f3dtinsert()\fP and \f3dtinstall()\fP will insert a new object
 in front of such a current object
 while the call \f3dtappend()\fP will append in back of it.
 .Ss "  Dtdeque"
-Objects are kept in a deque. This is similar to \f3Dtlist\fP
+Objects are kept in a deque (double-ended queue; pronounce "deck").
+This is similar to \f3Dtlist\fP
 except that objects are always inserted at the front and appended at the tail
 of the list.
 .Ss "  Dtstack"
@@ -467,10 +468,10 @@ See \f3Dtdisc_t.makef\fP for object construction.
 \f3dtinsert()\fP and \f3dtappend()\fP perform the same function
 for all methods except for \f3Dtlist\fP (see \f3Dtlist\fP for details).
 For \f3Dtset\fP, \f3Dtrhset\fP or \f3Dtoset\fP,
-if there is an object in \f3dt\fP matching \f3obj\fP
-\f3dtinsert()\fP and \f3dtappend()\fP will not insert a new object.
-On the other hand, \f3dtinstall()\fP remove such a matching
-object then insert the new object.
+if there is an object in \f3dt\fP matching \f3obj\fP,
+\f3dtinsert()\fP and \f3dtappend()\fP will not insert a new object,
+whereas \f3dtinstall()\fP will remove such a matching
+object before inserting the new object.
 
 On failure, \f3dtinsert()\fP and \f3dtinstall()\fP return \f3NULL\fP.
 Otherwise, the return value is either the newly inserted object