Skip to content

Commit cfc7307

Browse files
committed
sh_exec(): do not save/restore PWD for non-BLT_ENV builtins
The question-everything department reporting for duty once again. Version 2005-05-22 ksh93q+ introduced code that saves the inode and path of the present working directory before invoking a built-in command without the BLT_ENV attribute (see data/builtins.c). When the built-in completes, it gets the PWD's inode again and compares. If they're different, it uses chdir(2) to change back to the old working directory. The corresponding commit in the ksh93-history repo contains no relevant entries in src/cmd/ksh93/RELEASE so no form of rationale for this addition is available. Changing back to a previous directory by path name is unsafe, because the directory may have been removed and even replaced by a completely different one by then. This is a common vector for symlink attacks. In the 93v- beta, this code was improved to use fstat(2) and fchdir(2) to guarantee changing back to the same directory. But is this worth doing at all? Why should a built-in not be able to change the current working directory? If it's not intended to do this, it simply should not do it. Even in the case of dynamically loadable third-party built-ins, we're running built-in code in the same process as ksh, so we've already decided the code is trusted. If it's not, there are far worse things than $PWD to worry about. Not only that, this code comes at a performance hit as the file system is accessed before and after running a non-BLT_ENV builtin. Before removing this: $ arch/*/bin/ksh.old -c 'typeset -i i; \ time for((i=0;i<=100000;i++)); do sleep 0; done >/dev/null' real 0m00.83s user 0m00.40s sys 0m00.42s After removing this: $ arch/*/bin/ksh -c 'typeset -i i; \ time for((i=0;i<=100000;i++)); do sleep 0; done >/dev/null' real 0m00.25s user 0m00.25s sys 0m00.00s Removing this nonsense causes no regressions -- which is obvious. because none of our built-ins except 'cd' change the PWD.
1 parent 148a8a3 commit cfc7307

File tree

1 file changed

+0
-14
lines changed

1 file changed

+0
-14
lines changed

src/cmd/ksh93/sh/xec.c

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1232,11 +1232,9 @@ int sh_exec(register const Shnode_t *t, int flags)
12321232
int save_prompt;
12331233
int was_nofork = execflg?sh_isstate(SH_NOFORK):0;
12341234
struct checkpt *buffp = (struct checkpt*)stkalloc(sh.stk,sizeof(struct checkpt));
1235-
struct stat statb;
12361235
bp = &sh.bltindata;
12371236
save_ptr = bp->ptr;
12381237
save_data = bp->data;
1239-
memset(&statb, 0, sizeof(struct stat));
12401238
if(execflg)
12411239
sh_onstate(SH_NOFORK);
12421240
sh_pushcontext(buffp,SH_JMPCMD);
@@ -1288,10 +1286,6 @@ int sh_exec(register const Shnode_t *t, int flags)
12881286
}
12891287
if(!(nv_isattr(np,BLT_ENV)))
12901288
{
1291-
if(!sh.pwd)
1292-
path_pwd();
1293-
if(sh.pwd)
1294-
stat(e_dot,&statb);
12951289
sfsync(NULL);
12961290
share = sfset(sfstdin,SF_SHARE,0);
12971291
sh_onstate(SH_STOPOK);
@@ -1364,14 +1358,6 @@ int sh_exec(register const Shnode_t *t, int flags)
13641358
sh_offstate(SH_NOFORK);
13651359
if(!(nv_isattr(np,BLT_ENV)))
13661360
{
1367-
if(sh.pwd)
1368-
{
1369-
struct stat stata;
1370-
stat(e_dot,&stata);
1371-
/* restore directory changed */
1372-
if(statb.st_ino!=stata.st_ino || statb.st_dev!=stata.st_dev)
1373-
chdir(sh.pwd);
1374-
}
13751361
sh_offstate(SH_STOPOK);
13761362
if(share&SF_SHARE)
13771363
sfset(sfstdin,SF_PUBLIC|SF_SHARE,1);

0 commit comments

Comments
 (0)