From feaf718f16725efbd1bc102c9b92c29d476943ba Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Sun, 18 Apr 2021 23:28:50 +0100 Subject: [PATCH] More fixes for subshell directory handling (re: 7bab9508, 5ee290c7) This commit fixes what are hopefully the two final aspects of #153: 1. If the present working directory does not exist (was moved or deleted) upon entering a virtual subshell, no PWD directory path is saved. Since restoring the state after exiting a virtual subshell is contingent on a previous PWD path existing, this resulted in entire aspects of the virtual subshell, such as the subshell function tree, not being cleaned up. 2. A separate problem is that 'cd ..' does not update PWD or OLDPWD when run from a nonexistent directory. A reproducer exposing both problems is: $ mkdir test $ cd test $ ksh -c '(subfn() { BAD; }; cd ..; echo subPWD==$PWD); typeset -f subfn; echo mainPWD==$PWD' subPWD==/usr/local/src/ksh93/ksh/test subfn() { BAD; };mainPWD==/usr/local/src/ksh93/ksh/test Expected output: subPWD==/usr/local/src/ksh93/ksh mainPWD==/usr/local/src/ksh93/ksh/test src/cmd/ksh93/bltins/cd_pwd.c: - If path_pwd() fails to get the PWD (usually it no longer exists), don't set $OLDPWD to '.' as that is pointless; use $PWD instead. After cd'ing from a nonexistent directory, 'cd -' *should* fail and should not be equivalent to 'cd .'. - Remove a redundant check for (!oldpwd) where it is always set. - Do not prematurely return without setting PWD or OLDPWD if pathcanon() fails to canonicalise a nonexistent directory. Instead, fall back to setting PWD to the result of getcwd(3). src/cmd/ksh93/sh/subshell.c: - Minor stylistic adjustment. Some NULL macros sneaked in. This historic code base does not use them (yet); change to NIL(type*). - sh_subshell(): Fix logic for determining whether to save/restore subshell state. 1. When saving, 'if(!comsub || !shp->subshare)' is redundant; 'if(!shp->subshare)' should be enough. If we're not in a subshare, state should be saved. 2. When restoring, 'if(sp->shpwd)' is just nonsense as there is no guarantee that the PWD exists upon entering a subshell. Simply use the same 'if(!shp->subshare)'. Add an extra check for sp->pwd to avoid a possible segfault. Always restore the PWD on subshell exit and not only if shp->pwd is set. - sh_subshell(): Issue fatal errors in libast's "panic" format. src/cmd/ksh93/tests/builtins.sh: - Adjust a relevant test to run err_exit() outside of the subshell so that any error is counted in the main shell. - Add test for problem 2 described at the top. src/cmd/ksh93/tests/subshell.sh: - Add test for problems 1 and 2 based on reproducer above. Resolves: https://github.com/ksh93/ksh/issues/153 --- src/cmd/ksh93/bltins/cd_pwd.c | 35 +++++++++++++++++++++-------- src/cmd/ksh93/sh/subshell.c | 39 +++++++++++++++------------------ src/cmd/ksh93/tests/builtins.sh | 15 +++++++++++-- src/cmd/ksh93/tests/subshell.sh | 16 +++++++++----- 4 files changed, 67 insertions(+), 38 deletions(-) diff --git a/src/cmd/ksh93/bltins/cd_pwd.c b/src/cmd/ksh93/bltins/cd_pwd.c index b14aa31a9..30a39833f 100644 --- a/src/cmd/ksh93/bltins/cd_pwd.c +++ b/src/cmd/ksh93/bltins/cd_pwd.c @@ -92,6 +92,8 @@ int b_cd(int argc, char *argv[],Shbltin_t *context) oldpwd = path_pwd(shp,0); opwdnod = sh_scoped(shp,OLDPWDNOD); pwdnod = sh_scoped(shp,PWDNOD); + if(oldpwd == e_dot && pwdnod->nvalue.cp) + oldpwd = (char*)pwdnod->nvalue.cp; /* if path_pwd() failed to get the pwd, use $PWD */ if(shp->subshell) { opwdnod = sh_assignok(opwdnod,1); @@ -137,8 +139,6 @@ int b_cd(int argc, char *argv[],Shbltin_t *context) cdpath->shp = shp; } } - if(!oldpwd) - oldpwd = path_pwd(shp,1); } if(*dir!='/') { @@ -221,14 +221,31 @@ success: dir = (char*)stakfreeze(1)+PATH_OFFSET; if(*dp && (*dp!='.'||dp[1]) && strchr(dir,'/')) sfputr(sfstdout,dir,'\n'); - if(*dir != '/') - return(0); nv_putval(opwdnod,oldpwd,NV_RDONLY); - flag = strlen(dir); - /* delete trailing '/' */ - while(--flag>0 && dir[flag]=='/') - dir[flag] = 0; - nv_putval(pwdnod,dir,NV_RDONLY); + if(*dir == '/') + { + flag = strlen(dir); + /* delete trailing '/' */ + while(--flag>0 && dir[flag]=='/') + dir[flag] = 0; + nv_putval(pwdnod,dir,NV_RDONLY); + } + 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 + { + 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); diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c index 4679ea7e0..344b5e8b5 100644 --- a/src/cmd/ksh93/sh/subshell.c +++ b/src/cmd/ksh93/sh/subshell.c @@ -219,7 +219,7 @@ void sh_subfork(void) shp->subshell = 0; shp->comsub = 0; sp->subpid=0; - shp->st.trapcom[0] = (comsub==2 ? NULL : trap); + shp->st.trapcom[0] = (comsub==2 ? NIL(char*) : trap); shp->savesig = 0; /* sh_fork() increases ${.sh.subshell} but we forked an existing virtual subshell, so undo */ shgd->realsubshell--; @@ -581,7 +581,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) shp->comsub = comsub; job.bktick_waitall = (comsub==1); } - if(!comsub || !shp->subshare) + if(!shp->subshare) { struct subshell *xp; sp->shpwd = shp->pwd; @@ -634,9 +634,9 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) else if(shp->st.trapcom[isig]) savsig[isig] = sh_strdup(shp->st.trapcom[isig]); else - savsig[isig] = NULL; + savsig[isig] = NIL(char*); } - /* this nonsense needed for $(trap) */ + /* this is needed for var=$(trap) */ shp->st.otrapcom = (char**)savsig; } sp->cpid = shp->cpid; @@ -794,7 +794,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) job.curpgid = savejobpgid; job.exitval = saveexitval; shp->bckpid = sp->bckpid; - if(sp->shpwd) /* restore environment if saved */ + if(!shp->subshare) /* restore environment if saved */ { int n; shp->options = sp->options; @@ -842,24 +842,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 || strcmp(sp->pwd,shp->pwd)) + if(shp->pwd != sp->pwd && (!shp->pwd || !sp->pwd || strcmp(sp->pwd,shp->pwd))) { - /* restore PWDNOD */ + /* restore the present working directory */ Namval_t *pwdnod = sh_scoped(shp,PWDNOD); - if(shp->pwd) - { #if _lib_fchdir - if(fchdir(sp->pwdfd) < 0) + if(fchdir(sp->pwdfd) < 0) #else - if(chdir(sp->pwd) < 0) + if(!sp->pwd || chdir(sp->pwd) < 0) #endif /* _lib_fchdir */ - { - saveerrno = errno; - fatalerror = 2; - } - shp->pwd=sp->pwd; - path_newdir(shp,shp->pathlist); + { + 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; } @@ -939,17 +936,17 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) case 1: shp->toomany = 1; errno = saveerrno; - errormsg(SH_DICT,ERROR_system(1),e_redirect); + errormsg(SH_DICT,ERROR_SYSTEM|ERROR_PANIC,e_redirect); UNREACHABLE(); case 2: /* reinit PWD as it will be wrong */ - shp->pwd = NULL; + shp->pwd = NIL(const char*); path_pwd(shp,0); errno = saveerrno; - errormsg(SH_DICT,ERROR_system(1),"Failed to restore PWD upon exiting subshell"); + errormsg(SH_DICT,ERROR_SYSTEM|ERROR_PANIC,"Failed to restore PWD upon exiting subshell"); UNREACHABLE(); default: - errormsg(SH_DICT,ERROR_system(1),"Subshell error %d",fatalerror); + errormsg(SH_DICT,ERROR_SYSTEM|ERROR_PANIC,"Subshell error %d",fatalerror); UNREACHABLE(); } } diff --git a/src/cmd/ksh93/tests/builtins.sh b/src/cmd/ksh93/tests/builtins.sh index e37b211a0..bd70f010d 100755 --- a/src/cmd/ksh93/tests/builtins.sh +++ b/src/cmd/ksh93/tests/builtins.sh @@ -671,8 +671,8 @@ then ( mv t1 t2 mkdir t1 ) - [[ -f real_t1 ]] || err_exit 'real_t1 not found after parent directory renamed in subshell' - ) + [[ -f real_t1 ]] + ) || err_exit 'real_t1 not found after parent directory renamed in subshell' fi cd "$tmp" @@ -1206,5 +1206,16 @@ then exp=' version cat (*) ????-??-??' else warning 'skipping path-bound builtin tests: builtin /opt/ast/bin/cat not found' fi +# ====== +# part of https://github.com/ksh93/ksh/issues/153 +mkdir "$tmp/deleted" +cd "$tmp/deleted" +tmp=$tmp "$SHELL" -c 'cd /; rmdir "$tmp/deleted"' +exp=$PWD +got=$("$SHELL" -c 'cd /; echo "$OLDPWD"' 2>&1) +[[ $got == "$exp" ]] || err_exit "OLDPWD not correct after cd'ing from a nonexistent PWD" \ + "(expected $(printf %q "$exp"), got $(printf %q "$got"))" +cd "$tmp" + # ====== exit $((Errors<125?Errors:125)) diff --git a/src/cmd/ksh93/tests/subshell.sh b/src/cmd/ksh93/tests/subshell.sh index 58df83020..968a29f62 100755 --- a/src/cmd/ksh93/tests/subshell.sh +++ b/src/cmd/ksh93/tests/subshell.sh @@ -887,17 +887,21 @@ got=$( { "$SHELL" "$tmp/crash_rhbz1117404.ksh"; } 2>&1) # ====== # Segmentation fault when using cd in a subshell, when current directory cannot be determined # https://github.com/ksh93/ksh/issues/153 -cd "$tmp" -mkdir deleted -cd deleted +mkdir "$tmp/deleted" +cd "$tmp/deleted" tmp=$tmp "$SHELL" -c 'cd /; rmdir "$tmp/deleted"' + +exp="subPWD: ${PWD%/deleted}"$'\n'"mainPWD: $PWD" +got=$( { "$SHELL" -c '(subshfn() { bad; }; cd ..; echo "subPWD: $PWD"); typeset -f subshfn; echo "mainPWD: $PWD"'; } 2>&1 ) +[[ $got == "$exp" ]] || err_exit "subshell state not restored after 'cd ..' from deleted PWD" \ + "(expected $(printf %q "$exp"), got $(printf %q "$got"))" + exp="PWD=$PWD" got=$( { "$SHELL" -c '(cd /; (cd /)); print -r -- "PWD=$PWD"'; } 2>&1 ) ((!(e = $?))) && [[ $got == "$exp" ]] || err_exit 'failed to restore nonexistent PWD on exiting a virtual subshell' \ "(got status $e$( ((e>128)) && print -n / && kill -l "$e"), $(printf %q "$got"))" -cd "$tmp" -mkdir recreated -cd recreated +mkdir "$tmp/recreated" +cd "$tmp/recreated" tmp=$tmp "$SHELL" -c 'cd /; rmdir "$tmp/recreated"; mkdir "$tmp/recreated"' exp="PWD=$PWD" got=$( { "$SHELL" -c '(cd /); print -r -- "PWD=$PWD"'; } 2>&1 )