From 28f3ea5ebee356beedf2c2fc920598ce251e5f9e Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Mon, 16 Jun 2025 13:30:55 -0700 Subject: [PATCH] Fix out of bounds access when running POSIX functions This commit salvages a (long in LOC) bugfix from the currently postponed local-builtin branch. The bug in question has previously been described in the following ksh2020 bug report: https://github.com/att/ast/issues/1423 The ksh2020 bugfix in 9490d109 does fix the out of bounds access, but I've opted to avoid backporting it because it uses malloc wastefully. On 01-01-2024 in 7721eebf I undertook the task of fixing this bug differently by moving POSIX function execution from b_dot_cmd() to the much more appropriate sh_funscope(). Initially, this was primarily intended to aid in implementing dynamic scopes. For this commit I have used my previous work on that branch, but have been careful to avoid changing scoping behavior in any way whatsoever (this patch primarily aims to fix the out of bounds behavior). Perhaps this approach is a bit overzealous, but I think this route is better because it provides an improved framework for tackling scoping problems, (see #123), which makes this manner of fix better longterm. It's also better for SSOT to have the function execution code concentrated in sh_funscope with if statements clearly demarcating those parts peculiar to POSIX functions and codepaths especial to KornShell functions. (It's not like this patch is brand new either; I should've submitted this fix standalone over a year ago. Better late than never, I suppose.) Overview of changes: - Move POSIX function execution out of b_dot_cmd into sh_funscope. Due to the nature of dotted KornShell functions, those are still executed in b_dot_cmd(). - Delineate the codepaths for POSIX and KornShell functions in sh_funscope() using if statements. - Eliminate of the profitless DOTMAX definition; MAXDEPTH is adequate. - With the above change, simplify sh_funct() by doing away with the now needless b_dot_cmd() call and out of bounds access. As an aside, these changes slightly improve POSIX function performance (but only because POSIX functions lack scoping). Arbitrary benchmarks: $ cat bench/v-posix-fun.ksh for ((i=0; i!=1000000; i++)) do fun() { :; } fun foo bar args unset -f fun done $ cat bench/v-posix-fun-less.ksh for ((i=0; i!=1000000; i++)) do fun() { :; } fun unset -f fun done Results when run under shbench with 20 iterations and CCFLAGS=-O2 (devbranch is unpatched, basicfix is u+m with the ksh2020 fix applied, funscope-overhaul is u+m with this patch): -------------------------------------------------------------------------------------------- name /tmp/ksh-devbranch /tmp/ksh-basicfix /tmp/ksh-funscope-overhaul -------------------------------------------------------------------------------------------- v-posix-fun.ksh 1.125 [1.111-1.159] 1.159 [1.140-1.205] 0.966 [0.948-0.990] v-posix-fun-less.ksh 1.115 [1.099-1.130] 1.141 [1.117-1.226] 0.946 [0.929-0.971] -------------------------------------------------------------------------------------------- --- NEWS | 5 + src/cmd/ksh93/bltins/misc.c | 87 ++++++----- src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh/xec.c | 253 +++++++++++++++++++------------- 4 files changed, 200 insertions(+), 147 deletions(-) diff --git a/NEWS b/NEWS index 81c063c046d0..412bb060e4d7 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,11 @@ 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. +2025-06-16: + +- Fixed a possible out of bounds crash that could occur when running + a POSIX function (aka a function of the form 'name() {'). + 2025-06-14: - Fixed a bug occurring on systems with posix_spawn(3), spawnve(2), and diff --git a/src/cmd/ksh93/bltins/misc.c b/src/cmd/ksh93/bltins/misc.c index 168633f61762..3c3c4d04108c 100644 --- a/src/cmd/ksh93/bltins/misc.c +++ b/src/cmd/ksh93/bltins/misc.c @@ -56,8 +56,6 @@ #include #endif -#define DOTMAX MAXDEPTH /* maximum level of . nesting */ - /* * Handler function for nv_scan() that unsets a variable's export attribute */ @@ -232,16 +230,15 @@ int b_eval(int argc,char *argv[], Shbltin_t *context) #endif int b_dot_cmd(int n,char *argv[],Shbltin_t *context) { - char *script; - Namval_t *np; - int jmpval; - struct sh_scoped savst, *prevscope = sh.st.self; - char *filename=0, *buffer=0, *tofree; - int fd; - struct dolnod *saveargfor; - volatile struct dolnod *argsave=0; - struct checkpt buff; - Sfio_t *iop=0; + char *script; + Namval_t *np; + int jmpval, fd; + struct sh_scoped savst, *prevscope = sh.st.self; + char *filename=0, *buffer=0, *tofree; + struct dolnod *saveargfor; + volatile struct dolnod *argsave=0; + struct checkpt buff; + Sfio_t *iop=0; while (n = optget(argv,sh_optdot)) switch (n) { case ':': @@ -259,52 +256,47 @@ int b_dot_cmd(int n,char *argv[],Shbltin_t *context) errormsg(SH_DICT,ERROR_usage(2),"%s",optusage(NULL)); UNREACHABLE(); } - if(sh.dot_depth >= DOTMAX) + if(sh.dot_depth >= MAXDEPTH) { errormsg(SH_DICT,ERROR_exit(1),e_toodeep,script); UNREACHABLE(); } - if(!(np=sh.posix_fun)) + /* check for KornShell style function first */ + np = nv_search(script,sh.fun_tree,0); + if(np && is_afunction(np) && !nv_isattr(np,NV_FPOSIX) && !(sh_isoption(SH_POSIX) && context->bnode==SYSDOT)) { - /* check for KornShell style function first */ - np = nv_search(script,sh.fun_tree,0); - if(np && is_afunction(np) && !nv_isattr(np,NV_FPOSIX) && !(sh_isoption(SH_POSIX) && context->bnode==SYSDOT)) + if(!np->nvalue) { - if(!np->nvalue) + path_search(script,NULL,0); + if(np->nvalue) { - path_search(script,NULL,0); - if(np->nvalue) - { - if(nv_isattr(np,NV_FPOSIX)) - np = 0; - } - else - { - errormsg(SH_DICT,ERROR_exit(1),e_found,script); - UNREACHABLE(); - } + if(nv_isattr(np,NV_FPOSIX)) + np = 0; } - } - else - np = 0; - if(!np) - { - if((fd=path_open(script,path_get(script))) < 0) + else { - errormsg(SH_DICT,ERROR_system(1),e_open,script); + errormsg(SH_DICT,ERROR_exit(1),e_found,script); UNREACHABLE(); } - filename = path_fullname(stkptr(sh.stk,PATH_OFFSET)); } } + else + np = 0; + if(!np) + { + /* open the dot script */ + if((fd=path_open(script,path_get(script))) < 0) + { + errormsg(SH_DICT,ERROR_system(1),e_open,script); + UNREACHABLE(); + } + filename = path_fullname(stkptr(sh.stk,PATH_OFFSET)); + } *prevscope = sh.st; - sh.st.lineno = np?((struct functnod*)nv_funtree(np))->functline:1; + sh.st.lineno = np ? ((struct functnod*)nv_funtree(np))->functline : 1; sh.st.save_tree = sh.var_tree; if(filename) - { sh.st.filename = filename; - sh.st.lineno = 1; - } sh.st.prevst = prevscope; sh.st.self = &savst; sh.topscope = (Shscope_t*)sh.st.self; @@ -312,8 +304,7 @@ int b_dot_cmd(int n,char *argv[],Shbltin_t *context) tofree = sh.st.filename; if(np) sh.st.filename = ((struct Ufunction*)np->nvalue)->fname; - nv_putval(SH_PATHNAMENOD, sh.st.filename ,NV_NOFREE); - sh.posix_fun = 0; + nv_putval(SH_PATHNAMENOD,sh.st.filename,NV_NOFREE); if(np || argv[1]) argsave = sh_argnew(argv,&saveargfor); sh_pushcontext(&buff,SH_JMPDOT); @@ -325,9 +316,13 @@ int b_dot_cmd(int n,char *argv[],Shbltin_t *context) sh.dot_depth++; update_sh_level(); if(np) + { + /* execute the function as though it were a dot script */ sh_exec((Shnode_t*)(nv_funtree(np)),sh_isstate(SH_ERREXIT)); + } else { + /* run the dot script */ buffer = sh_malloc(IOBSIZE+1); iop = sfnew(NULL,buffer,IOBSIZE,fd,SFIO_READ); sh_offstate(SH_NOFORK); @@ -348,12 +343,12 @@ int b_dot_cmd(int n,char *argv[],Shbltin_t *context) prevscope->dolc = sh.st.dolc; prevscope->dolv = sh.st.dolv; } - if (sh.st.self != &savst) + if(sh.st.self != &savst) *sh.st.self = sh.st; - /* only restore the top Shscope_t portion for POSIX functions */ + /* only restore the top Shscope_t portion for functions */ memcpy(&sh.st, prevscope, sizeof(Shscope_t)); sh.topscope = (Shscope_t*)prevscope; - nv_putval(SH_PATHNAMENOD, sh.st.filename ,NV_NOFREE); + nv_putval(SH_PATHNAMENOD,sh.st.filename,NV_NOFREE); if(jmpval && jmpval!=SH_JMPFUN) siglongjmp(*sh.jmplist,jmpval); return sh.exitval; diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index b4776a335394..51d98331ba28 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -18,7 +18,7 @@ #include #include "git.h" -#define SH_RELEASE_DATE "2025-06-14" /* must be in this format for $((.sh.version)) */ +#define SH_RELEASE_DATE "2025-06-16" /* must be in this format for $((.sh.version)) */ /* * This comment keeps SH_RELEASE_DATE a few lines away from SH_RELEASE_SVER to avoid * merge conflicts when cherry-picking dev branch commits onto a release branch. diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index 1f78e3a738f4..31edc6731de4 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/src/cmd/ksh93/sh/xec.c @@ -2913,133 +2913,177 @@ int sh_funscope(int argn, char *argv[],int(*fun)(void*),void *arg,int execflg) { char *trap; int nsig; - struct dolnod *argsav=0,*saveargfor; + struct dolnod *argsav = 0, *saveargfor; struct sh_scoped *savst = stkalloc(sh.stk,sizeof(struct sh_scoped)); struct sh_scoped *prevscope = sh.st.self; - struct argnod *envlist=0; - int isig,jmpval; + struct argnod *envlist = 0; + int isig, jmpval; volatile int r = 0; - int n; + int posix_fun = 0, save_loopcnt = sh.st.loopcnt; char save_invoc_local; - char **savsig, *save_debugtrap = 0; + char **savsig; struct funenv *fp = 0; struct checkpt *buffp = stkalloc(sh.stk,sizeof(struct checkpt)); Namval_t *nspace = sh.namespace; Dt_t *last_root = sh.last_root; - Shopt_t options; - options = sh.options; + Shopt_t save_options; NOT_USED(argn); - if(sh.fn_depth==0) - sh.glob_options = sh.options; - else - sh.options = sh.glob_options; *prevscope = sh.st; - sh_offoption(SH_ERREXIT); sh.st.prevst = prevscope; sh.st.self = savst; sh.topscope = (Shscope_t*)sh.st.self; - sh.st.opterror = sh.st.optchar = 0; - sh.st.optindex = 1; sh.st.loopcnt = 0; if(!fun) { fp = (struct funenv*)arg; - sh.st.real_fun = fp->node->nvalue; envlist = fp->env; + posix_fun = nv_isattr(fp->node,NV_FPOSIX); + if(!posix_fun) + sh.st.real_fun = fp->node->nvalue; + } + if(posix_fun) + { + /* increase POSIX function depth (shared with dotted KornShell function depth) */ + if(sh.dot_depth >= MAXDEPTH) + { + errormsg(SH_DICT,ERROR_exit(1),e_toodeep,argv[0]); + UNREACHABLE(); + } + sh.dot_depth++; + } + else + { + save_options = sh.options; + sh.st.opterror = sh.st.optchar = 0; + sh.st.optindex = 1; + if(sh.fn_depth==0) + sh.glob_options = sh.options; + else + sh.options = sh.glob_options; + sh_offoption(SH_ERREXIT); } prevscope->save_tree = sh.var_tree; - n = dtvnext(prevscope->save_tree)!= (sh.namespace?sh.var_base:0); - sh_scope(envlist,1); - if(n) + if(!posix_fun) { - /* eliminate parent scope */ - nv_scan(prevscope->save_tree, local_exports, NULL, NV_EXPORT, NV_EXPORT|NV_NOSCOPE); + /* create a local scope for the KornShell function */ + int dtret = dtvnext(prevscope->save_tree) != (sh.namespace?sh.var_base:0); + sh_scope(envlist,1); + if(dtret) + { + /* + * Clone exported variables from the previous scope into + * the new static local scope. The NV_NOSCOPE flag will + * cause nv_scan() to eliminate the viewpath to the parent + * function's static scope. + */ + nv_scan(prevscope->save_tree, local_exports, NULL, NV_EXPORT, NV_EXPORT|NV_NOSCOPE); + } } sh.st.save_tree = sh.var_tree; if(!fun) { - if(nv_isattr(fp->node,NV_TAGGED)) - sh_onoption(SH_XTRACE); - else if(!sh_isoption(SH_FUNCTRACE)) - sh_offoption(SH_XTRACE); + if(fp->node->nvalue) + sh.st.filename = ((struct Ufunction *)fp->node->nvalue)->fname; + nv_putval(SH_PATHNAMENOD, sh.st.filename, NV_NOFREE); + sh.last_root = nv_dict(DOTSHNOD); } - sh.st.cmdname = argv[0]; - /* save trap table */ - if((nsig=sh.st.trapmax)>0 || sh.st.trapcom[0]) + if(!posix_fun) { - savsig = sh_malloc(nsig * sizeof(char*)); - /* - * the data is, usually, modified in code like: - * tmp = buf[i]; buf[i] = sh_strdup(tmp); free(tmp); - * so sh.st.trapcom needs a "deep copy" to properly save/restore pointers. - */ - for (isig = 0; isig < nsig; ++isig) + char *save_debugtrap = 0; + if(!fun) { - if(sh.st.trapcom[isig] == Empty) - savsig[isig] = Empty; - else if(sh.st.trapcom[isig]) - savsig[isig] = sh_strdup(sh.st.trapcom[isig]); - else - savsig[isig] = NULL; + if(nv_isattr(fp->node,NV_TAGGED)) + sh_onoption(SH_XTRACE); + else if(!sh_isoption(SH_FUNCTRACE)) + sh_offoption(SH_XTRACE); + } + sh.st.cmdname = argv[0]; + /* save trap table */ + if((nsig=sh.st.trapmax)>0 || sh.st.trapcom[0]) + { + savsig = sh_malloc((size_t)nsig * sizeof(char*)); + /* + * the data is, usually, modified in code like: + * tmp = buf[i]; buf[i] = sh_strdup(tmp); free(tmp); + * so sh.st.trapcom needs a "deep copy" to properly save/restore pointers. + */ + for (isig = 0; isig < nsig; ++isig) + { + if(sh.st.trapcom[isig] == Empty) + savsig[isig] = Empty; + else if(sh.st.trapcom[isig]) + savsig[isig] = sh_strdup(sh.st.trapcom[isig]); + else + savsig[isig] = NULL; + } } + if(!fun && sh_isoption(SH_FUNCTRACE) && sh.st.trap[SH_DEBUGTRAP] && *sh.st.trap[SH_DEBUGTRAP]) + save_debugtrap = sh_strdup(sh.st.trap[SH_DEBUGTRAP]); + sh_sigreset(-1); + if(save_debugtrap) + sh.st.trap[SH_DEBUGTRAP] = save_debugtrap; + save_invoc_local = sh.invoc_local; + sh.invoc_local = 0; } - if(!fun && sh_isoption(SH_FUNCTRACE) && sh.st.trap[SH_DEBUGTRAP] && *sh.st.trap[SH_DEBUGTRAP]) - save_debugtrap = sh_strdup(sh.st.trap[SH_DEBUGTRAP]); - sh_sigreset(-1); - if(save_debugtrap) - sh.st.trap[SH_DEBUGTRAP] = save_debugtrap; argsav = sh_argnew(argv,&saveargfor); sh_pushcontext(buffp,SH_JMPFUN); errorpush(&buffp->err,0); error_info.id = argv[0]; if(!fun) { - struct Ufunction *rp = fp->node->nvalue; - if(rp) - sh.st.filename = rp->fname; sh.st.funname = nv_name(fp->node); - sh.last_root = nv_dict(DOTSHNOD); - nv_putval(SH_PATHNAMENOD,sh.st.filename,NV_NOFREE); nv_putval(SH_FUNNAMENOD,sh.st.funname,NV_NOFREE); } - save_invoc_local = sh.invoc_local; - sh.invoc_local = 0; jmpval = sigsetjmp(buffp->buff,0); if(jmpval == 0) { - if(sh.fn_depth >= MAXDEPTH) + if(!posix_fun) { - sh.toomany = 1; - siglongjmp(*sh.jmplist,SH_JMPERRFN); + /* increase KornShell function depth */ + if(sh.fn_depth >= MAXDEPTH) + { + sh.toomany = 1; + siglongjmp(*sh.jmplist,SH_JMPERRFN); + } + sh.fn_depth++; } - sh.fn_depth++; update_sh_level(); if(fun) - r= (*fun)(arg); + r = (*fun)(arg); else { - char **arg = sh.st.real_fun->argv; - Namval_t *np, *nq, **nref; - if(nref=fp->nref) + if(posix_fun) + sh_exec((Shnode_t*)(nv_funtree(fp->node)),sh_isstate(SH_ERREXIT)); + else { - sh.last_root = 0; - for(r=0; arg[r]; r++) + char **arg = sh.st.real_fun->argv; + Namval_t *np, *nq, **nref; + if(nref=fp->nref) { - np = nv_search(arg[r],sh.var_tree,NV_NOSCOPE|NV_ADD); - if(np && (nq=*nref++)) + sh.last_root = 0; + for(r=0; arg[r]; r++) { - np->nvalue = sh_newof(0,struct Namref,1,0); - ((struct Namref*)np->nvalue)->np = nq; - nv_onattr(np,NV_REF|NV_NOFREE); + np = nv_search(arg[r],sh.var_tree,NV_NOSCOPE|NV_ADD); + if(np && (nq=*nref++)) + { + np->nvalue = sh_newof(0,struct Namref,1,0); + ((struct Namref*)np->nvalue)->np = nq; + nv_onattr(np,NV_REF|NV_NOFREE); + } } } + sh_exec((Shnode_t*)(nv_funtree((fp->node))),execflg|SH_ERREXIT); } - sh_exec((Shnode_t*)(nv_funtree((fp->node))),execflg|SH_ERREXIT); r = sh.exitval; } } - sh.invoc_local = save_invoc_local; - sh.fn_depth--; + if(posix_fun) + sh.dot_depth--; + else + { + sh.fn_depth--; + sh.invoc_local = save_invoc_local; + } update_sh_level(); if(sh.fn_depth==1 && jmpval==SH_JMPERRFN) { @@ -3047,10 +3091,39 @@ int sh_funscope(int argn, char *argv[],int(*fun)(void*),void *arg,int execflg) UNREACHABLE(); } sh_popcontext(buffp); - sh_unscope(); - sh.namespace = nspace; + /* reinstate the previous scope */ + if(!posix_fun) + { + sh_unscope(); + sh.namespace = nspace; + } sh.var_tree = (Dt_t*)prevscope->save_tree; - sh_argreset(argsav,saveargfor); + if((posix_fun && jmpval!=SH_JMPSCRIPT) || !posix_fun) + sh_argreset(argsav,saveargfor); + else + { + prevscope->dolc = sh.st.dolc; + prevscope->dolv = sh.st.dolv; + } + /* + * POSIX function cleanup + */ + if(posix_fun) + { + if(sh.st.self != savst) + *sh.st.self = sh.st; + /* Only restore the top Shscope_t portion for POSIX functions */ + memcpy(&sh.st, prevscope, sizeof(Shscope_t)); + sh.topscope = (Shscope_t*)prevscope; + nv_putval(SH_PATHNAMENOD,sh.st.filename,NV_NOFREE); + if(jmpval && jmpval!=SH_JMPFUN) + siglongjmp(*sh.jmplist,jmpval); + sh.st.loopcnt = save_loopcnt; + return r; + } + /* + * KornShell function cleanup + */ trap = sh.st.trapcom[0]; sh.st.trapcom[0] = 0; sh_sigreset(1); @@ -3062,11 +3135,11 @@ int sh_funscope(int argn, char *argv[],int(*fun)(void*),void *arg,int execflg) for (isig = 0; isig < nsig; ++isig) if (sh.st.trapcom[isig] && sh.st.trapcom[isig]!=Empty) free(sh.st.trapcom[isig]); - memcpy((char*)&sh.st.trapcom[0],savsig,nsig*sizeof(char*)); + memcpy((char*)&sh.st.trapcom[0],savsig,(size_t)nsig*sizeof(char*)); free(savsig); } - sh.trapnote=0; - sh.options = options; + sh.trapnote = 0; + sh.options = save_options; sh.last_root = last_root; if(jmpval == SH_JMPSUB) siglongjmp(*sh.jmplist,jmpval); @@ -3103,30 +3176,10 @@ static void sh_funct(Namval_t *np,int argn, char *argv[],struct argnod *envlist, sh_setscope(sh.topscope); sh.st.lineno = error_info.line; rp->running += 2; - if(nv_isattr(np,NV_FPOSIX)) - { - char *save; - int loopcnt = sh.st.loopcnt; - sh.posix_fun = np; - save = argv[-1]; - argv[-1] = 0; - sh.st.funname = nv_name(np); - sh.last_root = nv_dict(DOTSHNOD); - nv_putval(SH_FUNNAMENOD, nv_name(np),NV_NOFREE); - opt_info.index = opt_info.offset = 0; - error_info.errors = 0; - sh.st.loopcnt = 0; - b_dot_cmd(argn+1,argv-1,&sh.bltindata); - sh.st.loopcnt = loopcnt; - argv[-1] = save; - } - else - { - fun.env = envlist; - fun.node = np; - fun.nref = 0; - sh_funscope(argn,argv,0,&fun,execflg); - } + fun.env = envlist; + fun.node = np; + fun.nref = 0; + sh_funscope(argn,argv,0,&fun,execflg); sh.last_root = nv_dict(DOTSHNOD); nv_putval(SH_FUNNAMENOD,fname,NV_NOFREE); nv_putval(SH_PATHNAMENOD,sh.st.filename,NV_NOFREE);