From 61e0f90460ed4518837434d5901319766ff5df3c Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Tue, 20 Apr 2021 05:39:10 +0100 Subject: [PATCH] Yet more fixes for subshell directory handling (re: feaf718f) There were still problems left after the previous commit. On at least one system (QNX i386), the following regression test crashed: src/cmd/ksh93/test/subshell.c 900 got=$( { "$SHELL" -c '(cd /; (cd /)); print -r -- "PWD=$PWD"'; } 2>&1 ) A backtrace done on the core dunp pointed to the free() call here: src/cmd/ksh93/bltins/cd_pwd.c 90 if(oldpwd && oldpwd!=shp->pwd && oldpwd!=e_dot) 91 free(oldpwd); Analysis: The interaction between $PWD, sh.pwd aka shp->pwd, and the path_pwd() function is a mess. path_pwd() usually returns a freeable value, but not always. sh.pwd is sometimes a pointer to the value of $PWD, but not always (e.g. when you unset PWD or assign to it). Instead of debugging the exact cause of the crash, I think it is better to make this work in a more consistent way. As of this commit: 1. sh.pwd keeps its own copy of the PWD, independently of the PWD variable. The old value must always be freed immediately before assigning a new one. This is simple and consistent, reducing the chance of bugs at negligible cost. 2. The PWD variable is no longer given the NV_NOFREE attribute because its value no longer points to sh.pwd. It is now a variable like any other. src/cmd/ksh93/sh/path.c: path_pwd(): - Do not give PWDNOD the NV_NOFREE attribute. - Give sh.pwd its own copy of the PWD by strdup'ing PWDNOD's value. src/cmd/ksh93/bltins/cd_pwd.c: b_cd(): - Since sh.pwd is now consistently freed before giving it a new value and at no other time, oldpwd must not be freed any longer and can become a regular non-static variable. - If the PWD needs reinitialising, call path_pwd() to do it. src/cmd/ksh93/sh/subshell.c: sh_subshell(): - Systems with fchdir(2): Always restore the PWD upon exiting a non-subshare subshell. The check to decide whether or not to restore it was unsafe: it was not restored if the current PWD pointer and value was identical to the saved one, but a directory can be deleted and recreated under the same name. - Systems without fchdir(2) (if any exist): . Entry: Fork if the PWD is nonexistent or has no x permission. . Restore: Only chdir back if the subshell PWD was changed. That's probably the best we can do. It remains inherently unsafe. We should probably just require fchdir(2) at some point. --- src/cmd/ksh93/bltins/cd_pwd.c | 22 +++++++-------- src/cmd/ksh93/sh/path.c | 11 +++++--- src/cmd/ksh93/sh/subshell.c | 51 +++++++++++++++++------------------ 3 files changed, 41 insertions(+), 43 deletions(-) diff --git a/src/cmd/ksh93/bltins/cd_pwd.c b/src/cmd/ksh93/bltins/cd_pwd.c index 30a39833f..6262b9003 100644 --- a/src/cmd/ksh93/bltins/cd_pwd.c +++ b/src/cmd/ksh93/bltins/cd_pwd.c @@ -57,7 +57,7 @@ int b_cd(int argc, char *argv[],Shbltin_t *context) register Shell_t *shp = context->shp; int saverrno=0; int rval,flag=0; - static char *oldpwd; + char *oldpwd; Namval_t *opwdnod, *pwdnod; if(sh_isoption(SH_RESTRICTED)) { @@ -87,8 +87,6 @@ int b_cd(int argc, char *argv[],Shbltin_t *context) errormsg(SH_DICT,ERROR_usage(2),"%s",optusage((char*)0)); UNREACHABLE(); } - if(oldpwd && oldpwd!=shp->pwd && oldpwd!=e_dot) - free(oldpwd); oldpwd = path_pwd(shp,0); opwdnod = sh_scoped(shp,OLDPWDNOD); pwdnod = sh_scoped(shp,PWDNOD); @@ -229,25 +227,25 @@ success: while(--flag>0 && dir[flag]=='/') dir[flag] = 0; nv_putval(pwdnod,dir,NV_RDONLY); + nv_onattr(pwdnod,NV_EXPORT); + if(shp->pwd) + free((void*)shp->pwd); + shp->pwd = sh_strdup(pwdnod->nvalue.cp); } else { /* pathcanon() failed to canonicalize the directory, which happens when 'cd' is invoked from a nonexistent PWD with a relative path as the argument. Reinitialize $PWD as it will be wrong. */ - char *cp = getcwd(NIL(char*),0); - if(cp) - { - nv_putval(pwdnod,cp,NV_RDONLY); - free(cp); - } - else + if(shp->pwd) + free((void*)shp->pwd); + shp->pwd = NIL(const char*); + path_pwd(shp,0); + if(*shp->pwd != '/') { errormsg(SH_DICT,ERROR_system(1),e_direct); UNREACHABLE(); } } - nv_onattr(pwdnod,NV_NOFREE|NV_EXPORT); - shp->pwd = pwdnod->nvalue.cp; nv_scan(sh_subtracktree(1),rehash,(void*)0,NV_TAGGED,NV_TAGGED); path_newdir(shp,shp->pathlist); path_newdir(shp,shp->cdpathlist); diff --git a/src/cmd/ksh93/sh/path.c b/src/cmd/ksh93/sh/path.c index 7776028e7..edb975876 100644 --- a/src/cmd/ksh93/sh/path.c +++ b/src/cmd/ksh93/sh/path.c @@ -256,6 +256,7 @@ char *path_pwd(Shell_t *shp,int flag) { /* Check if $HOME is a path to the PWD; this ensures $PWD == $HOME at login, even if $HOME is a path that contains symlinks */ + char tofree = 0; cp = nv_getval(sh_scoped(shp,HOME)); if(!(cp && *cp=='/' && test_inode(cp,e_dot))) { @@ -263,17 +264,19 @@ char *path_pwd(Shell_t *shp,int flag) cp = getcwd(NIL(char*),0); if(!cp) return((char*)e_dot); + tofree++; } /* Store in PWD variable */ if(shp->subshell) pwdnod = sh_assignok(pwdnod,1); - nv_offattr(pwdnod,NV_NOFREE); nv_putval(pwdnod,cp,NV_RDONLY); + if(tofree) + free(cp); } - nv_onattr(pwdnod,NV_NOFREE|NV_EXPORT); + nv_onattr(pwdnod,NV_EXPORT); /* Set shell PWD */ - shp->pwd = (char*)(pwdnod->nvalue.cp); - return(cp); + shp->pwd = sh_strdup(pwdnod->nvalue.cp); + return((char*)shp->pwd); } /* diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c index 344b5e8b5..3039b9795 100644 --- a/src/cmd/ksh93/sh/subshell.c +++ b/src/cmd/ksh93/sh/subshell.c @@ -82,7 +82,6 @@ static struct subshell pid_t subpid; /* child process id */ Sfio_t* saveout;/* saved standard output */ char *pwd; /* present working directory */ - const char *shpwd; /* saved pointer to sh.pwd */ void *jobs; /* save job info */ mode_t mask; /* saved umask */ short tmpfd; /* saved tmp file descriptor */ @@ -213,7 +212,6 @@ void sh_subfork(void) else { /* this is the child part of the fork */ - /* setting subpid to 1 causes subshell to exit when reached */ sh_onstate(SH_FORKED); subshell_data = 0; shp->subshell = 0; @@ -561,9 +559,6 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) path_get(shp,e_dot); shp->pathinit = 0; } -#if _lib_fchdir - sp->pwdfd = -1; -#endif /* _lib_fchdir */ if(!shp->pwd) path_pwd(shp,0); sp->bckpid = shp->bckpid; @@ -584,8 +579,8 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) if(!shp->subshare) { struct subshell *xp; - sp->shpwd = shp->pwd; #if _lib_fchdir + sp->pwdfd = -1; for(xp=sp->prev; xp; xp=xp->prev) { if(xp->pwdfd>0 && xp->pwd && strcmp(xp->pwd,shp->pwd)==0) @@ -687,6 +682,17 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) #if _lib_fchdir if(sp->pwdfd < 0 && !shp->subshare) /* if we couldn't get a file descriptor to our PWD ... */ sh_subfork(); /* ...we have to fork, as we cannot fchdir back to it. */ +#else + if(!shp->subshare) + { + if(sp->pwd && access(sp->pwd,X_OK)<0) + { + free(sp->pwd); + sp->pwd = NIL(char*); + } + if(!sp->pwd) + sh_subfork(); + } #endif /* _lib_fchdir */ sh_offstate(SH_INTERACTIVE); sh_exec(t,flags); @@ -842,32 +848,21 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) free((void*)savsig); } shp->options = sp->options; - if(shp->pwd != sp->pwd && (!shp->pwd || !sp->pwd || strcmp(sp->pwd,shp->pwd))) - { - /* restore the present working directory */ - Namval_t *pwdnod = sh_scoped(shp,PWDNOD); + /* restore the present working directory */ #if _lib_fchdir - if(fchdir(sp->pwdfd) < 0) + if(sp->pwdfd > 0 && fchdir(sp->pwdfd) < 0) #else - if(!sp->pwd || chdir(sp->pwd) < 0) + if(sp->pwd && strcmp(sp->pwd,shp->pwd) && chdir(sp->pwd) < 0) #endif /* _lib_fchdir */ - { - saveerrno = errno; - fatalerror = 2; - } - shp->pwd=sp->pwd; - path_newdir(shp,shp->pathlist); - if(nv_isattr(pwdnod,NV_NOFREE)) - pwdnod->nvalue.cp = (const char*)sp->pwd; - } - else if(sp->shpwd != shp->pwd) { - shp->pwd = sp->pwd; - if(PWDNOD->nvalue.cp==sp->shpwd) - PWDNOD->nvalue.cp = sp->pwd; + saveerrno = errno; + fatalerror = 2; } - else - free((void*)sp->pwd); + else if(sp->pwd && strcmp(sp->pwd,shp->pwd)) + path_newdir(shp,shp->pathlist); + if(shp->pwd) + free((void*)shp->pwd); + shp->pwd = sp->pwd; #if _lib_fchdir if(sp->pwdclose) close(sp->pwdfd); @@ -940,6 +935,8 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) UNREACHABLE(); case 2: /* reinit PWD as it will be wrong */ + if(shp->pwd) + free((void*)shp->pwd); shp->pwd = NIL(const char*); path_pwd(shp,0); errno = saveerrno;