From cfc7307be2435d677bbfeaa0a2dbb81420bd47ac Mon Sep 17 00:00:00 2001
From: Martijn Dekker <martijn@inlv.org>
Date: Sun, 12 Jun 2022 12:44:17 +0100
Subject: [PATCH] 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.
---
 src/cmd/ksh93/sh/xec.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c
index 5789a728d..f313635f4 100644
--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -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);