external/cde
1
0
Fork 0
mirror of git://git.code.sf.net/p/cdesktopenv/code synced 2025-02-13 19:52:20 +00:00
Code Issues Releases Wiki Activity

sh_exec(): do not save/restore PWD for non-BLT_ENV builtins

Browse source 
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.
This commit is contained in:
Martijn Dekker 2022-06-12 12:44:17 +01:00
parent 148a8a3f46
commit cfc7307be2
1 changed files with 0 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
src/cmd/ksh93/sh/xec.c
View file

@ -1232,11 +1232,9 @@ int sh_exec(register const Shnode_t *t, int flags)
int save_prompt;
int was_nofork = execflg?sh_isstate(SH_NOFORK):0;
struct checkpt *buffp = (struct checkpt*)stkalloc(sh.stk,sizeof(struct checkpt));
struct stat statb;
bp = &sh.bltindata;
save_ptr = bp->ptr;
save_data = bp->data;
memset(&statb, 0, sizeof(struct stat));
if(execflg)
sh_onstate(SH_NOFORK);
sh_pushcontext(buffp,SH_JMPCMD);
@ -1288,10 +1286,6 @@ int sh_exec(register const Shnode_t *t, int flags)
}
if(!(nv_isattr(np,BLT_ENV)))
{
if(!sh.pwd)
path_pwd();
if(sh.pwd)
stat(e_dot,&statb);
sfsync(NULL);
share = sfset(sfstdin,SF_SHARE,0);
sh_onstate(SH_STOPOK);
@ -1364,14 +1358,6 @@ int sh_exec(register const Shnode_t *t, int flags)
sh_offstate(SH_NOFORK);
if(!(nv_isattr(np,BLT_ENV)))
{
if(sh.pwd)
{
struct stat stata;
stat(e_dot,&stata);
/* restore directory changed */
if(statb.st_ino!=stata.st_ino || statb.st_dev!=stata.st_dev)
chdir(sh.pwd);
}
sh_offstate(SH_STOPOK);
if(share&SF_SHARE)
sfset(sfstdin,SF_PUBLIC|SF_SHARE,1);