From 9067bb3cada9e78b3a1cabb1f5adc9ab5d0b3bf2 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Sat, 6 Aug 2022 10:47:24 +0100 Subject: [PATCH] Fix crashes on redefining/unsetting predefined alias (re: 7e7f1372) 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: https://github.com/ksh93/ksh/discussions/503#discussioncomment-3337172 --- NEWS | 8 ++++++++ src/cmd/ksh93/bltins/typeset.c | 12 +++--------- src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh/init.c | 5 ++--- src/cmd/ksh93/sh/main.c | 11 +++++++++-- src/cmd/ksh93/tests/alias.sh | 10 ++++++++++ src/lib/libast/man/cdt.3 | 11 ++++++----- 7 files changed, 39 insertions(+), 20 deletions(-) diff --git a/NEWS b/NEWS index eb7675f7fc15..6b37177b6604 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,14 @@ This documents significant changes in the dev branch of ksh 93u+m. For full details, see the git log at: https://github.com/ksh93/ksh 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 diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c index 07a2d5695c68..cc744fa29143 100644 --- a/src/cmd/ksh93/bltins/typeset.c +++ b/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) diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index 8997d4c7100a..aa88dc21e54d 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -18,7 +18,7 @@ #define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */ #define SH_RELEASE_SVER "1.1.0-alpha" /* semantic version number: https://semver.org */ -#define SH_RELEASE_DATE "2022-08-05" /* must be in this format for $((.sh.version)) */ +#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. */ diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c index aad6999b3807..06563cd91057 100644 --- a/src/cmd/ksh93/sh/init.c +++ b/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) diff --git a/src/cmd/ksh93/sh/main.c b/src/cmd/ksh93/sh/main.c index ace2d7619f2e..4a32ca061e2c 100644 --- a/src/cmd/ksh93/sh/main.c +++ b/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 /* diff --git a/src/cmd/ksh93/tests/alias.sh b/src/cmd/ksh93/tests/alias.sh index 6b4dc19b867e..10992c0dc066 100755 --- a/src/cmd/ksh93/tests/alias.sh +++ b/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)) diff --git a/src/lib/libast/man/cdt.3 b/src/lib/libast/man/cdt.3 index 9bcce2df13ac..0019fba03140 100644 --- a/src/lib/libast/man/cdt.3 +++ b/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