cd - shouldn't ignore $OLDPWD when in a new scope (#249)

This bug was first reported at <https://github.com/att/ast/issues/8>.
The 'cd' command currently takes the value of $OLDPWD from the
wrong scope. In the following example 'cd -' will change the
directory to /bin instead of /tmp:

    $ OLDPWD=/bin ksh93 -c 'OLDPWD=/tmp cd -'
    /bin

src/cmd/ksh93/bltins/cd_pwd.c:
- Use sh_scoped() to obtain the correct value of $OLDPWD.
- Fix a use-after-free bug. Make the 'oldpwd' variable a static
  char that points to freeable memory. Each time cd is used, this
  variable is freed if it points to a freeable memory address and
  isn't also a pointer to shp->pwd.

src/cmd/ksh93/sh/path.c: path_pwd():
- Simplify and add comments.
- Scope $PWD properly.

src/cmd/ksh93/tests/builtins.sh,
src/cmd/ksh93/tests/leaks.sh:
- Backport the ksh2020 regression tests for 'cd -' when $OLDPWD is
  set.
- Add test for $OLDPWD and $PWD after subshare.
- Add test for $PWD after 'cd'.
- Add test for possible memory leak.
- Add testing for 'unset' on OLDPWD and PWD.

src/cmd/ksh93/COMPATIBILITY:
- Add compatibility note about changes to $PWD and $OLDPWD.

Co-authored-by: Martijn Dekker <martijn@inlv.org>

NEWS

@@ -3,6 +3,14 @@ For full details, see the git log at: https://github.com/ksh93/ksh
 
 Any uppercase BUG_* names are modernish shell bug IDs.
 
+2021-03-31:
+
+- Fixed a bug that caused 'cd -' to ignore the current value of $OLDPWD
+  when it's set to a different directory in a new scope.
+
+- Fixed a related bug that caused ksh to use the wrong value for $PWD
+  when in a new scope.
+
 2021-03-29:
 
 - Fixed an intermittent crash that could occur in vi mode when using the 'b'

src/cmd/ksh93/COMPATIBILITY

@@ -111,6 +111,12 @@ For more details, see the NEWS file and for complete details, see the git log.
 	Turning on the new --globcasedetect shell option restores
 	case-insensitive globbing for case-insensitive file systems.
 
+22.	If $PWD or $OLDPWD are passed as invocation-local assignments to cd,
+	their values are no longer altered in the outer scope when cd finishes.
+	For example:
+		ksh -c 'OLDPWD=/bin; OLDPWD=/tmp cd - > /dev/null; echo $OLDPWD'
+		ksh -c 'cd /var; PWD=/tmp cd /usr; echo $PWD'
+	now prints '/bin' followed by '/var'.
 ____________________________________________________________________________
 
 		KSH-93 VS. KSH-88

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;
-	char *oldpwd;
+	static char *oldpwd;
 	Namval_t *opwdnod, *pwdnod;
 	if(sh_isoption(SH_RESTRICTED))
 		errormsg(SH_DICT,ERROR_exit(1),e_restricted+4);
@@ -81,9 +81,16 @@ int	b_cd(int argc, char *argv[],Shbltin_t *context)
 	dir =  argv[0];
 	if(error_info.errors>0 || argc >2)
 		errormsg(SH_DICT,ERROR_usage(2),"%s",optusage((char*)0));
+	if(oldpwd && oldpwd!=shp->pwd && oldpwd!=e_dot)
+		free(oldpwd);
 	oldpwd = path_pwd(shp,0);
-	opwdnod = (shp->subshell?sh_assignok(OLDPWDNOD,1):OLDPWDNOD); 
+	opwdnod = sh_scoped(shp,OLDPWDNOD);
-	pwdnod = (shp->subshell?sh_assignok(PWDNOD,1):PWDNOD); 
+	pwdnod = sh_scoped(shp,PWDNOD);
+	if(shp->subshell)
+	{
+		opwdnod = sh_assignok(opwdnod,1);
+		pwdnod = sh_assignok(pwdnod,1);
+	}
 	if(argc==2)
 		dir = sh_substitute(oldpwd,dir,argv[1]);
 	else if(!dir)
@@ -216,8 +223,6 @@ success:
 	nv_scan(sh_subtracktree(1),rehash,(void*)0,NV_TAGGED,NV_TAGGED);
 	path_newdir(shp,shp->pathlist);
 	path_newdir(shp,shp->cdpathlist);
-	if(oldpwd && oldpwd!=e_dot)
-		free(oldpwd);
 	return(0);
 }
 

src/cmd/ksh93/include/version.h

@@ -20,7 +20,7 @@
 
 #define SH_RELEASE_FORK	"93u+m"		/* only change if you develop a new ksh93 fork */
 #define SH_RELEASE_SVER	"1.0.0-alpha"	/* semantic version number: https://semver.org */
-#define SH_RELEASE_DATE	"2021-03-29"	/* must be in this format for $((.sh.version)) */
+#define SH_RELEASE_DATE	"2021-03-31"	/* must be in this format for $((.sh.version)) */
 #define SH_RELEASE_CPYR	"(c) 2020-2021 Contributors to ksh " SH_RELEASE_FORK
 
 /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */

src/cmd/ksh93/sh/path.c

@@ -244,49 +244,35 @@ static pid_t path_xargs(Shell_t *shp,const char *path, char *argv[],char *const
 char *path_pwd(Shell_t *shp,int flag)
 {
 	register char *cp;
-	register int count = 0;
+	Namval_t *pwdnod;
 	NOT_USED(flag);
+	/* Don't bother if PWD already set */
 	if(shp->pwd)
 		return((char*)shp->pwd);
-	while(1) 
+	/* First see if PWD variable is correct */
+	pwdnod = sh_scoped(shp,PWDNOD);
+	cp = nv_getval(pwdnod);
+	if(!(cp && *cp=='/' && test_inode(cp,e_dot)))
 	{
-		/* try from lowest to highest */
+		/* Check if $HOME is a path to the PWD; this ensures $PWD == $HOME
-		switch(count++)
+		   at login, even if $HOME is a path that contains symlinks */
+		cp = nv_getval(sh_scoped(shp,HOME));
+		if(!(cp && *cp=='/' && test_inode(cp,e_dot)))
 		{
-			case 0:
+			/* Get physical PWD (no symlinks) using getcwd(3), fall back to "." */
-				cp = nv_getval(PWDNOD);
+			cp = getcwd(NIL(char*),0);
-				break;
+			if(!cp)
-			case 1:
-				cp = nv_getval(HOME);
-				break;
-			case 2:
-				cp = "/";
-				break;
-			case 3:
-			{
-				if(cp=getcwd(NIL(char*),0))
-				{  
-					nv_offattr(PWDNOD,NV_NOFREE);
-					_nv_unset(PWDNOD,0);
-					PWDNOD->nvalue.cp = cp;
-					goto skip;
-				}
-				break;
-			}
-			case 4:
 				return((char*)e_dot);
 		}
-		if(cp && *cp=='/' && test_inode(cp,e_dot))
+		/* Store in PWD variable */
-			break;
+		if(shp->subshell)
+			pwdnod = sh_assignok(pwdnod,1);
+		nv_offattr(pwdnod,NV_NOFREE);
+		nv_putval(pwdnod,cp,NV_RDONLY);
 	}
-	if(count>1)
+	nv_onattr(pwdnod,NV_NOFREE|NV_EXPORT);
-	{
+	/* Set shell PWD */
-		nv_offattr(PWDNOD,NV_NOFREE);
+	shp->pwd = (char*)(pwdnod->nvalue.cp);
-		nv_putval(PWDNOD,cp,NV_RDONLY);
-	}
-skip:
-	nv_onattr(PWDNOD,NV_NOFREE|NV_EXPORT);
-	shp->pwd = (char*)(PWDNOD->nvalue.cp);
 	return(cp);
 }
 

src/cmd/ksh93/tests/builtins.sh

@@ -1059,5 +1059,85 @@ got=$?
 exp=1
 [[ $got == $exp ]] || err_exit "'kill %' has the wrong exit status (expected '$exp'; got '$got')"
 
+# ======
+# 'cd -' should recognize the value of an overriden $OLDPWD variable
+# https://github.com/ksh93/ksh/pull/249
+# https://github.com/att/ast/issues/8
+
+mkdir "$tmp/oldpwd" "$tmp/otherpwd"
+exp=$tmp/oldpwd
+OLDPWD=$exp
+cd - > /dev/null
+got=$PWD
+[[ $got == "$exp" ]] || err_exit "cd - doesn't recognize overridden OLDPWD variable" \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
+cd "$tmp"
+OLDPWD=$tmp/otherpwd
+got=$(OLDPWD=$tmp/oldpwd cd -)
+[[ $got == "$exp" ]] ||
+	err_exit "cd - doesn't recognize overridden OLDPWD variable if it is overridden in new scope" \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
+function fn
+{
+	typeset OLDPWD=/tmp
+	cd -
+}
+exp='/tmp'
+got=$(OLDPWD=/bin fn)
+[[ $got == "$exp" ]] ||
+	err_exit "cd - doesn't recognize overridden OLDPWD variable if it is overridden in function scope" \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
+function fn
+{
+	typeset PWD=bug
+	cd /tmp
+	echo "$PWD"
+}
+exp='/tmp'
+got=$(fn)
+[[ $got == "$exp" ]] ||
+	err_exit "PWD isn't set after cd if already set in function scope" \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
+# $PWD should be set correctly after cd
+exp="$PWD
+$PWD"
+got=$(echo $PWD; PWD=/tmp cd /home; echo $PWD)
+[[ $got == "$exp" ]] ||
+	err_exit "PWD is incorrect after cd" \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
+# Test for $OLDPWD and/or $PWD leaking out of subshell
+exp='/tmp /dev'
+got=$(
+	PWD=/dev
+	OLDPWD=/tmp
+	(
+		cd /usr; cd /bin
+		cd - > /dev/null
+	)
+	echo $OLDPWD $PWD
+)
+[[ $got == "$exp" ]] ||
+	err_exit "OLDPWD and/or PWD leak out of subshell" \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
+# $OLDPWD and $PWD should survive after being set in a subshare
+exp='/usr /bin'
+got=$(
+	PWD=/dev
+	OLDPWD=/tmp
+	foo=${
+		cd /usr; cd /bin
+	}
+	echo $OLDPWD $PWD
+)
+[[ $got == "$exp" ]] ||
+	err_exit "OLDPWD and/or PWD fail to survive subshare" \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
 # ======
 exit $((Errors<125?Errors:125))

src/cmd/ksh93/tests/leaks.sh

@@ -417,5 +417,27 @@ after=$(getmem)
 err_exit_if_leak 'unset PATH in subshell'
 disabled
 
+# ======
+# Test for a memory leak after 'cd' (in relation to $PWD and $OLDPWD)
+original_pwd=$PWD
+before=$(getmem)
+for ((i=0; i < N; i++))
+do	cd /tmp
+	cd - > /dev/null
+	PWD=/foo
+	OLDPWD=/bar
+	cd /bin
+	cd /usr
+	cd /home
+	cd /home
+	cd - > /dev/null
+	unset OLDPWD PWD
+	cd /bin
+	cd /tmp
+done
+after=$(getmem)
+err_exit_if_leak 'PWD and/or OLDPWD changed by cd'
+cd $original_pwd
+
 # ======
 exit $((Errors<125?Errors:125))