From 686c381109ff03bb362607eb22308189a91b5ef7 Mon Sep 17 00:00:00 2001
From: Avery Pennarun <apenwarr@gmail.com>
Date: Mon, 17 Dec 2018 15:58:06 +0000
Subject: [PATCH] Fix more inconsistent behaviour with symlinks in paths.

Both redo and minimal/do were doing slightly weird things with
symlinked directories, especially when combined with "..".  For
example, if x is a link to ., then x/x/x/x/../y should resolve to
"../y", which is quite non-obvious.

Added some tests to make sure this stays fixed.
---
 .gitignore                   |  1 +
 clean.do                     |  2 +-
 do                           |  5 +++++
 minimal/do                   | 36 ++++++++++++++++++++++++++++++++++--
 minimal/do.test              | 11 +++++++++++
 redo/state.py                | 22 +++++++++++++++++++---
 t/.gitignore                 |  2 ++
 t/105-sympath/.gitignore     |  5 +++++
 t/105-sympath/all.do         | 31 +++++++++++++++++++++++++++++++
 t/105-sympath/clean.do       |  2 ++
 t/105-sympath/default.dyn.do |  2 ++
 t/clean.do                   |  4 ++--
 t/shelltest.od               |  6 ++++++
 13 files changed, 121 insertions(+), 8 deletions(-)
 create mode 100644 t/105-sympath/.gitignore
 create mode 100644 t/105-sympath/all.do
 create mode 100644 t/105-sympath/clean.do
 create mode 100644 t/105-sympath/default.dyn.do

diff --git a/.gitignore b/.gitignore
index 863b2cb..6a69e34 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,6 +3,7 @@
 /.do_built.dir
 /minimal/.do_built
 /minimal/.do_built.dir
+/minimal/y
 *~
 *.tmp
 *.did
diff --git a/clean.do b/clean.do
index e8f975e..360887f 100644
--- a/clean.do
+++ b/clean.do
@@ -6,7 +6,7 @@ if [ -e .do_built ]; then
 	done <.do_built
 fi
 [ -z "$DO_BUILT" ] && rm -rf .do_built .do_built.dir
-rm -rf minimal/.do_built minimal/.do_built.dir docs.out
+rm -rf minimal/.do_built minimal/.do_built.dir minimal/y docs.out
 redo t/clean docs/clean redo/clean
 rm -f *~ .*~ */*~ */.*~ *.pyc install.wrapper
 find . -name '*.tmp' -exec rm -f {} \;
diff --git a/do b/do
index 223b1ad..ce67bdb 100755
--- a/do
+++ b/do
@@ -66,6 +66,11 @@ case $target in
 		build && bin/redo $args "$target"
 		;;
 	test)
+		# Be intentionally confusing about paths, to try to
+		# detect bugs.
+		rm -f 't/symlink path'
+		ln -s .. 't/symlink path' || die 'failed to make test dir.'
+		cd 't/symlink path/t/symlink path'
 		# First test minimal/do
 		build
 		# Add ./redo to PATH so we launch with redo/sh as the shell
diff --git a/minimal/do b/minimal/do
index 4f634ee..6d6386b 100755
--- a/minimal/do
+++ b/minimal/do
@@ -204,6 +204,38 @@ _normpath()
 )
 
 
+# Prints a "real" path, with all symlinks resolved where possible.
+_realpath()
+{
+	local path="$1" relto="$2" isabs= rest=
+	if _startswith "$path" "/"; then
+		isabs=1
+	else
+		path="${relto%/}/$path"
+	fi
+	(
+		for d in $(seq 100); do
+			#echo "Trying: $PWD--$path" >&2
+			if cd -P "$path" 2>/dev/null; then
+				# success
+				pwd=$(/bin/pwd)
+				#echo "  chdir ok: $pwd--$rest" >&2
+				np=$(_normpath "${pwd%/}/$rest" "$relto")
+				if [ -n "$isabs" ]; then
+					echo "$np"
+				else
+					_relpath "$np" "$relto"
+				fi
+				break
+			fi
+			_dirsplit "${path%/}"
+			path=$_dirsplit_dir
+			rest="$_dirsplit_base/$rest"
+		done
+	)
+}
+
+
 # List the possible names for default*.do files in dir $1 matching the target
 # pattern in $2.  We stop searching when we find the first one that exists.
 _find_dofiles_pwd()
@@ -241,7 +273,7 @@ _find_dofiles()
 		[ -n "$dodir" ] && dodir=${dodir%/}/
 		#echo "_find_dofiles: '$dodir' '$dofile'" >&2
 		_find_dofiles_pwd "$dodir" "$dofile" && return 0
-		newdir=$(_normpath "${dodir}.." "$PWD")
+		newdir=$(_realpath "${dodir}.." "$PWD")
 		[ "$newdir" = "$dodir" ] && break
 		dodir=$newdir
 	done
@@ -369,7 +401,7 @@ _redo()
 		i=$(_abspath "$i" "$startdir")
 		(
 			cd "$DO_STARTDIR" || return 99
-			i=$(_normpath "$(_relpath "$i" "$PWD")" "$PWD")
+			i=$(_realpath "$(_relpath "$i" "$PWD")" "$PWD")
 			_dirsplit "$i"
 			dir=$_dirsplit_dir base=$_dirsplit_base
 			_do "$dir" "$base"
diff --git a/minimal/do.test b/minimal/do.test
index 04fed5f..ceaa1a9 100755
--- a/minimal/do.test
+++ b/minimal/do.test
@@ -130,6 +130,17 @@ check ".." _normpath ../ "$x"
 check ".." _normpath .. "$x"
 
 
+SECTION _realpath
+rm -rf y
+mkdir y
+ln -s . y/x
+check "/usr/__does_not/b"  _realpath "/usr/__does_not/a/../b" "$x"
+check "foo"  _realpath "y/x/x/x/x/x/../foo" "$PWD"
+check "$(/bin/pwd)/foo"  _realpath "$PWD/y/x/x/x/x/x/../foo" "$PWD"
+check "foo/blam"  _realpath "y/x/x/x/x/x/../foo/spam/../blam" "$PWD"
+check "$(/bin/pwd)/foo/blam"  _realpath "$PWD/y/x/x/../foo/spam/../blam" "$PWD"
+
+
 SECTION _find_dofile
 check "test.do"  _find_dofiles test
 check "test.do"  _find_dofile test
diff --git a/redo/state.py b/redo/state.py
index b2270d7..00dd254 100644
--- a/redo/state.py
+++ b/redo/state.py
@@ -151,14 +151,28 @@ def check_sane():
     return not _insane
 
 
+def _realdirpath(t):
+    """Like realpath(), but don't follow symlinks for the last element.
+
+    redo needs this because targets can be symlinks themselves, and we want
+    to talk about the symlink, not what it points at.  However, all the path
+    elements along the way could result in pathname aliases for a *particular*
+    target, so we want to resolve it to one unique name.
+    """
+    dname, fname = os.path.split(t)
+    if dname:
+        dname = os.path.realpath(dname)
+    return os.path.join(dname, fname)
+
+
 _cwd = None
 def relpath(t, base):
     """Given a relative or absolute path t, express it relative to base."""
     global _cwd
     if not _cwd:
         _cwd = os.getcwd()
-    t = os.path.normpath(os.path.join(_cwd, t))
-    base = os.path.normpath(base)
+    t = os.path.normpath(_realdirpath(os.path.join(_cwd, t)))
+    base = os.path.normpath(_realdirpath(base))
     tparts = t.split('/')
     bparts = base.split('/')
     for tp, bp in zip(tparts, bparts):
@@ -172,7 +186,9 @@ def relpath(t, base):
     return join('/', tparts)
 
 
-# Return a path for t, if cwd were the dirname of env.v.TARGET.
+# Return a relative path for t that will work after we do
+# chdir(dirname(env.v.TARGET)).
+#
 # This is tricky!  STARTDIR+PWD is the directory for the *dofile*, when
 # the dofile was started.  However, inside the dofile, someone may have done
 # a chdir to anywhere else.  env.v.TARGET is relative to the dofile path, so
diff --git a/t/.gitignore b/t/.gitignore
index 5206ddf..0ed68c5 100644
--- a/t/.gitignore
+++ b/t/.gitignore
@@ -3,5 +3,7 @@
 /shellfail
 /shelltest.warned
 /shelltest.failed
+/shlink
 /stress.log
+/symlink path
 /flush-cache
diff --git a/t/105-sympath/.gitignore b/t/105-sympath/.gitignore
new file mode 100644
index 0000000..ad4ad3e
--- /dev/null
+++ b/t/105-sympath/.gitignore
@@ -0,0 +1,5 @@
+/*.dyn
+/src
+/x
+/y
+
diff --git a/t/105-sympath/all.do b/t/105-sympath/all.do
new file mode 100644
index 0000000..b2be79a
--- /dev/null
+++ b/t/105-sympath/all.do
@@ -0,0 +1,31 @@
+redo-ifchange ../flush-cache
+rm -f src
+: >src
+
+for iter in 10 20; do
+	rm -rf y
+	rm -f x *.dyn static
+	mkdir y
+	: >y/static
+	ln -s . y/x
+	../flush-cache
+
+	(
+		cd y/x/x/x/x/x
+		IFS=$(printf '\n')
+		redo-ifchange static x/x/x/static $PWD/static \
+			$(/bin/pwd)/static /etc/passwd
+		redo-ifchange $PWD/../static 2>/dev/null && exit 35
+		redo-ifchange 1.dyn x/x/x/2.dyn $PWD/3.dyn \
+			 $PWD/../4.dyn $(/bin/pwd)/5.dyn
+	)
+	[ -e y/1.dyn ] || exit $((iter + 1))
+	[ -e y/2.dyn ] || exit $((iter + 2))
+	[ -e y/3.dyn ] || exit $((iter + 3))
+	[ -e 4.dyn ]   || exit $((iter + 4))
+	[ -e y/5.dyn ] || exit $((iter + 5))
+
+	# Second iteration won't work in minimal/do since it only ever
+	# builds things once.
+	. ../skip-if-minimal-do.sh
+done
diff --git a/t/105-sympath/clean.do b/t/105-sympath/clean.do
new file mode 100644
index 0000000..6b5d773
--- /dev/null
+++ b/t/105-sympath/clean.do
@@ -0,0 +1,2 @@
+rm -rf y
+rm -f src x *.dyn *~ .*~
diff --git a/t/105-sympath/default.dyn.do b/t/105-sympath/default.dyn.do
new file mode 100644
index 0000000..0afe756
--- /dev/null
+++ b/t/105-sympath/default.dyn.do
@@ -0,0 +1,2 @@
+redo-ifchange src
+echo dynamic >$3
diff --git a/t/clean.do b/t/clean.do
index 0267669..9157fbe 100644
--- a/t/clean.do
+++ b/t/clean.do
@@ -2,6 +2,6 @@
 sed 's/\.do$//' |
 xargs redo
 
-rm -f broken shellfile shellfail shelltest.warned shelltest.failed \
-	*~ .*~ stress.log flush-cache
+rm -f broken shellfile shellfail shelltest.warned shelltest.failed shlink \
+	*~ .*~ stress.log flush-cache 'symlink path'
 rm -rf 'space home dir'
diff --git a/t/shelltest.od b/t/shelltest.od
index 8957ee3..259335e 100644
--- a/t/shelltest.od
+++ b/t/shelltest.od
@@ -474,6 +474,12 @@ set x y z
 x=$(printf "a%-5sc" "b")
 [ "$x" = "ab    c" ] || warn 119
 
+# Make sure cd supports -L and -P options properly
+rm -f shlink
+ln -s . shlink
+(cd -L shlink/shlink/shlink/../shlink) || fail 120
+(cd -P shlink/shlink/shlink/../shlink) && fail 121
+
 [ -e shelltest.failed ] && exit 41
 [ -e shelltest.warned ] && exit 42
 exit 40